-
Notifications
You must be signed in to change notification settings - Fork 8
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
Handle model:// path in URI #20
Conversation
@oKermorgant Thanks for your contribution! Do you mind refactoring the header a bit based on the suggestions by the workflow run, like one here, it's mostly reformatting, missing spaces, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's efficient. Added suggestions based on CI and some improvements to make it more readable and preferring C++ methods.
sdformat_urdf/src/sdformat_urdf.cpp
Outdated
@@ -32,6 +32,7 @@ | |||
#include <sdf/Visual.hh> | |||
|
|||
#include "sdformat_urdf/sdformat_urdf.hpp" | |||
#include "uri_resolve.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be renamed to resolve_model_uri
for clarity?
sdformat_urdf/src/uri_resolve.hpp
Outdated
char* path = strtok_r(paths, ":", save_ptr); | ||
while(path) | ||
{ | ||
fs::path root(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could root
be renamed to something more descriptive like modelDirectory
?
sdformat_urdf/src/uri_resolve.hpp
Outdated
continue; | ||
|
||
char** save_ptr{}; | ||
char* path = strtok_r(paths, ":", save_ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be defined as const
?
@oKermorgant regarding the environment variables, Gazebo will look for URIs (path / URL) in the following order:
The SDF_PATH environment variable is not recommended. More details here: https://gazebosim.org/api/sim/7/resources.html |
Co-authored-by: Dharini Dutia <[email protected]>
Co-authored-by: Dharini Dutia <[email protected]>
Co-authored-by: Dharini Dutia <[email protected]>
Co-authored-by: Dharini Dutia <[email protected]>
This is an interesting one. On the ROS side
I think my favorite solution would be one or both of these instead of resolving the URI here:
|
Hi, Indeed the whole thing is about ROS and Gazebo coupling through having to parse possibly the same URDF and SDF files. To my experience, URIs starting with I came into this issue when playing with pure Gazebo models I wanted to run a |
Hi, I am not familiar enough with libsdformat and ros_gz plugins to deal with the suggestions, so feel free to tackle this one. For my use cases resolving URI based on Gazebo environment variables is fine, though dealing with nested models would be a bit better. |
The problem with (1) is that we have a chicken and egg problem. Gazebo plugins are currently read from SDF files, which requires loading the model or world SDF that also contains the paths we need to resolve. The plugin would not have a chance to configure libsdformat to know about Another solution might be to add the
We can add We would provide a launch helper python module to do this. gazebosim/ros_gz#492 is doing something similar on a more global scale to replicate what we had in Gazebo Classic. Maybe adding all available packages instead of just dependencies to the path might work too. |
@azeey the approach you shared worked like a charm! ✨ This commit could serve as a useful reference for others. Further, I've opened #26 to add information from your comment to the README. I guess we could close this PR and my ticket with that? |
@Yadunund Glad it worked! I've also created gazebosim/ros_gz#497 that would eventually allow us to do (1) from #20 (comment) |
Hi, it looks like the proposed solution is incomplete. Apparently, path resolution happens twice when rviz loads model description provided by robot_state_publisher:
The first step does not seem to work with either It is possible to workaround this issue by launching rviz in |
@asherikov could you share a link to a minimal repo that allows one to reproduce the issue? |
Hi, it may take some time, rough idea:
rviz would spit errors to the console, observed with ROS2 Humble, havent tried anything else. |
Hi,
This PR is to convert URI's starting with
model://
to absolute paths, by using classical Gazebo environment variables.While it does not answer all cases (http link / relative path), many models just rely on
model://
meshes which are resolved here.I am not sure on the order the environment variables should be processed (see here).