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

composition: Add section for up-converting unflattened models #41

Conversation

Copy link
Member

@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.

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


composition/proposal.md, line 61 at r1 (raw file):

behavior than direct nesting with `//model/model`,
part of the reason being that nesting directly with `//model/model` was not
implemented in the `libsdformat` until 9.3.0. As mentioned in the

nit: in the libsdformat -> in libsdformat


composition/proposal.md, line 64 at r1 (raw file):

[Legacy Behavior](/tutorials?tut=composition&ver=1.5&#libsdformats-implementation-of-include-in-models) documentation, `//include` works by effectively flattening the model; when this happens, certain details may "leak" through.

Normall, nesting of models generally implies that the elements can be

Normally


composition/proposal.md, line 583 at r1 (raw file):

`M1::my_link`) should be "unnested" (e.g. `my_link` in model `M1`).

To illustrate, the following model from the Legacy Behavior documentation:

add hyperlink? http://sdformat.org/tutorials?tut=composition&cat=specification&#libsdformats-implementation-of-include-in-models


composition/proposal.md, line 597 at r1 (raw file):

      </visual>
    </link>
    <link name="ChildModel::L2"/>

I think the example in the legacy behavior documentation may be wrong; I think this link would have a pose of 1 0 1 0 0 0


composition/proposal.md, line 662 at r1 (raw file):

* Since the naively converted file is neither SDFormat 1.5 nor 1.8, it is
indicated as SDFormat `1.5*`, meaning that it's an intermediate format which
namely adheres to SDFormat 1.5, but has directly nested models.

SDFormat 1.5 supports directly nested models in the spec, so I think we could leave it as 1.5


composition/proposal.md, line 665 at r1 (raw file):

* Since `libsdformat9.3` supported direct nesting but `//include` still
worked via flattening, unflattening will occur regardless of whether or not
there are directly nested models in the model, and the unflattening my handle

my -> may


composition/proposal.md, line 670 at r1 (raw file):

the newly created `//model/pose`.
  * This offers simplicity and ensures that the effective `__model__` frame
  will coincide with the newly created nested models' `__model__` frames.

as of libsdformat 9.2, the a flattened model would also have an injected ChildModel::__model__ per bitbucket pr 668; perhaps we need to upgrade the legacy behavior documentation a bit; this reasoning is valid for versions < 9.2 but doesn't cover the injected __model__ frames

Copy link
Collaborator Author

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


composition/proposal.md, line 61 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: in the libsdformat -> in libsdformat

Done.


composition/proposal.md, line 64 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

Normally

Done.


composition/proposal.md, line 583 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

add hyperlink? http://sdformat.org/tutorials?tut=composition&cat=specification&#libsdformats-implementation-of-include-in-models

Done.


composition/proposal.md, line 597 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I think the example in the legacy behavior documentation may be wrong; I think this link would have a pose of 1 0 1 0 0 0

Done. Good catch!


composition/proposal.md, line 662 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

SDFormat 1.5 supports directly nested models in the spec, so I think we could leave it as 1.5

Done. I actually changed it to 1.4; if we think we won't run into this in the wild, then I could strip it out entirely.


composition/proposal.md, line 665 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

my -> may

Done.

Copy link
Collaborator Author

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


composition/proposal.md, line 680 at r3 (raw file):

  will coincide with the newly created nested models' `__model__` frames.

**Example of a failing conversions**

Working: Should also try to explicitly state where this happens in high or low level parsing stages.

  • readDoc - may handle the conversion, then pass off to readXml
  • readXml - should handle the latest version

Copy link
Collaborator Author

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


composition/proposal.md, line 670 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

as of libsdformat 9.2, the a flattened model would also have an injected ChildModel::__model__ per bitbucket pr 668; perhaps we need to upgrade the legacy behavior documentation a bit; this reasoning is valid for versions < 9.2 but doesn't cover the injected __model__ frames

Working: Will update examples (legacy and here) that incorporate the __model__ frames.

Copy link
Member

@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.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Will update examples (legacy and here) that incorporate the __model__ frames.

Will first confirm what happens with SDFormat 1.4 and include.

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Will first confirm what happens with SDFormat 1.4 and include.

Will modified tests introduced here:
https://github.com/osrf/sdformat/compare/cc1f9b00e31b991a71343db5afbe3665d8af1ff8..4836e3d2577fe4e7fe52a94805bc8a9ba6bb432d

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Will modified tests introduced here:
https://github.com/osrf/sdformat/compare/cc1f9b00e31b991a71343db5afbe3665d8af1ff8..4836e3d2577fe4e7fe52a94805bc8a9ba6bb432d

Setup:

Using this workspace setup: EricCousineau-TRI/repro@0162a3d

colcon build
cd build/sdformat11
make INTEGRATION_nested_model  # For intermediate changes.
./test/integration/INTEGRATION_nested_model --gtest_filter=NestedModel.NestedModelWithFramesDirectComparison

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Setup:

Using this workspace setup: EricCousineau-TRI/repro@0162a3d

colcon build
cd build/sdformat11
make INTEGRATION_nested_model  # For intermediate changes.
./test/integration/INTEGRATION_nested_model --gtest_filter=NestedModel.NestedModelWithFramesDirectComparison

With this branch: gazebosim/sdformat@master...EricCousineau-TRI:feature-composition-model-inject-wip | gazebosim/sdformat@3f46da9

Some minor notes (related to above) discussion - but setting /sdf/@version="1.4" doesn't seem to trip anything up in the parsing setup.
Makes senese, just wanted to confirm.

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

With this branch: gazebosim/sdformat@master...EricCousineau-TRI:feature-composition-model-inject-wip | gazebosim/sdformat@3f46da9

Some minor notes (related to above) discussion - but setting /sdf/@version="1.4" doesn't seem to trip anything up in the parsing setup.
Makes senese, just wanted to confirm.

Here's what the flattened model looks like with SDFormat 1.8 (assuming it'll be the same for SDFormat 1.7):

gazebosim/sdformat@56d076d2

Copy link
Collaborator Author

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


composition/legacy_behavior.md, line 223 at r4 (raw file):

```xml
<model name="ParentModel">

BTW I've replaced this with the output of current libsdformat, but with some excess default values trimmed.


composition/proposal.md, line 670 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Here's what the flattened model looks like with SDFormat 1.8 (assuming it'll be the same for SDFormat 1.7):

gazebosim/sdformat@56d076d2

Done.


composition/proposal.md, line 668 at r3 (raw file):

been emitted by `libsdformat10` by using *valid* `//include` statements. It is possible to write valid models in SDFormat 1.7 that are invalid in SDFormat
1.8; no effort will be made to reconcile those models (see examples below).
* Since the naively converted file is neither SDFormat 1.5 nor 1.8, it is

BTW I've removed this section for now. Per mention below, the whole "is the version tag strictly true" seems like a separate concern.

Copy link
Member

@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.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


composition/legacy_behavior.md, line 223 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW I've replaced this with the output of current libsdformat, but with some excess default values trimmed.

looks good. How do you feel about adding a duplicate versioned 1.7 file with these changes and leaving the old version in a separate file? I can do the grunt work


composition/proposal.md, line 679 at r4 (raw file):

`ChildModel::__model__`) will have this frame removed, and this will be
converted to the appropriate `//model/pose`. If `@attached_to` is specified to
something other than this nexted model's first link, then

nexted -> nested

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 comments

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


composition/proposal.md, line 680 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Should also try to explicitly state where this happens in high or low level parsing stages.

  • readDoc - may handle the conversion, then pass off to readXml
  • readXml - should handle the latest version

Working: What if we create a new conversion keyword "unnest" and do it in the Convert class? This would only be done in a 1.7->1.8 conversion.


composition/proposal.md, line 670 at r4 (raw file):

the naive up-conversion process.
* This up-conversion is *only* intended to support models that *could* have
been emitted by `libsdformat10` by using *valid* `//include` statements. It is possible to write valid models in SDFormat 1.7 that are invalid in SDFormat

nit: libsdformat10 -> libsdformat9.3?

Copy link
Member

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


composition/proposal.md, line 680 at r3 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

Working: What if we create a new conversion keyword "unnest" and do it in the Convert class? This would only be done in a 1.7->1.8 conversion.

that makes sense to me


composition/proposal.md, line 670 at r4 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

nit: libsdformat10 -> libsdformat9.3?

flattening has been happening since before libsdformat4 and its behavior changed in libsdformat 9.2 with the inclusion of bitbucket PR 668. maybe we could just omit the version and say libsdformat?

Copy link
Collaborator Author

@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)


composition/proposal.md, line 670 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

flattening has been happening since before libsdformat4 and its behavior changed in libsdformat 9.2 with the inclusion of bitbucket PR 668. maybe we could just omit the version and say libsdformat?

Actually, this bit of history seems useful. Will incorporate it here.

Copy link
Collaborator Author

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


composition/proposal.md, line 680 at r3 (raw file):

Previously, scpeters (Steve Peters) wrote…

that makes sense to me

Er, not sure what "conversion keyword" means. Can you point to that context?


composition/proposal.md, line 670 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Actually, this bit of history seems useful. Will incorporate it here.

Done. Incorporated libsdformat4.

However, I don't think the inclusion of pose frame semantics matters in this exact bullet point. Yes, the below model does use pose frame semantics, but an invalid model could also arise from an invalid specification of other interface elements, e.g. //joint/child, //join/parent, etc.

I've updated the example below to use pre-pose-frame semantics stuff as a better illustration.


composition/proposal.md, line 679 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

nexted -> nested

Done. Oops!

Copy link
Collaborator Author

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


composition/proposal.md, line 680 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er, not sure what "conversion keyword" means. Can you point to that context?

Yeah, also not sure what the gain is there? Can talk more f2f.

@EricCousineau-TRI
Copy link
Collaborator Author

Per f2f:

  • EC: Conversion keywords for unnesting?
    • AT: Add it to the conversion schema, like <unnest/> or something

e.g. code

Copy link
Member

@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.

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


composition/proposal.md, line 680 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yeah, also not sure what the gain is there? Can talk more f2f.

I think we agreed on this during f2f today. Does this idea require any text changes here?


composition/proposal.md, line 83 at r6 (raw file):

legacy format. Generally, the interface between models only really needs access
to explicit and implicit frames (for welding joints, attaching sensors, etc.).
The present implementation of `//include` (present

nit: using "present" twice so closely sounds funny.

  • "The current implementation"
    or
  • remove "present" from the parenthetical "(since `libsdformat4)"

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/proposal.md, line 680 at r3 (raw file):

Previously, scpeters (Steve Peters) wrote…

I think we agreed on this during f2f today. Does this idea require any text changes here?

Yeah, will add a brief blurb above about the potential implementation.

Copy link
Collaborator Author

@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 @EricCousineau-TRI)


composition/legacy_behavior.md, line 223 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

looks good. How do you feel about adding a duplicate versioned 1.7 file with these changes and leaving the old version in a separate file? I can do the grunt work

OK Sounds good to me! I'm assuming that'll happen in a separate PR, so will get this 'un merged in.

Copy link
Collaborator Author

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


composition/proposal.md, line 680 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yeah, will add a brief blurb above about the potential implementation.

Done. Added in Addisu's suggestion; we can change later if it becomes different.


composition/proposal.md, line 83 at r6 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: using "present" twice so closely sounds funny.

  • "The current implementation"
    or
  • remove "present" from the parenthetical "(since `libsdformat4)"

Done.

@EricCousineau-TRI EricCousineau-TRI merged commit 51fe4f3 into gazebosim:master Oct 1, 2020
scpeters added a commit that referenced this pull request Oct 1, 2020
Copied legacy_behavior.md from 66d3d88 (before #41)
to legacy_behavior_1-5.md

Signed-off-by: Steve Peters <[email protected]>
EricCousineau-TRI pushed a commit that referenced this pull request Oct 2, 2020
Copied legacy_behavior.md from 66d3d88 (before #41)
to legacy_behavior_1-5.md

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

composition: Should mention that explicitly flattened models may break
3 participants