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

Add //model/@placement_frame to proposal #42

Merged
merged 4 commits into from
Sep 30, 2020
Merged

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Sep 16, 2020

@azeey azeey added this to the SDFormat 1.8 / libsdformat11 milestone Sep 16, 2020
@azeey azeey self-assigned this Sep 16, 2020
@azeey azeey changed the title Add //model/@placement_frame to composition proposal Add //model/@placement_frame to proposal Sep 16, 2020
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: with two small comments

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


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

~~~xml
<model name="super_model">
  <model name="table" placement_frame="bottom_left_leg">

BTW In an example here or in a unittest in code, should we note the interaction between placement_frame and @canonical_link?

(i.e. they are similarly-scoped aspects, but orthogonal in application... or something less obtuse than that :P)


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

  </include>
</model>
~~~

nit Consider mentioning that we originally thought about just having //include/placement_frame, but found it'd be hard to implement w/o it being a proper model attribute.

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


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

      <pose relative_to="bottom_center">0 0 2 0 0 0</pose>
    </frame>
    <link name="bottom_left_leg"/>

I would add a pose offset with non-zero values of x and y translation for this example


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

    <link name="bottom_left_leg"/>
  </model>
  <model name="mug" placement_frame="bottom_center">

having frames named bottom_center in both models makes it a little confusing. we could add some comments clarifying scope, or use different names just for clarity (center_bottom?)


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Consider mentioning that we originally thought about just having //include/placement_frame, but found it'd be hard to implement w/o it being a proper model attribute.

this could be an "alternative considered"

Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Copy link
Collaborator Author

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

I turned on the spell checker before I pushed and found some typos. Fixed in f0457c9

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


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

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW In an example here or in a unittest in code, should we note the interaction between placement_frame and @canonical_link?

(i.e. they are similarly-scoped aspects, but orthogonal in application... or something less obtuse than that :P)

Done. 0fcaee0


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

Previously, scpeters (Steve Peters) wrote…

I would add a pose offset with non-zero values of x and y translation for this example

I added a pose to the model in 0fcaee0. Is that what you had in mind?


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

Previously, scpeters (Steve Peters) wrote…

having frames named bottom_center in both models makes it a little confusing. we could add some comments clarifying scope, or use different names just for clarity (center_bottom?)

I removed bottom_center from table and used bottom_left_leg as the relative_to for specifying top_center. 0fcaee0


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

Previously, scpeters (Steve Peters) wrote…

this could be an "alternative considered"

Done. 0fcaee0

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 r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey)


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

Previously, azeey (Addisu Z. Taddese) wrote…

I added a pose to the model in 0fcaee0. Is that what you had in mind?

LGTM


composition/proposal.md, line 516 at r2 (raw file):

will have that pose. This is a very practical means of setting the location of
models. The `//model/@canonical_link` attribute on the other hand specifies to
which link the implicit model frame is attached. This does not affect the

nit: "specifies to which link" -> "specifies the link to which"

Signed-off-by: Addisu Z. Taddese <[email protected]>
Copy link
Collaborator Author

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @scpeters)


composition/proposal.md, line 516 at r2 (raw file):

Previously, scpeters (Steve Peters) wrote…

nit: "specifies to which link" -> "specifies the link to which"

Done. 579eb16

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.

:lgtm:

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

@azeey azeey merged commit e96466b into master Sep 30, 2020
@azeey azeey deleted the model_placement_frame branch September 30, 2020 21:34
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.

Add //model/@placement_frame to composition proposal
3 participants