Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert model:// to Fuel URLs instead of absolute paths #85

Merged
merged 8 commits into from
Jul 29, 2020

Conversation

chapulina
Copy link
Contributor

Builds on top of #84 .

Resolves #77.

Summary

Without this PR, model:// URIs like this:

<uri>model://Radio/meshes/Radio.dae</uri>

Would become absolute paths like this:

<uri>/home/chapulina/.ignition/fuel/fuel.ignitionrobotics.org/OpenRobotics/models/Radio/4/meshes/Radio.dae</uri>

With this PR, it becomes a Fuel URL like this:

<uri>https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Radio/4/files/meshes/Radio.dae</uri>

Resources across models

It's common for one model to use resources from other models to avoid duplication of resources. For example, the Radio model could use model://Phone/meshes/antenna,dae from the Phone model (assuming both.

For this to work on Blueprint and Citadel, Radio will need to use the full URL of the phone's antenna, i.e.

https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/Phone/4/files/meshes/antenna.dae.

From Dome onwards, the same owner will be assumed for both models, and

model://Phone/meshes/antenna,dae

will be converted to the URL above automatically.

The reason not to do this for Blueprint is that there's a non-trivial number of models on Fuel right now whose Fuel name doesn't match the model:// URI. These would look like different models which don't exist. For example, the Construction Cone uses model://construction_cone.

Right now, these models load fine, but a debug message is written:

[Dbg] [LocalCache.cc:583] Model [Construction Cone] loading resource from another model, named [construction_cone]. On Blueprint and Citadel, [construction_cone] is ignored. From Dome, [Construction Cone] will be used. If [construction_cone] is not a model belonging to owner [OpenRobotics], fix your SDF file!

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Base automatically changed from chapulina/3/fetch_files to ign-fuel-tools3 July 22, 2020 22:32
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #85 into ign-fuel-tools3 will increase coverage by 0.04%.
The diff coverage is 78.78%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           ign-fuel-tools3      #85      +/-   ##
===================================================
+ Coverage            85.70%   85.74%   +0.04%     
===================================================
  Files                   16       16              
  Lines                 2028     2041      +13     
===================================================
+ Hits                  1738     1750      +12     
- Misses                 290      291       +1     
Impacted Files Coverage Δ
src/LocalCache.cc 79.51% <78.78%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56535a2...05bf741. Read the comment docs.

@chapulina chapulina added the migration Helps with migration from Gazebo classic to Ignition label Jul 24, 2020
Copy link
Contributor

@nkoenig nkoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add a test, or save that for later?

src/LocalCache.cc Outdated Show resolved Hide resolved
src/LocalCache.cc Show resolved Hide resolved
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Do you want to add a test, or save that for later?

The tests in src/FuelClient_TEST.cc were updated to reflect the changes in this PR. They check that resource tags now start with https://. I didn't want to check the entire URL in case the model changes.

@nkoenig
Copy link
Contributor

nkoenig commented Jul 29, 2020

I'm seeing a lot of errors with models in Subt. For example:

[Err] [Ogre2MeshFactory.cc:430] Cannot load null mesh [https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/X1 Config 1/15/files/meshes/wheel.dae]

@chapulina
Copy link
Contributor Author

I'm seeing a lot of errors with models in Subt. For example:

Those work for me. Are you running the latest commit of ign-gazebo2?

@nkoenig
Copy link
Contributor

nkoenig commented Jul 29, 2020

I'm seeing a lot of errors with models in Subt. For example:

Those work for me. Are you running the latest commit of ign-gazebo2?

That fixed the problem, however this change breaks gz3d. I'll have to fixed that library first.

@chapulina
Copy link
Contributor Author

this change breaks gz3d. I'll have to fixed that library first.

Oh I hadn't thought of that. Maybe it's safer to target this at Dome to preserve backwards compatibility. Let me know how gz3d goes.

@nkoenig
Copy link
Contributor

nkoenig commented Jul 29, 2020

this change breaks gz3d. I'll have to fixed that library first.

Oh I hadn't thought of that. Maybe it's safer to target this at Dome to preserve backwards compatibility. Let me know how gz3d goes.

Here is the patch. I'll approve this PR so that we can eventually make new docker images for SubT.

https://gitlab.com/ignitionrobotics/web/gz3d/-/merge_requests/26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint migration Helps with migration from Gazebo classic to Ignition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants