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 implicit nested canonical links #341

Merged
merged 5 commits into from
Sep 2, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Aug 20, 2020

This is a follow-up to #283 to support some models without direct child links if they contain a nested model with nested links like the following:

<model name="top">
  <model name="nested">
    <link name="link"/>
  </model>
</model>

Nested canonical links can be used implicitly, but not explicitly since the :: syntax for referencing across model boundaries will not be supported until SDFormat 1.8.

The FrameAttachedToGraph does need to store the relative name of a nested canonical link using :: syntax, so a private helper function Model::CanonicalLinkAndRelativeName is added that computes this relative name. This helper function is called by Model::CanonicalLink to avoid code duplication.


This change is Reviewable

Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose first link of the first nested model as canonical
link instead.
Repeat this logic for the `Model::CanonicalLink` method.

Nested links cannot be specified explicitly with :: syntax
in `//model/@canonical_link` in SDFormat 1.7.
This will be supported in SDFormat 1.8.

Signed-off-by: Steve Peters <[email protected]>
This is a new private function that de-duplicates the
code that was in both FrameSemantics.cc and Model::CanonicalLink.
It provides a Link* pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph.
This function is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.
A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a FrameAttachedToGraph&
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2020

Codecov Report

Merging #341 into sdf9 will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             sdf9     #341      +/-   ##
==========================================
+ Coverage   86.22%   86.25%   +0.03%     
==========================================
  Files          59       59              
  Lines        9148     9170      +22     
==========================================
+ Hits         7888     7910      +22     
  Misses       1260     1260              
Impacted Files Coverage Δ
src/FrameSemantics.cc 76.54% <100.00%> (+0.31%) ⬆️
src/Model.cc 83.00% <100.00%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d800d1c...821affe. Read the comment docs.

@EricCousineau-TRI
Copy link
Collaborator

Should resolve #342

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

:lgtm: with some minor comments

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @scpeters)


test/integration/model_dom.cc, line 349 at r1 (raw file):

  ASSERT_NE(nullptr, model->CanonicalLink());
  // this reports the local name, not the nested name "nested::link2"
  EXPECT_EQ("link2", model->CanonicalLink()->Name());

Is there a way to assert that this link belongs to "nested"?


test/integration/model_dom.cc, line 364 at r1 (raw file):

  EXPECT_EQ(2u, model->ModelCount());
  EXPECT_NE(nullptr, model->ModelByIndex(0));
  EXPECT_NE(nullptr, model->ModelByIndex(1));

nit Consider making this more explicit, and more explicitly check parity with ModelByName?

EXPECT_EQ(model->ModelByName("nested"), model->ModelByIndex(0));
EXPECT_EQ(model->ModelByName("shallow"), model->ModelByIndex(1));
EXPECT_EQ(nullptr, model->ModelByIndex(2));

* Expect CanonicalLink() == ModelByName("nested")->LinkByName("link2")
* Match up ModelByName and ModelByIndex expectations

Signed-off-by: Steve Peters <[email protected]>
Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @azeey and @EricCousineau-TRI)


test/integration/model_dom.cc, line 349 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Is there a way to assert that this link belongs to "nested"?

added this expectation in 6c7b3eb


test/integration/model_dom.cc, line 364 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Consider making this more explicit, and more explicitly check parity with ModelByName?

EXPECT_EQ(model->ModelByName("nested"), model->ModelByIndex(0));
EXPECT_EQ(model->ModelByName("shallow"), model->ModelByIndex(1));
EXPECT_EQ(nullptr, model->ModelByIndex(2));

ok, I've added this in 6c7b3eb

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @azeey)

@iche033
Copy link
Contributor

iche033 commented Sep 1, 2020

I tested this PR with gazebosim/gz-physics#66 and gazebosim/gz-sim#258. A model with a nested link and no direct child links now successfully loads in ign-gazebo. Changes look good to me.

@scpeters scpeters merged commit 8388f27 into gazebosim:sdf9 Sep 2, 2020
@scpeters scpeters deleted the nested_canonical branch September 2, 2020 00:30
scpeters added a commit to scpeters/sdformat that referenced this pull request Sep 2, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/sdformat that referenced this pull request Sep 2, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this pull request Sep 3, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/sdformat that referenced this pull request Sep 3, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this pull request Sep 3, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
traversaro pushed a commit to traversaro/sdformat that referenced this pull request Sep 5, 2020
Allow models without links if they have nested models instead.
When building FrameAttachedToGraph, if model has no links
choose the first link of the first nested model as canonical
link instead.

A new private function `Model::CanonicalLinkAndRelativeName`
is added that provides a `Link*` pointer to the canonical link and its
nested name relative to the current model, which is needed
in the FrameAttachedToGraph. This private prevents
duplicate code in `FrameSemantics.cc` and `Model::CanonicalLink`.
The method is private to hide :: syntax from libsdformat9,
at least until there is a compelling reason to make the API
public.

A helper function is added to FrameSemantics.cc as a friend
of Model so that buildFrameAttachedToGraph can call the
private API. That function can't be added directly
as a friend since it uses a `FrameAttachedToGraph&`
as an argument, which isn't defined in Model.hh.

Signed-off-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants