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 individual canonical links for nested models #685

Merged
merged 7 commits into from
Mar 24, 2021

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Mar 16, 2021

🎉 New feature

Summary

When #258 was added, we discussed a limitation that prevented the poses of nested models (just the model frame) from being updated and left a TODO in the code. This was due to the CanonicalLink being a component attached to links and not models. For example, in the following example, nested_model1 and nested_model2 have nested_link1 and nested_link2 as their canonical links respectively. The canonical link of top_level_model is nested_link1. However, since the CanonicalLink component on nested_link1 doesn't have enough information to indicate that it is the canonical link of top_level_model, the physics system has no way of knowing which canonical link to use to update top_level_model. Therefore, in #258, we opted for creating only one canonical link in a model hierarchy. Thus, for this example, only nested_link1 would have the CanonicalLink component.

<model name="top_level_model">
  <model name="nested_model1">
      <link name="nested_link1"/>
  </model>
  <model name="nested_model2">
      <link name="nested_link2"/>
  </model>
 
</model> 

And since only one canonical link is created, the poses of other nested models would not get updated.

This PR creates a new component ModelCanonicalLink that is attached to models and contains a reference to the link entity that serves as the canonical link of the model. This allows us to update the poses of all models including nested ones. In addition, by updating the model poses first, it addresses the issue discussed here where we have previously assumed that the canonical link's pose update will be handled before the rest of the links in the model since the it is the first link in the model. However, in SDFormat 1.7 and later, the canonical link can be manually specified such that it's not the first link in the model.

Finally, this also addresses the comment in #479

Bounding box of child model doesn't follow child model. It is created initially with the appropriate size and position, but follows the root model's canonical link.

Peek.2021-03-16.01-43.mp4

Test it

TODO

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey added the 🏢 edifice Ignition Edifice label Mar 16, 2021
@azeey azeey requested a review from chapulina as a code owner March 16, 2021 06:46
@azeey azeey self-assigned this Mar 16, 2021
@chapulina chapulina added enhancement New feature or request beta Targeting beta release of upcoming collection labels Mar 16, 2021
@adlarkin adlarkin self-requested a review March 19, 2021 20:12
@chapulina
Copy link
Contributor

Conflicts need to be fixed

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor Author

azeey commented Mar 22, 2021

Conflicts need to be fixed

Done!

Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

A few minor comments. I haven't looked over the changes in physics.cc too closely yet since we discussed some modifications that need to be made offline, but once those changes are in, I'll give it a more thorough review.

include/ignition/gazebo/SdfEntityCreator.hh Outdated Show resolved Hide resolved
src/SdfEntityCreator.cc Outdated Show resolved Hide resolved
src/SdfEntityCreator.cc Show resolved Hide resolved
test/integration/physics_system.cc Show resolved Hide resolved
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! The comments in Physics.cc about frame semantics/computations are also very helpful. I left a few small comments/questions, but other than that, I think this is ready to go 🚢

test/integration/physics_system.cc Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Show resolved Hide resolved
src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
azeey and others added 2 commits March 23, 2021 22:53
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor Author

azeey commented Mar 24, 2021

Windows CI is backed up. The previous job (https://build.osrfoundation.org/job/ign_gazebo-pr-win/2399/) built fine, so I'll go ahead and merge this.

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 🏢 edifice Ignition Edifice enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants