From 5f2127029c1f8db458002fd2f0c410355f302500 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Fri, 17 Jul 2020 16:17:00 -0700 Subject: [PATCH 1/6] Convert model:// to Fuel URLs instead of absolute paths Signed-off-by: Louise Poubel --- src/FuelClient_TEST.cc | 25 ++++++---- src/LocalCache.cc | 103 +++++++++++++++++++++++++---------------- 2 files changed, 79 insertions(+), 49 deletions(-) diff --git a/src/FuelClient_TEST.cc b/src/FuelClient_TEST.cc index 9daac5ef..03e2681c 100644 --- a/src/FuelClient_TEST.cc +++ b/src/FuelClient_TEST.cc @@ -473,10 +473,10 @@ TEST_F(FuelClientTest, DownloadModel) "/test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2"); EXPECT_TRUE(common::exists( "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2")); - const std::string model_sdf_path = + const std::string modelSdfPath = "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2/" "model.sdf"; - EXPECT_TRUE(common::exists(model_sdf_path)); + EXPECT_TRUE(common::exists(modelSdfPath)); EXPECT_TRUE(common::exists( "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2/" "model.config")); @@ -494,14 +494,21 @@ TEST_F(FuelClientTest, DownloadModel) "/test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2", cachedPath); - // Check that pbr paths have been updated. - std::ifstream ifs(model_sdf_path); - std::string model_sdf((std::istreambuf_iterator(ifs)), + // Check that URIs have been updated. + std::ifstream ifs(modelSdfPath); + std::string modelSdf((std::istreambuf_iterator(ifs)), std::istreambuf_iterator()); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); } // Try using nonexistent URL diff --git a/src/LocalCache.cc b/src/LocalCache.cc index 61c18bcc..0e5c3709 100644 --- a/src/LocalCache.cc +++ b/src/LocalCache.cc @@ -57,34 +57,36 @@ class ignition::fuel_tools::LocalCachePrivate /// \brief return all models in a given Owner/models directory public: std::vector ModelsInPath(const std::string &_path); - /// \brief Associate model:// URI paths with paths on disk + /// \brief Associate model:// URI paths with Fuel URLs. /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model's Fuel URL. /// \return True if the paths were fixed. False could occur if the /// `model.config` file is not present or contains XML errors. - public: bool FixPaths(const std::string &_modelVersionedDir); + public: bool FixPaths(const std::string &_modelVersionedDir, + const ModelIdentifier &_id); /// \brief Helper function to fix model:// URI paths in geometry elements. /// \param[in] _geomElem Pointer to the geometry element. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief Helper function to fix model:// URI paths in material elements. /// \param[in] _matElem Pointer to the material element. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInMaterialElement( tinyxml2::XMLElement *_matElem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief Helper function to fix a single model:// URI that is contained /// in an element. /// \param[in] _elem Pointer to an element tha contains a URI. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInUri(tinyxml2::XMLElement *_elem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief client configuration public: const ClientConfig *config = nullptr; @@ -413,8 +415,8 @@ bool LocalCache::SaveModel( return false; } - // Convert model:// URIs to locations on disk. - this->dataPtr->FixPaths(modelVersionedDir); + // Convert model:// URIs to Fuel URLs + this->dataPtr->FixPaths(modelVersionedDir, _id); // Cleanup the zip file. if (!common::removeDirectoryOrFile(zipFile)) @@ -426,7 +428,8 @@ bool LocalCache::SaveModel( } ////////////////////////////////////////////////// -bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) +bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir, + const ModelIdentifier &_id) { // Get model.config std::string modelConfigPath = common::joinPaths( @@ -498,7 +501,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) while (collisionElem) { this->FixPathsInGeomElement( - collisionElem->FirstChildElement("geometry"), _modelVersionedDir); + collisionElem->FirstChildElement("geometry"), _id); // Next collision element. collisionElem = collisionElem->NextSiblingElement("collision"); } @@ -509,9 +512,9 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) while (visualElem) { this->FixPathsInGeomElement( - visualElem->FirstChildElement("geometry"), _modelVersionedDir); + visualElem->FirstChildElement("geometry"), _id); this->FixPathsInMaterialElement( - visualElem->FirstChildElement("material"), _modelVersionedDir); + visualElem->FirstChildElement("material"), _id); visualElem = visualElem->NextSiblingElement("visual"); } linkElem = linkElem->NextSiblingElement("link"); @@ -531,7 +534,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) auto filenameElem = skinElem->FirstChildElement("filename"); if (filenameElem) { - this->FixPathsInUri(filenameElem, _modelVersionedDir); + this->FixPathsInUri(filenameElem, _id); } skinElem = skinElem->NextSiblingElement("skin"); } @@ -543,7 +546,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) auto filenameElem = animationElem->FirstChildElement("filename"); if (filenameElem) { - this->FixPathsInUri(filenameElem, _modelVersionedDir); + this->FixPathsInUri(filenameElem, _id); } animationElem = animationElem->NextSiblingElement("animation"); } @@ -556,34 +559,54 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInUri(tinyxml2::XMLElement *_elem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_elem) return; - std::string uri = _elem->GetText(); + std::string oldUri = _elem->GetText(); std::string prefix = "model://"; // Make sure the URI is of the form model:// - if (uri.find(prefix) != std::string::npos) - { - int firstSlash = uri.find('/', prefix.size()+1); - std::string suffix = uri.substr(firstSlash); + if (oldUri.find(prefix) == std::string::npos) + return; - // Convert the model:// to point to an actual file. - // \todo(nkoenig) Handle URIs to other models. For - // example, ModelA may use something from ModelB. - std::string diskPath = common::joinPaths("file:/", - _modelVersionedDir, suffix); + int firstSlash = oldUri.find('/', prefix.size()+1); - _elem->SetText(diskPath.c_str()); + auto resourceName = oldUri.substr(prefix.size(), firstSlash - prefix.size()); + + if (resourceName != _id.Name()) + { + // On Blueprint and Citadel, just warn the user + // From Dome, use the name on the URI (resourceName) and assume the same + // owner + igndbg << "Model [" << _id.Name() + << "] loading resource from another model, named [" << resourceName + << "]. On Blueprint and Citadel, [" << resourceName << "] is ignored. " + << "From Dome, [" << _id.Name() << "] will be used. If [" + << resourceName << "] is not a model belonging to owner [" + << _id.Owner() << "], fix your SDF file!" << std::endl; } + + auto filePath = oldUri.substr(firstSlash); + + auto fuelUrl = + _id.Server().Url().Str() + '/' + + _id.Server().Version() + '/' + + _id.Owner() + + "/models/" + + _id.Name() + '/' + + _id.VersionStr() + + "/files/" + + filePath; + + _elem->SetText(fuelUrl.c_str()); } ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *_matElem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_matElem) return; @@ -597,7 +620,7 @@ void LocalCachePrivate::FixPathsInMaterialElement( // Convert the "model://" URI pattern to file:// while (uriElem) { - this->FixPathsInUri(uriElem, _modelVersionedDir); + this->FixPathsInUri(uriElem, _id); uriElem = uriElem->NextSiblingElement("uri"); } } @@ -616,30 +639,30 @@ void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *albedoElem = workflowElem->FirstChildElement("albedo_map"); if (albedoElem) - this->FixPathsInUri(albedoElem, _modelVersionedDir); + this->FixPathsInUri(albedoElem, _id); tinyxml2::XMLElement *normalElem = workflowElem->FirstChildElement("normal_map"); if (normalElem) - this->FixPathsInUri(normalElem, _modelVersionedDir); + this->FixPathsInUri(normalElem, _id); tinyxml2::XMLElement *envElem = workflowElem->FirstChildElement("environment_map"); if (envElem) - this->FixPathsInUri(envElem, _modelVersionedDir); + this->FixPathsInUri(envElem, _id); tinyxml2::XMLElement *emissiveElem = workflowElem->FirstChildElement("emissive_map"); if (emissiveElem) - this->FixPathsInUri(emissiveElem, _modelVersionedDir); + this->FixPathsInUri(emissiveElem, _id); // metal workflow specific elements if (workflow == "metal") { tinyxml2::XMLElement *metalnessElem = workflowElem->FirstChildElement("metalness_map"); if (metalnessElem) - this->FixPathsInUri(metalnessElem, _modelVersionedDir); + this->FixPathsInUri(metalnessElem, _id); tinyxml2::XMLElement *roughnessElem = workflowElem->FirstChildElement("roughness_map"); if (roughnessElem) - this->FixPathsInUri(roughnessElem, _modelVersionedDir); + this->FixPathsInUri(roughnessElem, _id); } // specular workflow specific elements else if (workflow == "specular") @@ -647,11 +670,11 @@ void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *specularElem = workflowElem->FirstChildElement("specular_map"); if (specularElem) - this->FixPathsInUri(specularElem, _modelVersionedDir); + this->FixPathsInUri(specularElem, _id); tinyxml2::XMLElement *glossinessElem = workflowElem->FirstChildElement("glossiness_map"); if (glossinessElem) - this->FixPathsInUri(glossinessElem, _modelVersionedDir); + this->FixPathsInUri(glossinessElem, _id); } } } @@ -661,7 +684,7 @@ void LocalCachePrivate::FixPathsInMaterialElement( ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_geomElem) return; @@ -673,7 +696,7 @@ void LocalCachePrivate::FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, { tinyxml2::XMLElement *uriElem = meshElem->FirstChildElement("uri"); // Convert the "model://" URI pattern to file:// - this->FixPathsInUri(uriElem, _modelVersionedDir); + this->FixPathsInUri(uriElem, _id); } } From ed4f7882a080088a00fd7c56f05a0bb11fe5e0a0 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Fri, 17 Jul 2020 16:46:56 -0700 Subject: [PATCH 2/6] Backport file fetching Signed-off-by: Louise Poubel --- src/Interface.cc | 20 ++++ src/Interface_TEST.cc | 238 +++++++++++++++++++++++++++++++++++------- 2 files changed, 222 insertions(+), 36 deletions(-) diff --git a/src/Interface.cc b/src/Interface.cc index 4434be31..c2cf3a6f 100644 --- a/src/Interface.cc +++ b/src/Interface.cc @@ -15,6 +15,7 @@ * */ +#include "ignition/common/Console.hh" #include "ignition/fuel_tools/Interface.hh" #include "ignition/fuel_tools/WorldIdentifier.hh" @@ -37,6 +38,7 @@ namespace ignition ignition::fuel_tools::ModelIdentifier model; ignition::fuel_tools::WorldIdentifier world; + std::string fileUrl; ignition::common::URI uri(_uri); // Download the model, if it is a model URI if (_client.ParseModelUrl(uri, model) && @@ -44,12 +46,30 @@ namespace ignition { _client.DownloadModel(uri, result); } + // Download the model, if it's a model file URI + else if (_client.ParseModelFileUrl(uri, model, fileUrl) && + !_client.CachedModelFile(uri, result)) + { + auto modelUri = _uri.substr(0, + _uri.find("files", model.UniqueName().size())-1); + _client.DownloadModel(common::URI(modelUri), result); + result += "/" + fileUrl; + } // Download the world, if it is a world URI else if (_client.ParseWorldUrl(uri, world) && !_client.CachedWorld(uri, result)) { _client.DownloadWorld(uri, result); } + // Download the world, if it's a world file URI + else if (_client.ParseWorldFileUrl(uri, world, fileUrl) && + !_client.CachedWorldFile(uri, result)) + { + auto worldUri = _uri.substr(0, + _uri.find("files", world.UniqueName().size())-1); + _client.DownloadWorld(common::URI(worldUri), result); + result += "/" + fileUrl; + } return result; } diff --git a/src/Interface_TEST.cc b/src/Interface_TEST.cc index bfb54220..62bef2bc 100644 --- a/src/Interface_TEST.cc +++ b/src/Interface_TEST.cc @@ -16,6 +16,7 @@ */ #include +#include #include #include "ignition/fuel_tools/ClientConfig.hh" #include "ignition/fuel_tools/FuelClient.hh" @@ -35,8 +36,10 @@ using namespace ignition; using namespace ignition::fuel_tools; ///////////////////////////////////////////////// -TEST(Interface, FetchResource) +TEST(Interface, FetchResources) { + common::Console::SetVerbosity(4); + // Configure to use binary path as cache ASSERT_EQ(0, ChangeDirectory(PROJECT_BINARY_PATH)); common::removeAll("test_cache"); @@ -44,44 +47,207 @@ TEST(Interface, FetchResource) ClientConfig config; config.SetCacheLocation(common::cwd() + "/test_cache"); - common::URI url{ - "https://fuel.ignitionrobotics.org/1.0/chapulina/models/Test box"}; - // Create client FuelClient client(config); EXPECT_EQ(config.CacheLocation(), client.Config().CacheLocation()); - // Check it is not cached std::string cachedPath; - Result res1 = client.CachedModel(url, cachedPath); - EXPECT_FALSE(res1) << "Cached Path: " << cachedPath; - EXPECT_EQ(Result(ResultType::FETCH_ERROR), res1); - - // Download - std::string path = fetchResourceWithClient(url.Str(), client); - - // Check it was downloaded to `2` - EXPECT_EQ(path, common::cwd() + - "/test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2"); - EXPECT_TRUE(common::exists( - "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2")); - EXPECT_TRUE(common::exists( - "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2/" - "model.sdf")); - EXPECT_TRUE(common::exists( - "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2/" - "model.config")); - - // Check it wasn't downloaded to model root directory - EXPECT_FALSE(common::exists( - "test_cache/fuel.ignitionrobotics.org/chapulina/models" - "/Test box/model.config")); - - // Check it is cached - Result res3 = client.CachedModel(url, cachedPath); - EXPECT_TRUE(res3); - EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res3); - EXPECT_EQ(common::cwd() + - "/test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2", - cachedPath); + + // Model + { + // Check it's not cached + common::URI modelUrl{ + "https://fuel.ignitionrobotics.org/1.0/chapulina/models/Test box"}; + { + Result res = client.CachedModel(modelUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + + // Download model + std::string path = fetchResourceWithClient(modelUrl.Str(), client); + + // Check it was downloaded to `2` + EXPECT_EQ(path, common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2"); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2/" + "model.sdf")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2/" + "model.config")); + + // Check it wasn't downloaded to model root directory + EXPECT_FALSE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/models" + "/Test box/model.config")); + + // Check it is cached + { + Result res = client.CachedModel(modelUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/chapulina/models/Test box/2", + cachedPath); + } + } + + // Model file + { + // Check neither file nor its model are cached + common::URI modelUrl{ + "https://fuel.ignitionrobotics.org/1.0/openrobotics/models/Bus/1/"}; + common::URI modelFileUrl{ + "https://fuel.ignitionrobotics.org/1.0/openrobotics/models/Bus/1/files" + "/meshes/bus.obj"}; + + { + Result res = client.CachedModel(modelUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + { + Result res = client.CachedModelFile(modelFileUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + + // Download model file + std::string path = fetchResourceWithClient(modelFileUrl.Str(), client); + + // Check entire model was downloaded to `1` + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1")); + EXPECT_EQ(path, common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "meshes/bus.obj"); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "model.sdf")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "model.config")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "meshes/bus.obj")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "meshes/bus.mtl")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1/" + "materials/textures/bus.png")); + + // Check model is cached + { + Result res = client.CachedModel(modelUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1", + cachedPath); + } + + // Check file is cached + { + Result res = client.CachedModelFile(modelFileUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/openrobotics/models/Bus/1" + "/meshes/bus.obj", + cachedPath); + } + } + + // World + { + // Check it's not cached + common::URI worldUrl{ + "https://fuel.ignitionrobotics.org/1.0/nate/worlds/Empty"}; + { + Result res = client.CachedWorld(worldUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + + // Download world + std::string path = fetchResourceWithClient(worldUrl.Str(), client); + + // Check it was downloaded to `1` + EXPECT_EQ(path, common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/nate/worlds/Empty/1"); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/nate/worlds/Empty/1")); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/nate/worlds/Empty/1/" + "empty.world")); + + // Check it is cached + { + Result res = client.CachedWorld(worldUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/nate/worlds/Empty/1", + cachedPath); + } + } + + // World file + { + // Check neither file nor its world are cached + common::URI worldUrl{ + "https://fuel.ignitionrobotics.org/1.0/chapulina/worlds/Test world/1/"}; + common::URI worldFileUrl{ + "https://fuel.ignitionrobotics.org/1.0/chapulina/worlds/Test world/1/" + "files/thumbnails/1.png"}; + + { + Result res = client.CachedWorld(worldUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + { + Result res = client.CachedWorldFile(worldFileUrl, cachedPath); + EXPECT_FALSE(res) << "Cached Path: " << cachedPath; + EXPECT_EQ(Result(ResultType::FETCH_ERROR), res); + } + + // Download world file + std::string path = fetchResourceWithClient(worldFileUrl.Str(), client); + + // Check entire world was downloaded to `1` + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/worlds/Test world/1")); + EXPECT_EQ(path, common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/chapulina/worlds/Test world/1/" + "thumbnails/1.png"); + EXPECT_TRUE(common::exists( + "test_cache/fuel.ignitionrobotics.org/chapulina/worlds/Test world/1/" + "test.world")); + + // Check world is cached + { + Result res = client.CachedWorld(worldUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/chapulina/worlds/Test world/1", + cachedPath); + } + + // Check file is cached + { + Result res = client.CachedWorldFile(worldFileUrl, cachedPath); + EXPECT_TRUE(res); + EXPECT_EQ(Result(ResultType::FETCH_ALREADY_EXISTS), res); + EXPECT_EQ(common::cwd() + + "/test_cache/fuel.ignitionrobotics.org/chapulina/worlds/Test world/1" + "/thumbnails/1.png", + cachedPath); + } + } } From 9e63e1493060732ea5771ea85f0176dd7e3d2f37 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Fri, 17 Jul 2020 16:17:00 -0700 Subject: [PATCH 3/6] Convert model:// to Fuel URLs instead of absolute paths Signed-off-by: Louise Poubel --- src/FuelClient_TEST.cc | 25 ++++++---- src/LocalCache.cc | 103 +++++++++++++++++++++++++---------------- 2 files changed, 79 insertions(+), 49 deletions(-) diff --git a/src/FuelClient_TEST.cc b/src/FuelClient_TEST.cc index 9daac5ef..03e2681c 100644 --- a/src/FuelClient_TEST.cc +++ b/src/FuelClient_TEST.cc @@ -473,10 +473,10 @@ TEST_F(FuelClientTest, DownloadModel) "/test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2"); EXPECT_TRUE(common::exists( "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2")); - const std::string model_sdf_path = + const std::string modelSdfPath = "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2/" "model.sdf"; - EXPECT_TRUE(common::exists(model_sdf_path)); + EXPECT_TRUE(common::exists(modelSdfPath)); EXPECT_TRUE(common::exists( "test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2/" "model.config")); @@ -494,14 +494,21 @@ TEST_F(FuelClientTest, DownloadModel) "/test_cache/fuel.ignitionrobotics.org/iche033/models/Rescue Randy/2", cachedPath); - // Check that pbr paths have been updated. - std::ifstream ifs(model_sdf_path); - std::string model_sdf((std::istreambuf_iterator(ifs)), + // Check that URIs have been updated. + std::ifstream ifs(modelSdfPath); + std::string modelSdf((std::istreambuf_iterator(ifs)), std::istreambuf_iterator()); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); - EXPECT_EQ(std::string::npos, model_sdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + EXPECT_EQ(std::string::npos, modelSdf.find("model://")); + + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); + EXPECT_NE(std::string::npos, modelSdf.find("https://")); } // Try using nonexistent URL diff --git a/src/LocalCache.cc b/src/LocalCache.cc index 61c18bcc..0e5c3709 100644 --- a/src/LocalCache.cc +++ b/src/LocalCache.cc @@ -57,34 +57,36 @@ class ignition::fuel_tools::LocalCachePrivate /// \brief return all models in a given Owner/models directory public: std::vector ModelsInPath(const std::string &_path); - /// \brief Associate model:// URI paths with paths on disk + /// \brief Associate model:// URI paths with Fuel URLs. /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model's Fuel URL. /// \return True if the paths were fixed. False could occur if the /// `model.config` file is not present or contains XML errors. - public: bool FixPaths(const std::string &_modelVersionedDir); + public: bool FixPaths(const std::string &_modelVersionedDir, + const ModelIdentifier &_id); /// \brief Helper function to fix model:// URI paths in geometry elements. /// \param[in] _geomElem Pointer to the geometry element. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief Helper function to fix model:// URI paths in material elements. /// \param[in] _matElem Pointer to the material element. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInMaterialElement( tinyxml2::XMLElement *_matElem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief Helper function to fix a single model:// URI that is contained /// in an element. /// \param[in] _elem Pointer to an element tha contains a URI. - /// \param[in] _modelVersionedDir Directory containing the model. + /// \param[in] _id Model identifier /// \sa FixPaths public: void FixPathsInUri(tinyxml2::XMLElement *_elem, - const std::string &_modelVersionedDir); + const ModelIdentifier &_id); /// \brief client configuration public: const ClientConfig *config = nullptr; @@ -413,8 +415,8 @@ bool LocalCache::SaveModel( return false; } - // Convert model:// URIs to locations on disk. - this->dataPtr->FixPaths(modelVersionedDir); + // Convert model:// URIs to Fuel URLs + this->dataPtr->FixPaths(modelVersionedDir, _id); // Cleanup the zip file. if (!common::removeDirectoryOrFile(zipFile)) @@ -426,7 +428,8 @@ bool LocalCache::SaveModel( } ////////////////////////////////////////////////// -bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) +bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir, + const ModelIdentifier &_id) { // Get model.config std::string modelConfigPath = common::joinPaths( @@ -498,7 +501,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) while (collisionElem) { this->FixPathsInGeomElement( - collisionElem->FirstChildElement("geometry"), _modelVersionedDir); + collisionElem->FirstChildElement("geometry"), _id); // Next collision element. collisionElem = collisionElem->NextSiblingElement("collision"); } @@ -509,9 +512,9 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) while (visualElem) { this->FixPathsInGeomElement( - visualElem->FirstChildElement("geometry"), _modelVersionedDir); + visualElem->FirstChildElement("geometry"), _id); this->FixPathsInMaterialElement( - visualElem->FirstChildElement("material"), _modelVersionedDir); + visualElem->FirstChildElement("material"), _id); visualElem = visualElem->NextSiblingElement("visual"); } linkElem = linkElem->NextSiblingElement("link"); @@ -531,7 +534,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) auto filenameElem = skinElem->FirstChildElement("filename"); if (filenameElem) { - this->FixPathsInUri(filenameElem, _modelVersionedDir); + this->FixPathsInUri(filenameElem, _id); } skinElem = skinElem->NextSiblingElement("skin"); } @@ -543,7 +546,7 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) auto filenameElem = animationElem->FirstChildElement("filename"); if (filenameElem) { - this->FixPathsInUri(filenameElem, _modelVersionedDir); + this->FixPathsInUri(filenameElem, _id); } animationElem = animationElem->NextSiblingElement("animation"); } @@ -556,34 +559,54 @@ bool LocalCachePrivate::FixPaths(const std::string &_modelVersionedDir) ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInUri(tinyxml2::XMLElement *_elem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_elem) return; - std::string uri = _elem->GetText(); + std::string oldUri = _elem->GetText(); std::string prefix = "model://"; // Make sure the URI is of the form model:// - if (uri.find(prefix) != std::string::npos) - { - int firstSlash = uri.find('/', prefix.size()+1); - std::string suffix = uri.substr(firstSlash); + if (oldUri.find(prefix) == std::string::npos) + return; - // Convert the model:// to point to an actual file. - // \todo(nkoenig) Handle URIs to other models. For - // example, ModelA may use something from ModelB. - std::string diskPath = common::joinPaths("file:/", - _modelVersionedDir, suffix); + int firstSlash = oldUri.find('/', prefix.size()+1); - _elem->SetText(diskPath.c_str()); + auto resourceName = oldUri.substr(prefix.size(), firstSlash - prefix.size()); + + if (resourceName != _id.Name()) + { + // On Blueprint and Citadel, just warn the user + // From Dome, use the name on the URI (resourceName) and assume the same + // owner + igndbg << "Model [" << _id.Name() + << "] loading resource from another model, named [" << resourceName + << "]. On Blueprint and Citadel, [" << resourceName << "] is ignored. " + << "From Dome, [" << _id.Name() << "] will be used. If [" + << resourceName << "] is not a model belonging to owner [" + << _id.Owner() << "], fix your SDF file!" << std::endl; } + + auto filePath = oldUri.substr(firstSlash); + + auto fuelUrl = + _id.Server().Url().Str() + '/' + + _id.Server().Version() + '/' + + _id.Owner() + + "/models/" + + _id.Name() + '/' + + _id.VersionStr() + + "/files/" + + filePath; + + _elem->SetText(fuelUrl.c_str()); } ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *_matElem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_matElem) return; @@ -597,7 +620,7 @@ void LocalCachePrivate::FixPathsInMaterialElement( // Convert the "model://" URI pattern to file:// while (uriElem) { - this->FixPathsInUri(uriElem, _modelVersionedDir); + this->FixPathsInUri(uriElem, _id); uriElem = uriElem->NextSiblingElement("uri"); } } @@ -616,30 +639,30 @@ void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *albedoElem = workflowElem->FirstChildElement("albedo_map"); if (albedoElem) - this->FixPathsInUri(albedoElem, _modelVersionedDir); + this->FixPathsInUri(albedoElem, _id); tinyxml2::XMLElement *normalElem = workflowElem->FirstChildElement("normal_map"); if (normalElem) - this->FixPathsInUri(normalElem, _modelVersionedDir); + this->FixPathsInUri(normalElem, _id); tinyxml2::XMLElement *envElem = workflowElem->FirstChildElement("environment_map"); if (envElem) - this->FixPathsInUri(envElem, _modelVersionedDir); + this->FixPathsInUri(envElem, _id); tinyxml2::XMLElement *emissiveElem = workflowElem->FirstChildElement("emissive_map"); if (emissiveElem) - this->FixPathsInUri(emissiveElem, _modelVersionedDir); + this->FixPathsInUri(emissiveElem, _id); // metal workflow specific elements if (workflow == "metal") { tinyxml2::XMLElement *metalnessElem = workflowElem->FirstChildElement("metalness_map"); if (metalnessElem) - this->FixPathsInUri(metalnessElem, _modelVersionedDir); + this->FixPathsInUri(metalnessElem, _id); tinyxml2::XMLElement *roughnessElem = workflowElem->FirstChildElement("roughness_map"); if (roughnessElem) - this->FixPathsInUri(roughnessElem, _modelVersionedDir); + this->FixPathsInUri(roughnessElem, _id); } // specular workflow specific elements else if (workflow == "specular") @@ -647,11 +670,11 @@ void LocalCachePrivate::FixPathsInMaterialElement( tinyxml2::XMLElement *specularElem = workflowElem->FirstChildElement("specular_map"); if (specularElem) - this->FixPathsInUri(specularElem, _modelVersionedDir); + this->FixPathsInUri(specularElem, _id); tinyxml2::XMLElement *glossinessElem = workflowElem->FirstChildElement("glossiness_map"); if (glossinessElem) - this->FixPathsInUri(glossinessElem, _modelVersionedDir); + this->FixPathsInUri(glossinessElem, _id); } } } @@ -661,7 +684,7 @@ void LocalCachePrivate::FixPathsInMaterialElement( ////////////////////////////////////////////////// void LocalCachePrivate::FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, - const std::string &_modelVersionedDir) + const ModelIdentifier &_id) { if (!_geomElem) return; @@ -673,7 +696,7 @@ void LocalCachePrivate::FixPathsInGeomElement(tinyxml2::XMLElement *_geomElem, { tinyxml2::XMLElement *uriElem = meshElem->FirstChildElement("uri"); // Convert the "model://" URI pattern to file:// - this->FixPathsInUri(uriElem, _modelVersionedDir); + this->FixPathsInUri(uriElem, _id); } } From 99d54dd502f78af28c7e81934e8d1f719ed0387b Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Fri, 17 Jul 2020 18:25:13 -0700 Subject: [PATCH 4/6] remove extra / Signed-off-by: Louise Poubel --- src/LocalCache.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LocalCache.cc b/src/LocalCache.cc index 0e5c3709..cb2b7bc6 100644 --- a/src/LocalCache.cc +++ b/src/LocalCache.cc @@ -597,7 +597,7 @@ void LocalCachePrivate::FixPathsInUri(tinyxml2::XMLElement *_elem, "/models/" + _id.Name() + '/' + _id.VersionStr() + - "/files/" + + "/files" + filePath; _elem->SetText(fuelUrl.c_str()); From 6e9c320c9c811a31800836ac676ae227647d5575 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Wed, 22 Jul 2020 15:43:13 -0700 Subject: [PATCH 5/6] codecheck Signed-off-by: Louise Poubel --- src/LocalCache.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LocalCache.cc b/src/LocalCache.cc index cb2b7bc6..2ec15186 100644 --- a/src/LocalCache.cc +++ b/src/LocalCache.cc @@ -582,8 +582,8 @@ void LocalCachePrivate::FixPathsInUri(tinyxml2::XMLElement *_elem, // owner igndbg << "Model [" << _id.Name() << "] loading resource from another model, named [" << resourceName - << "]. On Blueprint and Citadel, [" << resourceName << "] is ignored. " - << "From Dome, [" << _id.Name() << "] will be used. If [" + << "]. On Blueprint and Citadel, [" << resourceName << "] is " + << "ignored. From Dome, [" << _id.Name() << "] will be used. If [" << resourceName << "] is not a model belonging to owner [" << _id.Owner() << "], fix your SDF file!" << std::endl; } From 05bf7412be60ce94bd612371ebfcaf7a2197ca20 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Mon, 27 Jul 2020 12:01:45 -0700 Subject: [PATCH 6/6] PR feedback Signed-off-by: Louise Poubel --- src/LocalCache.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/LocalCache.cc b/src/LocalCache.cc index 2ec15186..0b5f54d2 100644 --- a/src/LocalCache.cc +++ b/src/LocalCache.cc @@ -582,14 +582,17 @@ void LocalCachePrivate::FixPathsInUri(tinyxml2::XMLElement *_elem, // owner igndbg << "Model [" << _id.Name() << "] loading resource from another model, named [" << resourceName - << "]. On Blueprint and Citadel, [" << resourceName << "] is " - << "ignored. From Dome, [" << _id.Name() << "] will be used. If [" - << resourceName << "] is not a model belonging to owner [" - << _id.Owner() << "], fix your SDF file!" << std::endl; + << "]. On Blueprint (ign-fuel-tools 3) and Citadel " + << "(ign-fuel-tools 4), [" << resourceName << "] is ignored. " + << "From Dome (ign-fuel-tools 5), [" << _id.Name() + << "] will be used. If [" << resourceName + << "] is not a model belonging to owner [" << _id.Owner() + << "], fix your SDF file!" << std::endl; } auto filePath = oldUri.substr(firstSlash); + // Construct a model file URL used to download from the server auto fuelUrl = _id.Server().Url().Str() + '/' + _id.Server().Version() + '/' +