Add nested models to SDFormat 1.7 proposal #34

scpeters · 2020-07-23T19:09:15Z

This adds support for nested models to the SDFormat 1.7 proposal. The spec supports nested models, so even though it wasn't included in libsdformat 9.2 and earlier, we could consider adding it now.

This currently has only updated the parsing stages. It is a draft until the prose is updated as well for consistency.

http://sdformat.org/tutorials?tut=pose_frame_semantics_proposal&branch=nested_model_dom_9

This change is

EricCousineau-TRI

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

EricCousineau-TRI

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

a discussion (no related file):
This should have changes in composition/legacy.md mentioning that directly nested models will be supported:
https://github.com/osrf/sdf_tutorials/blame/044297cf20b0e50cb9ed81c811355363eb393be4/composition/legacy_behavior.md#L43-L45

pose_frame_semantics/proposal.md, line 9 at r1 (raw file):

* **Status**: Final
* **SDFormat Version**: 1.7
* **`libsdformat` Version**: 9.0

This should be updated, with a note mentioning that the directly nested models will be supported in 9.3.0.

EricCousineau-TRI

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

pose_frame_semantics/proposal.md, line 9 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This should be updated, with a note mentioning that the directly nested models will be supported in 9.3.0.

(Additionally, the sections changed should mention as much?)

scpeters · 2020-08-07T06:40:52Z

ready for review: http://sdformat.org/tutorials?tut=pose_frame_semantics_proposal&branch=nested_model_dom_9

azeey

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @scpeters)

pose_frame_semantics/proposal.md, line 774 at r2 (raw file):

  <model name="cycle1">
    <pose relative_to="cycle2">{X_C1C2}</pose>

nit: Should the model names be C1, C2, etc? That would make X_C1C2 clearer.

pose_frame_semantics/proposal.md, line 882 at r2 (raw file):

    <model name="N">
      <pose relative_to="F">{X_FN}</pose>     <!-- Pose relative relative to explicit frame F (F -> M) in model M1's scope. -->
      <link name="L"/>

nit: Can we rename the link to something other that L so it's not confusing when we reference the canonical link L of M1.

pose_frame_semantics/proposal.md, line 884 at r2 (raw file):

      <link name="L"/>
    </model>
    <frame name="F0">                         <!-- Frame indirectly attached_to canonical link link L via model frame. -->

nit: "link link"

pose_frame_semantics/proposal.md, line 1526 at r2 (raw file):

5.  ***Check `//model/@canonical_link` attribute value:***
    For models that are not static,
    if the `//model/@canonical_link` attribute exists and is not an empty

BTW, I know we're discussing parent models that use a child model's link as their canonical links in #36. How about a child model using the canonical link of the parent model? For example

<model name="P">
  <link name="L"/>
  <model name="C">
     <frame name="F"/>
  </model>
</model>

C doesn't have a link so this would be an error, but I can see how it would be useful to allow C's model frame be attached to L.

pose_frame_semantics/proposal.md, line 1553 at r2 (raw file):

    7.1 Add a vertex for the implicit frame of each `//link`,
        `//joint`, `//frame`, and nested `//model` in the model

nit: //frame is not an an implicit frame.

pose_frame_semantics/proposal.md, line 1571 at r2 (raw file):

    7.4.1 If `//model/frame/@attached_to` exists and is not empty,
          add an edge from the added vertex to the vertex

nit: It's not clear to say "added vertex" anymore since the heading is not "Add a vertex to the graph"

scpeters

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This should have changes in composition/legacy.md mentioning that directly nested models will be supported:
https://github.com/osrf/sdf_tutorials/blame/044297cf20b0e50cb9ed81c811355363eb393be4/composition/legacy_behavior.md#L43-L45

ok, added in c2beb7d

pose_frame_semantics/proposal.md, line 9 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

(Additionally, the sections changed should mention as much?)

ok, updated in af86d42

pose_frame_semantics/proposal.md, line 774 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: Should the model names be C1, C2, etc? That would make X_C1C2 clearer.

done in 79d4139

pose_frame_semantics/proposal.md, line 882 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: Can we rename the link to something other that L so it's not confusing when we reference the canonical link L of M1.

ok, good idea, I renamed to NL in 6609648

pose_frame_semantics/proposal.md, line 884 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: "link link"

ok, fixed in 6609648

pose_frame_semantics/proposal.md, line 1526 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

BTW, I know we're discussing parent models that use a child model's link as their canonical links in #36. How about a child model using the canonical link of the parent model? For example
<model name="P">
  <link name="L"/>
  <model name="C">
     <frame name="F"/>
  </model>
</model>
C doesn't have a link so this would be an error, but I can see how it would be useful to allow C's model frame be attached to L.

it breaks encapsulation, but it could be useful. let's discuss at our next meeting

pose_frame_semantics/proposal.md, line 1553 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: //frame is not an an implicit frame.

ok, removed "implicit" in 6609648

pose_frame_semantics/proposal.md, line 1571 at r2 (raw file):

Previously, azeey (Addisu Taddese) wrote…

nit: It's not clear to say "added vertex" anymore since the heading is not "Add a vertex to the graph"

ok, improved phrasing in 6609648

EricCousineau-TRI

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

pose_frame_semantics/proposal.md, line 9 at r4 (raw file):

* **Status**: Final
* **SDFormat Version**: 1.7
* **`libsdformat` Version**: 9.0 (initial support), 9.3 (nested models)

On second perusal, I want us to have an explicit amendment structure as in #36.

My recommendation is that #36 lands first as it is simple, and then these changes are replayed on top of it.
That sounds OK?

EricCousineau-TRI

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

pose_frame_semantics/proposal.md, line 374 at r4 (raw file):

link is within a nested model, but the scope is still limited to
objects contained in the current model.
The `::` syntax for referencing across model boundaries is not supported

nit (readability) This is shown as a parallel between SDFormat 1.5 to 1.7, but may be hard to quickly read as such.

Suggestions:

Put an actual newline between the two sentences (either two newlines in Markdown, or a literal <br/> / \)
Lead both sentences with In SDFormat 1.x

pose_frame_semantics/proposal.md, line 375 at r4 (raw file):

objects contained in the current model.
The `::` syntax for referencing across model boundaries is not supported
by SDFormat 1.7.

I think this statement "undersells" by not mentioning the plan for 1.8.

Can you reword this to that effect?

In SDFormat 1.5, <it worked>
In SDFormat 1.7, <it won't work>
In SDFormat 1.8, <it'll work again>

EricCousineau-TRI

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

pose_frame_semantics/proposal.md, line 1526 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

it breaks encapsulation, but it could be useful. let's discuss at our next meeting

OK I think this is useful to open as an issue. I've opened it here, but I think it's hardcore scope creep to consider this functionality for libsdformat9 / SDFormat 1.7
gazebosim/sdformat#336

EricCousineau-TRI

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

pose_frame_semantics/proposal.md, line 602 at r4 (raw file):

  <frame name="F1" attached_to="M"/>   <!-- VALID: Indirectly attached_to nested model M. -->
  <frame name="F2" attached_to="F1"/>  <!-- VALID: Indirectly attached_to nested model M via frame F1. -->
  <frame name="F3" attached_to="A"/>   <!-- INVALID: no sibling frame named A. -->

I'm not sure if I understand the value of mentioning "A" here.
There does not seem to be any connection between this model and any of the other models.

Consider:

Removing this example outright?
Using a more informative name?

<frame name="F3" attached_to="this_frame_is_not_defined_anywhere">

If you do keep it, be sure to reinforce that this is due to encapsulation?

pose_frame_semantics/proposal.md, line 749 at r4 (raw file):

~~~
<model name="nested_model_pose_relative_to">

nit M vs. M1 (or M2, etc.) might be slightly ambiguous.

Consider:

Explicitly defining frame M here
Choose a different name, like Mp, so then instead of X_MM1, you have X_MpM1.

This adds support for nested models to the SDFormat 1.7 proposal. The spec supports nested models, so even though it wasn't included in libsdformat 9.2 and earlier, we could consider adding it now. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

scpeters

I just rebased on master after #36 was merged.

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI)

pose_frame_semantics/proposal.md, line 9 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

On second perusal, I want us to have an explicit amendment structure as in #36.

My recommendation is that #36 lands first as it is simple, and then these changes are replayed on top of it.
That sounds OK?

Done.

pose_frame_semantics/proposal.md, line 374 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (readability) This is shown as a parallel between SDFormat 1.5 to 1.7, but may be hard to quickly read as such.

Suggestions:

Put an actual newline between the two sentences (either two newlines in Markdown, or a literal <br/> / \)

Lead both sentences with In SDFormat 1.x

I did both in 57230f5: added newlines between sentences and lead with `In SDFormat 1.X,

pose_frame_semantics/proposal.md, line 375 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I think this statement "undersells" by not mentioning the plan for 1.8.

Can you reword this to that effect?
In SDFormat 1.5, <it worked>
In SDFormat 1.7, <it won't work>
In SDFormat 1.8, <it'll work again>

I added an "In SDFormat 1.8" sentence in 57230f5

pose_frame_semantics/proposal.md, line 602 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'm not sure if I understand the value of mentioning "A" here.
There does not seem to be any connection between this model and any of the other models.

Consider:

Removing this example outright?

Using a more informative name?
<frame name="F3" attached_to="this_frame_is_not_defined_anywhere">
If you do keep it, be sure to reinforce that this is due to encapsulation?

I removed that line in 57230f5

pose_frame_semantics/proposal.md, line 749 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit M vs. M1 (or M2, etc.) might be slightly ambiguous.

Consider:

Explicitly defining frame M here

Choose a different name, like Mp, so then instead of X_MM1, you have X_MpM1.

I defined an explicit frame M in 57230f5

EricCousineau-TRI

Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scpeters)

pose_frame_semantics/proposal.md, line 9 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

Done.

Can you remove the changes here? (or add an asterisk about looking at the amendments?)

pose_frame_semantics/proposal.md, line 374 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

I did both in 57230f5: added newlines between sentences and lead with `In SDFormat 1.X,

OK Thanks!!!

pose_frame_semantics/proposal.md, line 749 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

I defined an explicit frame M in 57230f5

OK Thanks!

pose_frame_semantics/proposal.md, line 48 at r6 (raw file):

An initial version of this proposal without support for nested models was
implemented in `libsdformat` 9.0, while support for nested models is targeted
for `libsdformat` 9.3.

Can you reference the amendment instead of (or in addition to) libsdformat version?

pose_frame_semantics/proposal.md, line 283 at r6 (raw file):

referenced using the model's specified name.

As of `libsdformat 9.3`, nested models have an "external implicit model frame"

Can you reference the amendment instead of (or in addition to) libsdformat version?

pose_frame_semantics/proposal.md, line 777 at r6 (raw file):

~~~
<model name="nested_model_pose_relative_to">
  <frame name="M"/>                         <!-- Explicit frame M identical to implicit model frame __model__. -->

BTW "identical" may be a bit strong if unspecified. Perhaps "Explicit frame M that is coincident with model frame __model__"?

* revert a line discussing libsdformat versions * mention Amendment 1 in two places * use "coincident to" instead of "identical to" Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

scpeters

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI)

pose_frame_semantics/proposal.md, line 9 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you remove the changes here? (or add an asterisk about looking at the amendments?)

removed the change in 9b0e7bb

pose_frame_semantics/proposal.md, line 48 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you reference the amendment instead of (or in addition to) libsdformat version?

referenced the amendment in 9b0e7bb

pose_frame_semantics/proposal.md, line 283 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you reference the amendment instead of (or in addition to) libsdformat version?

referenced the amendment in 9b0e7bb

pose_frame_semantics/proposal.md, line 777 at r6 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW "identical" may be a bit strong if unspecified. Perhaps "Explicit frame M that is coincident with model frame __model__"?

changed to "coincident with" in 8debadb

EricCousineau-TRI

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved

scpeters requested review from azeey and EricCousineau-TRI July 23, 2020 19:09

EricCousineau-TRI mentioned this pull request Jul 23, 2020

[epic] proposal: Should provide better composition behavior gazebosim/sdformat#278

Closed

17 tasks

EricCousineau-TRI reviewed Jul 23, 2020

View reviewed changes

EricCousineau-TRI reviewed Jul 24, 2020

View reviewed changes

scpeters marked this pull request as ready for review August 7, 2020 06:40

azeey approved these changes Aug 12, 2020

View reviewed changes

scpeters commented Aug 13, 2020

View reviewed changes

EricCousineau-TRI requested changes Aug 13, 2020

View reviewed changes

EricCousineau-TRI mentioned this pull request Aug 13, 2020

Should permit nesting models without links (e.g. defining frames / visuals / collision only)? gazebosim/sdformat#336

Closed

EricCousineau-TRI reviewed Aug 13, 2020

View reviewed changes

EricCousineau-TRI requested changes Aug 13, 2020

View reviewed changes

scpeters added 5 commits August 13, 2020 14:47

Add nested models to rest of proposal

3aa3bb9

Signed-off-by: Steve Peters <[email protected]>

Rename cycle frames to C1, C2

9ee5c3f

Signed-off-by: Steve Peters <[email protected]>

Improve phrasing, suggested edits

077b488

Signed-off-by: Steve Peters <[email protected]>

Nested model support in 9.3 in composition docs

15fee5d

Signed-off-by: Steve Peters <[email protected]>

scpeters force-pushed the nested_model_dom_9 branch from c2beb7d to 15fee5d Compare August 13, 2020 22:20

Respond to review comments

57230f5

Signed-off-by: Steve Peters <[email protected]>

scpeters commented Aug 13, 2020

View reviewed changes

EricCousineau-TRI requested changes Aug 14, 2020

View reviewed changes

scpeters added 2 commits August 14, 2020 13:43

Respond to more review comments

9b0e7bb

* revert a line discussing libsdformat versions * mention Amendment 1 in two places * use "coincident to" instead of "identical to" Signed-off-by: Steve Peters <[email protected]>

"coincident with" instead of "coincident to"

8debadb

Signed-off-by: Steve Peters <[email protected]>

scpeters commented Aug 14, 2020

View reviewed changes

EricCousineau-TRI approved these changes Aug 14, 2020

View reviewed changes

scpeters merged commit e8d60cf into master Aug 14, 2020

scpeters deleted the nested_model_dom_9 branch August 14, 2020 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nested models to SDFormat 1.7 proposal #34

Add nested models to SDFormat 1.7 proposal #34

scpeters commented Jul 23, 2020 •

edited

Loading

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

scpeters commented Aug 7, 2020

azeey left a comment

scpeters left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

scpeters left a comment

EricCousineau-TRI left a comment

scpeters left a comment

EricCousineau-TRI left a comment

Add nested models to SDFormat 1.7 proposal #34

Add nested models to SDFormat 1.7 proposal #34

Conversation

scpeters commented Jul 23, 2020 • edited Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters commented Aug 7, 2020

azeey left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters commented Jul 23, 2020 •

edited

Loading