Skip to content

Merging for Composition: publish proposal#74

Merged
scpeters merged 14 commits intomasterfrom
scpeters/feature-include-merge
May 20, 2022
Merged

Merging for Composition: publish proposal#74
scpeters merged 14 commits intomasterfrom
scpeters/feature-include-merge

Conversation

@scpeters
Copy link
Copy Markdown
Member

@scpeters scpeters commented May 11, 2022

This is a resubmission of #55 by me to take over from @EricCousineau-TRI. I have addressed all feedback from that pull request.


This change is Reviewable

EricCousineau-TRI and others added 13 commits August 6, 2021 17:23
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copied text from gazebosim/sdformat#659.
Left a placeholder for an example of decomposing
an existing model into separate model files
using merge-include.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Add abridged version of husky with sensors on a pan-tilt
gimbal and show how to decompose it.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Also discuss how proxy frame name is implementation detail.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy link
Copy Markdown
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 a couple of small nits

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey and @scpeters)


composition/merge_proposal.md line 7 at r2 (raw file):

Addisu Taddese  `<addisu@openrobotics.org>`
Steve Peters `<scpeters@openrobotics.org>`
* **Status**: Draft

nit Should we replace "Draft" with "Accepted" / "Implemented"?


composition/merge_proposal.md line 32 at r2 (raw file):

while maintaining the encapsulation provided by SDFormat 1.8.
The cost of this feature is that users must take care to avoid name collisions
between the entities of the models to be merged.

nit Can we provide mention of "likeness" to Python? e.g.

It may be helpful to image this as Python module imports.
With a normal include, a model include is similar to an import such as
`import my_model`. If you rename, then it is like `import my_model as renamed_model`.
With a merge include, it's effectively the same as `from my_model import *`.

(from slides on Thurs)


composition/merge_proposal.md line 176 at r2 (raw file):

The following is an abridged version of a Clearpath Husky skid-steer with
sensors mounted on a pan-tilt gimbal used in the DARPA Subterranean Challenge
([MARBLE HUSKY SENSOR CONFIG 3](https://app.ignitionrobotics.org/OpenRobotics/fuel/models/MARBLE_HUSKY_SENSOR_CONFIG_3)):

nit Can you ensure that this is pinned to particular version / revision? e.g.
https://app.gazebosim.org/OpenRobotics/fuel/models/MARBLE_HUSKY_SENSOR_CONFIG_3/10


composition/merge_proposal.md line 428 at r2 (raw file):

~~~

## Appendix

nit Should we put "(Unused)" as the body text here?

* Status: Accepted
* Add namespacing analogy for python module imports
* Link to revision 10 of model in Fuel
* Appendix (Unused)

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy link
Copy Markdown
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, 1 unresolved discussion (waiting on @azeey and @EricCousineau-TRI)


composition/merge_proposal.md line 7 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should we replace "Draft" with "Accepted" / "Implemented"?

Done


composition/merge_proposal.md line 32 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can we provide mention of "likeness" to Python? e.g.

It may be helpful to image this as Python module imports.
With a normal include, a model include is similar to an import such as
`import my_model`. If you rename, then it is like `import my_model as renamed_model`.
With a merge include, it's effectively the same as `from my_model import *`.

(from slides on Thurs)

I paraphrased this slightly in 6c9c8d8


composition/merge_proposal.md line 176 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you ensure that this is pinned to particular version / revision? e.g.
https://app.gazebosim.org/OpenRobotics/fuel/models/MARBLE_HUSKY_SENSOR_CONFIG_3/10

Done


composition/merge_proposal.md line 428 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should we put "(Unused)" as the body text here?

Done

Copy link
Copy Markdown
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.

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

@scpeters scpeters changed the title Merging for Composition: Proposal part 2 Merging for Composition: update proposal May 19, 2022
Copy link
Copy Markdown
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 1 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @scpeters)

@scpeters scpeters changed the title Merging for Composition: update proposal Merging for Composition: publish proposal May 20, 2022
@scpeters scpeters merged commit 2c277f9 into master May 20, 2022
@scpeters scpeters deleted the scpeters/feature-include-merge branch May 20, 2022 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants