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.

Sign up for GitHub

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

Jump to bottom

Remove nested models #230

Merged
merged 44 commits into from
Mar 25, 2021
Merged

Remove nested models #230

merged 44 commits into from
Mar 25, 2021

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Mar 17, 2021 •
edited
Loading

🎉 New feature

Summary

This adds a new feature, RemoveNestedModelFromModel, that allow users to remove a nested model via it's parent model entity. This PR also ensures that when a parent model is removed, all its nested models are removed as well

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 10 commits March 3, 2021 16:49
@azeey
Fix error caused when the same link name is used in different models
b54cd35 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Make LoadWorld return WorldPtr
6c59b58 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Handle BodyNodes moving to other skeletons
d7257a4 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Codecheck
bcfc983 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Remove unnecessary functions
baed701 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Ensure Link and Model APIs continue to work after joint creation
372cd27 
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 <addisu@openrobotics.org>
@azeey
Add empty nested model construction and nested model entity management
353410c 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Add empty nested model construction and nested model entity managemen…
96f46c8 
…t in TPE

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Remove nested models in TPE
f9af478 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Remove nested models in dartsim
6bb3c6c 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey requested a review from mxgrey as a code owner March 17, 2021 11:31
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Mar 17, 2021
@azeey azeey mentioned this pull request Mar 17, 2021
Merged
8 tasks
@chapulina chapulina added DART DART engine enhancement New feature or request TPE Trivial Physics Engine beta Targeting beta release of upcoming collection labels Mar 17, 2021
@azeey azeey mentioned this pull request Mar 19, 2021
Merged
7 tasks
azeey added 6 commits March 19, 2021 15:17
@azeey
Merge remote-tracking branch 'upstream/main' into keep_track_of_links
8515664 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Address reviewer feedback
0bb7d9e 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Fix typo, add const ref
16da5e1 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge branch 'keep_track_of_links' into dartsim_nested_entities
19b0768 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge remote-tracking branch 'upstream/main' into dartsim_nested_enti…
ab95922 
…ties

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge branch 'dartsim_nested_entities' into tpe_nested_entities
6e25256
mxgrey
mxgrey reviewed Mar 23, 2021
View reviewed changes
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.

This PR looks good, although I do have some nitpicks that I think will help with code clarity.

dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated Show resolved Hide resolved
tpe/plugin/src/Base.hh Outdated Show resolved Hide resolved
include/ignition/physics/GetEntities.hh Outdated Show resolved Hide resolved
azeey added 2 commits March 23, 2021 12:07
@azeey
Merge remote-tracking branch 'upstream/main' into dartsim_nested_enti…
dca717a 
…ties
@azeey
Address reviewer feedback
b836a0a 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
azeey added 7 commits March 23, 2021 16:07
@azeey
Merge branch 'dartsim_nested_entities' into tpe_nested_entities
3f84880
@azeey
Fix function names after merge
f0a8492 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Address reviewer feedback
e17aed3 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge remote-tracking branch 'upstream/main' into tpe_nested_entities
11407c5
@azeey
Merge branch 'tpe_nested_entities' into remove_nested_models
ca61e24 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Fix function renames after merge
0b5e9b6 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Fix test failures due to separate bookkeeping of links and nested mod…
7012c67 
…els in tpelib::Model

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey mentioned this pull request Mar 23, 2021
Merged
8 tasks
azeey and others added 8 commits March 23, 2021 20:27
@azeey
Fix entity by index lookup, address reviewer feedback
e5679df 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge branch 'tpe_nested_entities' into remove_nested_models
adccd2a 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Fix windows warning
31474ee 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@adlarkin
Merge branch 'main' into tpe_nested_entities
2254dc5
@azeey
Merge branch 'tpe_nested_entities' into remove_nested_models
ae53c57
@azeey
Reviewer feedback
62abc8b 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge remote-tracking branch 'upstream/main' into remove_nested_models
641d342 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Remove unnecessary header
27ba015 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@chapulina chapulina self-requested a review March 24, 2021 20:15
chapulina
chapulina approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me, tests look good, and nested models seemed to be deleted cleanly on ign-gazebo for both DART and TPE.

I just have minor comments for your consideration

dartsim/src/Base.hh Outdated Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc Outdated
const DartSkeletonPtr &model =
this->ReferenceInterface<DartWorld>(_worldID)->getSkeleton(_modelIndex);
const std::size_t modelID =
this->models.indexInContainerToID.at(_worldID)[_modelIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do some checking to see if the index exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in ea42d0c

tpe/plugin/src/EntityManagementFeatures.cc Show resolved Hide resolved
tpe/plugin/src/EntityManagementFeatures.cc Show resolved Hide resolved
include/ignition/physics/RemoveEntities.hh Outdated Show resolved Hide resolved
tpe/lib/src/Model.cc Show resolved Hide resolved
tpe/lib/src/Model.hh Show resolved Hide resolved
dartsim/src/EntityManagementFeatures.cc
const std::size_t nestedModelID = this->models.IdentityOf(nestedSkel);
const auto filterPtr = GetFilterPtr(this, worldID);
filterPtr->RemoveSkeletonCollisions(nestedSkel);
return this->RemoveModelImpl(worldID, nestedModelID);
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here is pretty similar to RemoveNestedModelByIndex, I think this function could call that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it would be trivial to be able to call RemoveNestedModelByIndex because we have to find the index based on the name first. Ideally the lines

    const auto filterPtr = GetFilterPtr(this, worldID);
    filterPtr->RemoveSkeletonCollisions(nestedSkel);

could go into RemoveModelImpl, but that would involve moving GetFilterPtr into Base.hh. How about we revisit this later? I can leave a TODO for myself in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah later is fine, it was just a small suggestion

azeey added 4 commits March 24, 2021 19:41
@azeey
Merge remote-tracking branch 'upstream/main' into remove_nested_models
ef11291
@azeey
Rename RemoveModel to RemoveNestedModel in the RemoveNestedModelFromM…
b3b9ae7 
…odel feature

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Small fixes addressing reviewer feedback
ea42d0c 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Refactor entity removal code
27daecb 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor Author

azeey commented Mar 25, 2021

In b3b9ae7, I renamed e RemoveModel to RemoveNestedModel in the RemoveNestedModelFromModel feature.

I also realized the functions tpelib::Model::RemoveModelById and tpelib::Model::RemoveLinkById were internal implementation details, so I made them private.

@azeey
Add a TODO about de-duplicating model removal code in dartsim
5c87a07 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey enabled auto-merge (squash) March 25, 2021 02:40
@azeey azeey merged commit f9641a5 into gazebosim:main Mar 25, 2021
@diegoferigo diegoferigo mentioned this pull request Jun 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxgrey mxgrey mxgrey left review comments

@chapulina chapulina chapulina approved these changes

Assignees

@azeey azeey

Labels
beta Targeting beta release of upcoming collection DART DART engine 🏢 edifice Ignition Edifice enhancement New feature or request TPE Trivial Physics Engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@azeey @mxgrey @chapulina @adlarkin