Merging for Composition: Proposed behavior

[EricCousineau-TRI]

🎉 New feature

Merging for Composition: Proposed behavior

Towards gazebosim/sdformat#658
Draft implementation: gazebosim/sdformat#659

Summary

See proposal text.

Test it

See preview: via sdformat.org

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• ~ codecheck passed~
• All tests passed
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

---

This change is 

[EricCousineau-TRI]

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

---

composition/merge_proposal.md, line 54 at r1 (raw file):

## Proposed changes

TBD

Working: Will fill these out pending draft PR for implementation.

[scpeters]

I've started on this by updating the introduction and adding a bit to the motivation section: cc578e7

[scpeters]

I've updated this quite a bit. The only thing I can think to add still is how to handle placement frames when doing a merge-include. What do you think? @EricCousineau-TRI @azeey

[scpeters]

I've updated this quite a bit. The only thing I can think to add still is how to handle placement frames when doing a merge-include. What do you think? @EricCousineau-TRI @azeey

I've added discussion of the placement frame in 1f663c0

[EricCousineau-TRI]

Nice! , with some small nits.

Do you want to make alt. PR so you have primary authorship? (I'm fine either way)

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @scpeters)

---

composition/merge_proposal.md line 120 at r5 (raw file):

~~~
<?xml version="1.0" ?>

nit Other snippets do not contain this header.

Consider removing here?

---

composition/merge_proposal.md line 225 at r5 (raw file):

      <pose>0.424 0 0.460 0 0 0</pose>
      <!-- Based on Intel realsense D435 (intrinsics and distortion not modeled)-->
      <sensor name="camera_pan_tilt" type="rgbd_camera">

nit Consider removing additional elements for now?

Or perhaps empty out contents and replace with ellipsis?

[azeey]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)

---

composition/merge_proposal.md line 108 at r5 (raw file):

For the entities to be merged, any explicit references to the
implicit `__model__` frame are replaced with references to the proxy frame.
Additionally, the name of the proxy frame is inserted anywhere the is an

nit: anywhere the -> anywhere there

---

composition/merge_proposal.md line 487 at r5 (raw file):

      <parent>base_link</parent>
      <axis>
        <xyz expressed_in="_merged__pan_tilt_sensors_3__model__">0 0 1</xyz>

hmm, it's not ideal to have to reference the proxy frame explicitly. It's more of an implementation detail, I think, and the naming pattern could change. Ideally, we would just reference the name of the included model corresponding to pan_tile_sensor3.sdf here, but I'm not sure that would actually work. I don't want to block this, but it might be good to create an issue for it.

[EricCousineau-TRI]

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

---

composition/merge_proposal.md line 487 at r5 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

hmm, it's not ideal to have to reference the proxy frame explicitly. It's more of an implementation detail, I think, and the naming pattern could change. Ideally, we would just reference the name of the included model corresponding to pan_tile_sensor3.sdf here, but I'm not sure that would actually work. I don't want to block this, but it might be good to create an issue for it.

Per VC, we should actually leave it, but:

• Warn that it's an implementation detail
• Suggest two options if a user needs to explicitly reference the included model frame
  • (a) Make their own explicit alias frame in the included model, e.g. arm_frame
  • (b) Use experimental:params to inject the alias frame via //include

[scpeters]

sure, I'll open a separate pull request so it will make more sense for you and Addisu to review it

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

[scpeters]

see #74

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

---

composition/merge_proposal.md line 108 at r5 (raw file):

Previously, azeey (Addisu Z. Taddese) wrote…

nit: anywhere the -> anywhere there

9c6325f in #74

---

composition/merge_proposal.md line 120 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Other snippets do not contain this header.

Consider removing here?

ee994b1 in #74

---

composition/merge_proposal.md line 225 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Consider removing additional elements for now?

Or perhaps empty out contents and replace with ellipsis?

ee994b1 in #74

---

composition/merge_proposal.md line 487 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Per VC, we should actually leave it, but:

• Warn that it's an implementation detail
• Suggest two options if a user needs to explicitly reference the included model frame
  • (a) Make their own explicit alias frame in the included model, e.g. arm_frame
  • (b) Use experimental:params to inject the alias frame via //include

f4e05a7 in #74

[EricCousineau-TRI]

all changes look good, closing this in lieu of #74