Support explicitly specified nested canonical links using :: syntax #355
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 exampleif
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.sdfis<?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.sdfis<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::L2will 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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::Brefers toM1::C, then it would then referenceCinside ofM1).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.
There was a problem hiding this comment.
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
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
|
Superseded by #381 |
I'll do this |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Reviewed 7 of 7 files at r5.
Reviewable status: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!
|
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 <scpeters@openrobotics.org>
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 <scpeters@openrobotics.org>
scpeters
force-pushed
the
explicit_nested_canonical
branch
from
8a4e3df
to
d0af5f8
Compare
This is a partial step towards resolution of #342.
Support for implicit nested canonical links (i.e. when
//model/@canonical_linkis 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 theModel::*ByNameandModel::*NameExistsfunctions, which is enough to support explicit specification of nested canonical links using::syntax.I split the
test/sdf/nested_invalid_explicit_canonical_link.sdftest file into two parts, since it was demonstrating two different types of failures:In this implementation, if
::is detected in the name passed toModel::*ByNameandModel::*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 byaddNestedModelduring parsing (see #284), so that some link, joint, and frame names may contain::. I left some comments inModel::*ByNameabout how to change this behavior when::become reserved and not allowed in frame names.This change is