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 explicitly specified nested canonical links using :: syntax #355

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Sep 3, 2020

This is a partial step towards resolution of #342.

Support for implicit nested canonical links (i.e. when //model/@canonical_link is empty) was added in #341, but explicitly specifying a nested link using :: syntax (like <model name="M" canonical_link="nested_model::nested_link">) was not supported. This adds support for :: syntax in the Model::*ByName and Model::*NameExists functions, which is enough to support explicit specification of nested canonical links using :: syntax.

I split the test/sdf/nested_invalid_explicit_canonical_link.sdf test file into two parts, since it was demonstrating two different types of failures:

  • test/sdf/nested_explicit_canonical_link.sdf: now a valid file in 1.8
  • test/sdf/nested_without_links_invalid.sdf: still not supported

In this implementation, if :: is detected in the name passed to Model::*ByName and Model::*NameExists, those functions will interpret all instances of :: as separators between nested models and attempt to find a hierarchy of nested models that precedes the last instance of ::. If such a nested model cannot be found, it checks for an object that matches the full name (including instances of ::). I expect this behavior to be changed before we finish libsdformat11, but it is needed for now since included models are flattened by addNestedModel during parsing (see #284), so that some link, joint, and frame names may contain ::. I left some comments in Model::*ByName about how to change this behavior when :: become reserved and not allowed in frame names.


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2020

Codecov Report

Merging #355 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   87.40%   87.41%   +0.01%     
==========================================
  Files          60       60              
  Lines        9337     9346       +9     
==========================================
+ Hits         8161     8170       +9     
  Misses       1176     1176              
Impacted Files Coverage Δ
src/Model.cc 86.30% <100.00%> (+0.37%) ⬆️

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 113bf26...6659e83. Read the comment docs.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r1.
Reviewable status: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, and @scpeters)


src/Model.cc, line 719 at r2 (raw file):

  }

  if (nullptr != nextModel && index != std::string::npos)

How do we want to handle old SDFormat files that may have :: in a model name? For example

if test.sdf

<?xml version="1.0"?>
<sdf version='1.7'>
  <model name='MP'>
    <include>
      <uri>child_model</uri>
    </include>
  </model>
</sdf>

and child_model/model.sdf is

<?xml version="1.0"?>
<sdf version='1.7'>
  <model name='M1'>
    <link name="L1"/>
    <model name='M2'>
      <link name="L2"/>
    </model>
  </model>
</sdf>

The result of ign sdf -p test.sdf is

<sdf version='1.7'>
  <model name='MP'>
    <frame name='M1::__model__' attached_to='M1::L1'>
      <pose relative_to='__model__'>0 0 0 0 -0 0</pose>
    </frame>
    <link name='M1::L1'>
      <pose relative_to='M1::__model__'>0 0 0 0 -0 0</pose>
    </link>
    <model name='M1::M2'> <!-- This is actually be M2 in the output. I think it's a bug -->
      <link name='L2'/>
    </model>
  </model>
</sdf>

Querying for MP::M1::M2::L2 will return nullptr.

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: 7 of 10 files reviewed, 1 unresolved discussion (waiting on @azeey and @EricCousineau-TRI)


src/Model.cc, line 719 at r2 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

How do we want to handle old SDFormat files that may have :: in a model name? For example

if test.sdf

<?xml version="1.0"?>
<sdf version='1.7'>
  <model name='MP'>
    <include>
      <uri>child_model</uri>
    </include>
  </model>
</sdf>

and child_model/model.sdf is

<?xml version="1.0"?>
<sdf version='1.7'>
  <model name='M1'>
    <link name="L1"/>
    <model name='M2'>
      <link name="L2"/>
    </model>
  </model>
</sdf>

The result of ign sdf -p test.sdf is

<sdf version='1.7'>
  <model name='MP'>
    <frame name='M1::__model__' attached_to='M1::L1'>
      <pose relative_to='__model__'>0 0 0 0 -0 0</pose>
    </frame>
    <link name='M1::L1'>
      <pose relative_to='M1::__model__'>0 0 0 0 -0 0</pose>
    </link>
    <model name='M1::M2'> <!-- This is actually be M2 in the output. I think it's a bug -->
      <link name='L2'/>
    </model>
  </model>
</sdf>

Querying for MP::M1::M2::L2 will return nullptr.

You're right that this code will fail in that case, but I'm unsure how much to change it since I think this code is transitional (i.e. the composition proposal reserves :: so they won't be allowed in element names, which I expect will be implemented once we resolve #293 and #284). I do think this is a valuable test case, but I'm not sure if we should actually try to make it work in this branch or just add it as a disabled test that can be enabled when resolving #284 / #293.

Does that make sense?

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @scpeters)


src/Model.cc, line 719 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

You're right that this code will fail in that case, but I'm unsure how much to change it since I think this code is transitional (i.e. the composition proposal reserves :: so they won't be allowed in element names, which I expect will be implemented once we resolve #293 and #284). I do think this is a valuable test case, but I'm not sure if we should actually try to make it work in this branch or just add it as a disabled test that can be enabled when resolving #284 / #293.

Does that make sense?

:: is reserved in SDFormat 1.8, but not in 1.7, so I don't think this will be an uncommon case. One thought is to remove the :: during up-conversion and recreate the original nesting hierarchy based on the names, but I'm not sure how tractable that is.


test/integration/model_dom.cc, line 291 at r2 (raw file):

  const std::string innerLinkNestedName = innerModelNestedName + "::inner_link";
  EXPECT_TRUE(outerModel->LinkNameExists(innerLinkNestedName));
  EXPECT_NE(nullptr, outerModel->LinkByName(innerLinkNestedName));

FYI, you could compare this against innerModel->LinkByIndex(0) to be more precise.

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scpeters)

a discussion (no related file):
I guess I'm confused - why are you implementing this before #293 lands?

It seems like you'd need scaffolding that would go away once we have it, and there are also conversion issues that need to be solved as part of #293.

In #278, the way we formatted the dates (both #293 and #342), it seems like it could happen in parallel, but seeing y'all's discussion below, perhaps we'd want to wait?


a discussion (no related file):
In the overview, can you state (above the text explanation) that this is gearing towards #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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @scpeters)


src/Model.cc, line 719 at r2 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

:: is reserved in SDFormat 1.8, but not in 1.7, so I don't think this will be an uncommon case. One thought is to remove the :: during up-conversion and recreate the original nesting hierarchy based on the names, but I'm not sure how tractable that is.

Per f2f, it should be OK to be temporarily backwards-incompatible in code, given that:

  • We ensure that "master" is clearly denoted as unstable in the README
  • The breakage is shown in a unittest with a clear comment.
  • There's a TODO to resolve the comment afterwards.

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I guess I'm confused - why are you implementing this before #293 lands?

It seems like you'd need scaffolding that would go away once we have it, and there are also conversion issues that need to be solved as part of #293.

In #278, the way we formatted the dates (both #293 and #342), it seems like it could happen in parallel, but seeing y'all's discussion below, perhaps we'd want to wait?

OK See discussion below.


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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @scpeters)


src/Model.cc, line 722 at r2 (raw file):

  {
    return nextModel->ModelByName(_name.substr(index + 2));
  }

Per above, we should extend the composition proposal to talk about the up-conversion process.

Basically, what Addisu said: we should try to turn flattened models into nested models for declarations.

References would only be unflattened within the "unnested" elements (e.g. if link M1::B refers to M1::C, then it would then reference C inside of M1).

The hoisting / unflattening could happen prior to any semantic parsing, and then the unflattened XML can be handed to the nominal 1.8 parser.

One minor edge case:

<!-- 1.7 -->
<link name="M1::B">
   <pose relative_to="M2::B"/>
</link>
<link name="M2::B"/>

-->

<!-- 1.8 -->
<model name="M1">
   <link name="B">
     <pose relative_to="M2::B"/>  <!-- Invalid! -->
   </link>
</model>
<model name="M2">
  <link name="B"/>
</model>

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)


src/Model.cc, line 722 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per above, we should extend the composition proposal to talk about the up-conversion process.

Basically, what Addisu said: we should try to turn flattened models into nested models for declarations.

References would only be unflattened within the "unnested" elements (e.g. if link M1::B refers to M1::C, then it would then reference C inside of M1).

The hoisting / unflattening could happen prior to any semantic parsing, and then the unflattened XML can be handed to the nominal 1.8 parser.

One minor edge case:

<!-- 1.7 -->
<link name="M1::B">
   <pose relative_to="M2::B"/>
</link>
<link name="M2::B"/>

-->

<!-- 1.8 -->
<model name="M1">
   <link name="B">
     <pose relative_to="M2::B"/>  <!-- Invalid! -->
   </link>
</model>
<model name="M2">
  <link name="B"/>
</model>

Working: I will do this shortly.

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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @scpeters)


src/Model.cc, line 722 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: I will do this shortly.

Done: Filed gazebosim/sdf_tutorials#41

@EricCousineau-TRI
Copy link
Collaborator

Superseded by #381

@scpeters
Copy link
Member Author

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @scpeters)

src/Model.cc, line 719 at r2 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…
Per f2f, it should be OK to be temporarily backwards-incompatible in code, given that:

  • We ensure that "master" is clearly denoted as unstable in the README
  • The breakage is shown in a unittest with a clear comment.
  • There's a TODO to resolve the comment afterwards.

I'll do this

@codecov-io
Copy link

codecov-io commented Oct 22, 2020

Codecov Report

Merging #355 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   87.38%   87.40%   +0.01%     
==========================================
  Files          60       60              
  Lines        9341     9351      +10     
==========================================
+ Hits         8163     8173      +10     
  Misses       1178     1178              
Impacted Files Coverage Δ
src/Model.cc 86.30% <100.00%> (+0.37%) ⬆️
src/parser.cc 78.90% <100.00%> (+0.02%) ⬆️

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 144f82c...8a4e3df. Read the comment docs.

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 11 files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

In the overview, can you state (above the text explanation) that this is gearing towards #342?

I've added this to the PR description.



src/Model.cc, line 719 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

I'll do this

I've added a unit test showing the breakage with TODOs in 93658c3

I forgot to update the readme, will do that next

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 11 files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


src/Model.cc, line 719 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

I've added a unit test showing the breakage with TODOs in 93658c3

I forgot to update the readme, will do that next

readme in 8d3fdf2, this should be done by now


test/integration/model_dom.cc, line 291 at r2 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

FYI, you could compare this against innerModel->LinkByIndex(0) to be more precise.

added some more expectations in 026434e

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 12 files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


test/integration/model_dom.cc, line 291 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

added some more expectations in 026434e

I added them incorrectly, fixed in fef1d57

Copy link
Collaborator

@azeey azeey 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 2 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)


test/sdf/partially_flattened.sdf, line 2 at r4 (raw file):

<sdf version='1.7'>
  <model name='MP'>

I assume this was generated from an existing "included" model. Can you add a comment about which files were used to generate this?

Copy link
Collaborator

@azeey azeey 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 minor comment.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

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: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

I've added this to the PR description.

Done.



test/sdf/partially_flattened.sdf, line 2 at r4 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

I assume this was generated from an existing "included" model. Can you add a comment about which files were used to generate this?

I copied it directly from your comment: #355 (review)

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.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)

a discussion (no related file):
Steve will incorporate forward-porting of this PR:
#399


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: 10 of 16 files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Steve will incorporate forward-porting of this PR:
#399

Done



test/sdf/partially_flattened.sdf, line 2 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

I copied it directly from your comment: #355 (review)

after cherry-picking #399, I removed this file and changed the test to use nested_model_with_frames_expected.sdf, which now has a partially flattened nested model

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_strong:

Reviewed 8 of 10 files at r1, 1 of 2 files at r4, 7 of 7 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey)

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


test/sdf/partially_flattened.sdf, line 2 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

after cherry-picking #399, I removed this file and changed the test to use nested_model_with_frames_expected.sdf, which now has a partially flattened nested model

Ok. Thanks!

@scpeters
Copy link
Member Author

I'm going to squash everything but #399 and then rebase and merge the two commits

Currently the addNestedModel function in parser.cc prefixes
link, joint, and frame names with the flattened model name
delimited by "::". This applies the same prefix to the names
of nested models within the flattened model as well.

Signed-off-by: Steve Peters <[email protected]>
This extends the `Model::*NameExists` and `Model::*ByName` APIs
(like LinkNameExists and LinkByName) that allow passing
nested names that can begin with a sequence of nested model
names separated by :: and may end with the name of an object
of the specified type, such as "outer_model::inner_model::inner_joint".

For now, if a nested model is not found that matches the nested name
preceding the final ::, then it checks for objects in the current model
that match the entire name. This extra check should be disabled
when "::" is reserved and not allowed in frame names.

Add test showing that Model::ModelByName and Model::ModelNameExists
have trouble with model names that contain "::" with TODO
comments to remind that they should be fixed.

* README: note master is unstable development branch

Split nested_invalid_explicit_canonical_link.sdf
into nested_explicit_canonical_link.sdf and
nested_without_links_invalid.sdf and update tests
to expect that nested_explicit_canonical_link.sdf is valid.

Part of gazebosim#342.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters merged commit 9c5f750 into gazebosim:master Oct 30, 2020
@scpeters scpeters deleted the explicit_nested_canonical branch October 30, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants