-
Notifications
You must be signed in to change notification settings - Fork 91
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
Revert behavior of FilePath() for elements loaded from strings #582
Conversation
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
There were two test failures: 1. When a nested model is parsed, it's file path is not properly propagated from the parent element. In fact, the nested model is removed from its parent via `RemoveFromParent()`. This is probably a bug in itself which requires further investigation. For now, explicitly setting the `FilePath()` is necessary 2. Plugin elements were being treated differently than other override elements such as `name` and `pose` when `SetFilePath()` was called on them instead of inheriting the file path from their parents. Updating the test expectations fixed the failure. Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
FYI @aaronchongth from d3798f6 There were two test failures:
|
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.
Thanks for reverting the behavior and highlighting these bugs!
The fix for (1) makes sense, thanks!
For (2), I chose to set the file path of the plugin element explicitly to the file where it was defined, instead of following what was done for name
and pose
, as it seems more intuitive for debugging. It might be slightly confusing if the users head over to actorFIlePath
but the plugin element was missing. Names and poses are overriding instead of additive, so inheriting the file path from the included file makes sense. 🤔 Let me know what you think.
From the debugging perspective I think it is useful to have |
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.
Thanks for explaining. In that case, having the same behavior for all overriding elements makes sense. Thanks for the fixes!
It looks like the
Should we ticket an issue for this? |
🦟 Bug fix
Summary
#548 introduced a behavior change on
sdf::Element::FilePath()
for elements loaded from strings. Before that PR,FilePath()
returned""
for such elements, but presently, it returns eitherdata-string
or<data-string>
. This behavior change could cause failures in downstream applications once released (I know the testINTEGRATION_save_world
in ign-gazebo will have test failures).4876f92 adds failing test showing the behavior change.
Checklist
sh tools/code_check.sh
)test coverage)
another open pull request
to support the maintainers
Note to maintainers: Remember to use Squash-Merge