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

[tpe] Add empty nested model construction and nested model entity management #229

Merged
merged 30 commits into from
Mar 24, 2021

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Mar 17, 2021

🎉 New feature

This is the same as #228, but for TPE.

Summary

This adds two new features:

  1. ConstructEmptyNestedModelFeature, which allows users to create a nested model without having to use ConstructSdfNestedModel
  2. GetNestedModelFromModel, which allow retrieving a nested model from it's parent Model entity.

Requires:

Test it

Run tests

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

azeey added 8 commits March 3, 2021 16:49
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
We do this by keeping track of links separately from DART so that APIs
such as `Model::GetLink()` and `Link::GetIndex` are not affected by
BodyNode's moving from one skeleton to another when a joint is created
between different (nested) models.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested a review from mxgrey as a code owner March 17, 2021 11:26
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 17, 2021
@chapulina chapulina added enhancement New feature or request TPE Trivial Physics Engine beta Targeting beta release of upcoming collection labels Mar 17, 2021
mxgrey
mxgrey previously requested changes Mar 23, 2021
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

The implementation of the feature for dartsim looks fine, but I would urge us to change the implementation for TPE. If we split the children container into separate links and nestedModels containers, then we can replace some dynamic_casting with static_cast and eliminate O(N) complexity counts for finding the element counts.

tpe/lib/src/Model.cc Outdated Show resolved Hide resolved
tpe/plugin/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
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.

Looks great after addressing the comments from @mxgrey. I just have a few things to add before merging.

tpe/lib/src/Model.cc Outdated Show resolved Hide resolved
tpe/lib/src/Model.cc Outdated Show resolved Hide resolved
tpe/plugin/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
tpe/plugin/src/EntityManagement_TEST.cc Show resolved Hide resolved
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.

Everything looks good to me after the patch that was brought in via e5679df. The updated indexInContainerToId method definitely makes things easier in EntityManagementFeatures. I just left one question I had, as I am trying to still wrapping my mind around some of these physics concepts.

tpe/plugin/src/Base.hh Show resolved Hide resolved
@azeey azeey dismissed mxgrey’s stale review March 24, 2021 14:43

I have addressed all the feedback. In the interest of time, I'll dismiss this review so we can merge this. Please feel to give it another look and I'll address any addition feedback in a follow-up PR.

@azeey azeey merged commit b10a4c3 into gazebosim:main Mar 24, 2021
@azeey azeey deleted the tpe_nested_entities branch March 24, 2021 14:44
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 TPE Trivial Physics Engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants