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.

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?

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.

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

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

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)

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

9c6325f in #74

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

ee994b1 in #74

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

ee994b1 in #74

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

f4e05a7 in #74