Skip to content

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Jul 8, 2024

🦟 Bug fix

Part of #632, alternative to #1377

Summary

There is some overlap in the names used for elements in the //world/state specification and elsewhere, for example the model_state.sdf file included in state.sdf defines an element named model. This has caused challenges with aliasing in XSD (see #632 and #643), and it would be simpler if some of these elements had different names, so a _state suffix is appended to several subelements of the //state element. Additionally, the //joint_state definition is moved to its own file and included from state.sdf so that a //state/joint_state element can specify the state of a //world/joint.

I made a similar attempt at renaming these state elements in #1377 trying also to support backwards compatibility with a 1_11.convert file but only achieving partial functionality. Given that the //state element hasn't been used in any version of gz-sim, this pull request opts for a breaking change instead of partial support for backwards compatibility.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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.

scpeters added 3 commits July 7, 2024 20:37
Include joint_state.sdf from model_state.sdf
and state.sdf for //world/joint states.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters requested a review from azeey as a code owner July 8, 2024 06:27
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jul 8, 2024
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey azeey requested a review from jennuine August 6, 2024 21:12
Copy link
Collaborator

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, in each of the *_state.sdf can we add a little more description on what state is? For example, what is joint_state vs. normal joint. Alternatively, adding a more thorough comment in state.sdf would be helpful.

Also, since this is a breaking change, shouldn't we add a method of conversion in https://github.com/gazebosim/sdformat/blob/sdformat14_14.0.0/sdf/1.11/1_10.convert ?

@scpeters
Copy link
Member Author

LGTM, in each of the *_state.sdf can we add a little more description on what state is? For example, what is joint_state vs. normal joint. Alternatively, adding a more thorough comment in state.sdf would be helpful.

👍

Also, since this is a breaking change, shouldn't we add a method of conversion in https://github.com/gazebosim/sdformat/blob/sdformat14_14.0.0/sdf/1.11/1_10.convert ?

I tried to add that in #1377, but it was difficult to handle multiple levels of nested models. My last commit in that PR (e9de659) added a failing test showing what wasn't yet working (see jenkins job result). The //state elements were used in gazebo-classic log files but haven't been used with gz-sim, and enabling automatic conversion would have required substantial updates to the sdf::Converter class, so I'm proposing a breaking change here. I believe @azeey concurred during a weekly meeting

@scpeters
Copy link
Member Author

LGTM, in each of the *_state.sdf can we add a little more description on what state is? For example, what is joint_state vs. normal joint. Alternatively, adding a more thorough comment in state.sdf would be helpful.

👍

I've updated the description of state elements in cd32f6e

@scpeters
Copy link
Member Author

while updating state descriptions (see cd32f6e), I decided to allow joints in //state/insertions for consistency with //state/joint_state in 33d6634

@azeey
Copy link
Collaborator

azeey commented Aug 14, 2024

LGTM, in each of the *_state.sdf can we add a little more description on what state is? For example, what is joint_state vs. normal joint. Alternatively, adding a more thorough comment in state.sdf would be helpful.

👍

Also, since this is a breaking change, shouldn't we add a method of conversion in https://github.com/gazebosim/sdformat/blob/sdformat14_14.0.0/sdf/1.11/1_10.convert ?

I tried to add that in #1377, but it was difficult to handle multiple levels of nested models. My last commit in that PR (e9de659) added a failing test showing what wasn't yet working (see jenkins job result). The //state elements were used in gazebo-classic log files but haven't been used with gz-sim, and enabling automatic conversion would have required substantial updates to the sdf::Converter class, so I'm proposing a breaking change here. I believe @azeey concurred during a weekly meeting

Yes, like @scpeters said, gz-sim hasn't been using the //state element, so not having an automatic conversion is not a blocker.

@scpeters scpeters merged commit c8c4a6f into main Aug 18, 2024
@scpeters scpeters deleted the scpeters/state_1_12_suffix branch August 18, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants