-
Notifications
You must be signed in to change notification settings - Fork 138
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
Support <gazebo_ros>
in package.xml
exports
#492
Conversation
This copies the implementation from `gazebo_ros_paths.py` to provide a way for packages to set resource paths from `package.xml`. ``` e.g. <export> <gazebo_ros gazebo_model_path="${prefix}/models"/> <gazebo_ros gazebo_media_path="${prefix}/models"/> </export> ``` The value of `gazebo_model_path` and `gazebo_media_path` is appended to `GZ_SIM_RESOURCE_PATH` The value of `plugin_path` appended to `GZ_SIM_SYSTEM_PLUGIN_PATH` Signed-off-by: Addisu Z. Taddese <[email protected]>
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.
it might be worth it to add a tutorial about this or small doc about this.
Do you have plans to forward port this to Iron and Rolling?
Good point!
Yes, I'll cherry-pick this to Iron and Rolling. I was going to start on Rolling first, but I wanted to make sure the |
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.
Just commenting here what we spoke in the weekly meeting: let's try to create a tutorial that documents all the options of injecting the paths available and point from this code to the user to specify that this is not the easiest way or the proffered way with a pointer to the tutorial.
if 'gazebo_model_path' in export.attributes: | ||
xml_path = export.attributes['gazebo_model_path'] | ||
xml_path = xml_path.replace('${prefix}', package_share_path) | ||
gazebo_model_path.append(xml_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.
I think that one of the most frustrating issue for newcomers and people dealing with Gazebo + ROS integration is to badly configure the asset directories assuming random paths that are not the ones really processed here. I would like to use the PR to improve this situation.
How about adding a bit of sanity checks before the .append(
calls in this file that can emit warning/errors for the user to check/fix, some ideas:
- The
xml_path
in a vast majority of the cases needs to be an existing non empty directory. - For the
gazebo_model_path
, we can probably scan for the expected subdirectory structure with at least one .sdf and one .config. - For
gazebo_plugin_path
we can probably expect that the user hosts at least one .so or one binary file.
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.
I looked at doing this but the way this works, it's actually looking at all the packages in the system, not just the individual package the user might be interested in. And would have to do the checks for all packages every time this file is launched. Aside from the performance implications, a user launching this file would see errors/warnings coming from other packages they have no control over.
An alternative is to create a tool that can be used to search for a particular model/resource by it's URI. For example, if I wanted to check that the URI model://my_awesome_pkg/models/cool_robot
is valid, I would run this tool and it would search all exported model paths and give me some diagnostic messages if the model couldn't be found.
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.
@j-rivero any thoughts? I'd like to unblock ros/urdf_sim_tutorial#16 soon if possible.
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.
I looked at doing this but the way this works, it's actually looking at all the packages in the system, not just the individual package the user might be interested in.
I was thinking on limiting the directories to inspect to the ones added via:
<export>
<gazebo_ros gazebo_model_path="${prefix}/models"/>
<gazebo_ros gazebo_media_path="${prefix}/media"/>
<gazebo_ros plugin_path="${prefix}/plugins"/>
</export>
Why do we need to check all the packages in the system?
And would have to do the checks for all packages every time this file is launched. Aside from the performance implications,
I agree that there is a performance implication that can be bad.
An alternative is to create a tool that can be used to search for a particular model/resource by it's URI. For example, if I wanted to check that the URI model://my_awesome_pkg/models/cool_robot is valid, I would run this tool and it would search all exported model paths and give me some diagnostic messages if the model couldn't be found.
That would be really useful indeed. Another approach would be to enable a launch argument or environment flag that enable this debug behavior.
Given the situation, I'm not blocking the PR, thought it was easier to add the checks.
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.
The code for package_name in get_package_names():
iterates through all the packages available. Then we'd need to check if any package that exports gazebo_ros
meets the sanity requirements you described.
Are we waiting on the Tutorial to get this merged? I have a URDF/Gazebo tutorial waiting to be merged and there is a section ("Side note: Configuring meshes") that covers this area. The tutorial is written for for Gazebo Classique, but as linked above, I'm trying to get the Gazebo New Coke version to work too. |
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey do you have any plan to forward port this to iron and rolling? |
@azeey is this feature included in the jazzy release yet? |
Ah, this fell through the cracks. I've started the forward port process: #564 |
Thanks! |
🎉 New feature
Summary
This copies the implementation from
gazebo_ros_paths.py
to provide a way for packages to set resource paths frompackage.xml
.The value of
gazebo_model_path
andgazebo_media_path
is appended toGZ_SIM_RESOURCE_PATH
The value ofplugin_path
appended toGZ_SIM_SYSTEM_PLUGIN_PATH
.This will make transition from Gazebo Classic easier.
Test it
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.