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

Support <gazebo_ros> in package.xml exports #492

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Feb 2, 2024

🎉 New feature

Summary

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.

This will make transition from Gazebo Classic easier.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

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]>
Copy link
Collaborator

@ahcorde ahcorde left a 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?

@azeey
Copy link
Contributor Author

azeey commented Feb 5, 2024

it might be worth it to add a tutorial about this or small doc about this.

Good point!

Do you have plans to forward port this to Iron and Rolling?

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 humble branch got the IGN_* variables modified as well.

Copy link
Contributor

@j-rivero j-rivero left a 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)
Copy link
Contributor

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:

  1. The xml_path in a vast majority of the cases needs to be an existing non empty directory.
  2. For the gazebo_model_path, we can probably scan for the expected subdirectory structure with at least one .sdf and one .config.
  3. For gazebo_plugin_path we can probably expect that the user hosts at least one .so or one binary file.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

ros_gz_sim/launch/gz_sim.launch.py.in Outdated Show resolved Hide resolved
@DLu
Copy link
Contributor

DLu commented Feb 26, 2024

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.

@azeey
Copy link
Contributor Author

azeey commented Feb 28, 2024

@ahcorde , @DLu I've added a section to the README that explains how to use this. Let me know your thoughts. This could be expanded into a full tutorial on gazebosim.org/docs.

@DLu
Copy link
Contributor

DLu commented Mar 1, 2024

Tip the hat

@azeey azeey merged commit 13e5592 into gazebosim:humble Mar 11, 2024
9 checks passed
@azeey azeey deleted the gazebo_ros_export branch March 11, 2024 17:10
@ahcorde
Copy link
Collaborator

ahcorde commented Mar 21, 2024

@azeey do you have any plan to forward port this to iron and rolling?

@rakeshv24
Copy link

@azeey is this feature included in the jazzy release yet?

@azeey
Copy link
Contributor Author

azeey commented Jun 21, 2024

Ah, this fell through the cracks. I've started the forward port process: #564

@rakeshv24
Copy link

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants