[dartsim] Add empty nested model construction and nested model entity management #228

azeey · 2021-03-17T11:24:53Z

🎉 New feature

Summary

This adds two new features:

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

Requires:

~~[dartsim] Fix joint construction errors due to link name duplication or BodyNodes moving to other skeletons #220~~
~~[dartsim] Ensure Link and Model APIs continue to work after joint creation in DART #227~~

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

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

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>

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

…ties Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

adlarkin

Looks pretty good! I have a few comments, and also asked a few questions to help me get a better grasp of ign-physics (I'm still trying to familiarize myself with the repository).

include/ignition/physics/ConstructEmpty.hh

include/ignition/physics/GetEntities.hh

adlarkin · 2021-03-22T23:55:08Z

include/ignition/physics/GetEntities.hh

@@ -213,6 +213,56 @@ namespace ignition
      };
    };

+    /////////////////////////////////////////////////
+    /// \brief This feature retrieves the nested model pointer from the parent
+    /// model by specifying the model index and model index/name.


Suggested change

/// model by specifying the model index and model index/name.

/// model by specifying the parent model index and nested model index/name.

I found it a little confusing to read "model index" twice - should one be referring to the parent model, and the other to the nested model?

I think this is probably supposed to say

/// \brief This feature retrieves the nested model pointer from the parent /// model by specifying the index or name of the nested model.

From the user's point of view, the parent model does not need to be specified, so I would recommend not mentioning that in the public API documentation, as it may be misleading.

Changed to say "...name or index of the nested model". b836a0a

include/ignition/physics/detail/GetEntities.hh

dartsim/src/Base.hh

dartsim/src/EntityManagementFeatures.cc

adlarkin · 2021-03-23T00:46:25Z

dartsim/src/SDFFeatures.cc

+      return this->AddModel(  // NOLINT
+          {model, _sdfModel.Name(), modelFrame, _sdfModel.CanonicalLinkName()},
+          _parentID);


Suggested change

return this->AddModel( // NOLINT

{model, _sdfModel.Name(), modelFrame, _sdfModel.CanonicalLinkName()},

_parentID);

return this->AddModel( // NOLINT

{model, _sdfModel.Name(), modelFrame, _sdfModel.CanonicalLinkName()},

_parentID, worldID);

I believe the world ID parameter is missing here.

It's not that worldID is missing, but rather _parentID should be replaced with worldID.

After looking at the code further, it does appear to be correct, although I would urge some caution because the type of entity that _parentID refers to is conditional. It may refer to a world or a model depending on whether the model that's being constructed is nested. This may be a strong recipe for some maintenance confusion and a future bug creeping in.

The logic is _parentID and worldID are the same if the model is not nested. But I agree it is confusing. I changed it to worldID in 212c941.

This may be a strong recipe for some maintenance confusion and a future bug creeping in.

I agree. FWIW, this was done to avoid code duplication between ConstructSdfModel and ConstructSdfNestedModel where unlike ConstructEmptyNestedModel, ConstructSdfNestedModel is a feature that can construct a model and it's nested models from sdf::Model. i.e, ConstructSdfNestedModel is the same as ConstructSdfModel but can also handle nested models if they exist in the input sdf::Model.

dartsim/src/EntityManagement_TEST.cc

mxgrey · 2021-03-23T06:35:11Z

include/ignition/physics/GetEntities.hh

+        ///   Index of the model within this model.
+        /// \return A model reference. If _index is GetModelCount() or higher,
+        /// this will be a nullptr.
+        public: ModelPtrType GetModel(std::size_t _index);


I would suggest making this function more explicit by calling it GetNestedModel. Same for the overloads of GetModel below.

Done in d137bba. Also made the same change to ConstructEmptyNestedModel.

…ties

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-03-23T19:23:00Z

I believe I have addressed all the feedback. Please take a look.

azeey added 7 commits March 3, 2021 16:49

Fix error caused when the same link name is used in different models

b54cd35

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Make LoadWorld return WorldPtr

6c59b58

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Handle BodyNodes moving to other skeletons

d7257a4

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Codecheck

bcfc983

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Remove unnecessary functions

baed701

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add empty nested model construction and nested model entity management

353410c

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey requested a review from mxgrey as a code owner March 17, 2021 11:24

github-actions bot added the 🏢 edifice Ignition Edifice label Mar 17, 2021

This was referenced Mar 17, 2021

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

Merged

Remove nested models #230

Merged

Support FreeGroup features for nested models #231

Merged

chapulina added DART DART engine enhancement New feature or request beta Targeting beta release of upcoming collection labels Mar 17, 2021

chapulina assigned azeey Mar 19, 2021

azeey added 5 commits March 19, 2021 15:17

Merge remote-tracking branch 'upstream/main' into keep_track_of_links

8515664

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Address reviewer feedback

0bb7d9e

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Fix typo, add const ref

16da5e1

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge branch 'keep_track_of_links' into dartsim_nested_entities

19b0768

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge remote-tracking branch 'upstream/main' into dartsim_nested_enti…

ab95922

…ties Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

adlarkin self-requested a review March 22, 2021 17:18

adlarkin reviewed Mar 23, 2021

View reviewed changes

mxgrey reviewed Mar 23, 2021

View reviewed changes

azeey added 4 commits March 23, 2021 12:07

Merge remote-tracking branch 'upstream/main' into dartsim_nested_enti…

dca717a

…ties

Address reviewer feedback

b836a0a

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Make nested model API more explicit by using "nested" in function names

d137bba

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Fix bug in nested model construction

212c941

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Add a test for AddNestedModel

a652ef2

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Merge branch 'main' into dartsim_nested_entities

a9cc99c

adlarkin approved these changes Mar 23, 2021

View reviewed changes

azeey merged commit 246909b into gazebosim:main Mar 23, 2021

azeey deleted the dartsim_nested_entities branch March 23, 2021 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dartsim] Add empty nested model construction and nested model entity management #228

[dartsim] Add empty nested model construction and nested model entity management #228

azeey commented Mar 17, 2021 •

edited

Loading

adlarkin left a comment

adlarkin Mar 22, 2021

mxgrey Mar 23, 2021

azeey Mar 23, 2021

adlarkin Mar 23, 2021

mxgrey Mar 23, 2021

mxgrey Mar 23, 2021

azeey Mar 23, 2021

mxgrey Mar 23, 2021

azeey Mar 23, 2021

azeey commented Mar 23, 2021

	/// model by specifying the model index and model index/name.
	/// model by specifying the parent model index and nested model index/name.

[dartsim] Add empty nested model construction and nested model entity management #228

[dartsim] Add empty nested model construction and nested model entity management #228

Conversation

azeey commented Mar 17, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

adlarkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey commented Mar 23, 2021

azeey commented Mar 17, 2021 •

edited

Loading