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

Migration to sdf::Plugin broke plugins relying on Parent information #1561

Closed
luca-della-vedova opened this issue Jun 29, 2022 · 2 comments
Closed
Assignees
Labels
bug Something isn't working 🏯 fortress Ignition Fortress

Comments

@luca-della-vedova
Copy link
Member

luca-della-vedova commented Jun 29, 2022

Environment

  • OS Version: 20.04
  • Source or binary build? Either, behavior change was bisected to Use sdf plugin #1352

One of those classic XKCDs https://xkcd.com/1172/

Description

We have been relying on reading the ElementPtr that is passed to Gazebo plugins on initialization to traverse the hierarchy up and read properties of the model that the plugin was attached to.
For example, when simulating doors, we read joint limits of the Joint that the plugin was attached to to read the limits and smoothly accelerate / decelerate.
https://github.com/open-rmf/rmf_simulation/blob/main/rmf_building_sim_common/include/rmf_building_sim_common/door_common.hpp#L228-L251

This has been a reasonably stable usage shared between Gazebo classic and gz-sim, however since the linked PR this is not possible anymore and the code above fails when fetching the parent.
My guess is that this is due to the Plugin element in SDF not having an API to access its parent, hence when offering "backwards compatible behavior" the information is just not there.
I'm not sure this is a workflow that we want to support. I guess a possible fix could be to add an API to sdf::Plugin to have access to its parent so plugins can read information from elsewhere in the world if they need to.
Another alternative could be to just populate the parent at the gz-sim level when offering the backward compatible behavior, however this particular workflow would still fail once the migration to sdf::Plugin is completed and there is no more sdf::ElemetPtr API.
If that is not a clean fix I'm open to alternative on how we can get the same behavior with the new sdf::Plugin data structure.

If the fix of adding the API to sdf::Plugin sounds reasonable I'm happy to take a stab at implementing it!

Steps to reproduce

  1. Try to access the parent from a plugin's sdf::ElementPtr before the PR.
  2. Try the same after the PR
  3. Notice that it doesn't work.
@luca-della-vedova luca-della-vedova added the bug Something isn't working label Jun 29, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Jun 29, 2022
@chapulina
Copy link
Contributor

chapulina commented Jun 29, 2022

Ouch, sorry for the regression, this is not a way we usually use the plugins, so we didn't think to test it. In general, you should be getting properties for entities from the ECM, not from SDF elements. The SDF element's parent may not always be there (i.e. if the plugin was added programmatically), and it may have outdated data.

But this is not a change we should make in a stable release, so I think we should revert just the offending lines from #1352 and target them to Garden, with a note in the migration guide.

Thoughts, @nkoenig ?

@luca-della-vedova
Copy link
Member Author

Closing since it's probably not worth it to put too much effort into this somewhat obscure case, we went with the recommended approach of using the ECM and it works well!

Core development automation moved this from To do to Done Oct 3, 2022
@azeey azeey closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress
Projects
Archived in project
Development

No branches or pull requests

4 participants