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: Should implement //include/placement_frame #319

Closed
EricCousineau-TRI opened this issue Jul 23, 2020 · 10 comments · Fixed by #324
Closed

composition: Should implement //include/placement_frame #319

EricCousineau-TRI opened this issue Jul 23, 2020 · 10 comments · Fixed by #324
Assignees

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Jul 23, 2020

Per proposal:
https://github.com/osrf/sdf_tutorials/blob/044297cf20b0e50cb9ed81c811355363eb393be4/composition/proposal.md#144-placement-frame-includeplacement_frame

Towards #278

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Jul 23, 2020

Per f2f, @azeey knows of two options to implement with current frame pose graph:

  1. just update the RawPose() for the included model, given the placement frame, include pose, and model frame pose
  2. connect the nested model's frame pose subgraph directly via its placement frame, not via the model frame. However, this means the model frame has no parent, and the placement frame has two frames, which violates the tree invariant (by the consumer) of the frame graph.

Easiest option is (1), by far.

@scpeters
Copy link
Member

Per f2f, @azeey knows of two options to implement with current frame pose graph:

  1. just update the RawPose() for the included model, given the placement frame, include pose, and model frame pose
  2. connect the nested model's frame pose subgraph directly via its placement frame, not via the model frame. However, this means the model frame has no parent, and the placement frame has two frames, which violates the tree invariant (by the consumer) of the frame graph.

Easiest option is (1), by far.

yeah, option (1) seems much simpler to me

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Jul 23, 2020

Aye. Also, this works because our "instantiation of included models" is to just include the model in-place (e.g. if you include model a.sdf three times, we'll parse it three times, so mutation is fine).
If we ever cache parsed models, then we can just do a deep-copy when instantiating.

@azeey
Copy link
Collaborator

azeey commented Jul 24, 2020

This is actually less straightforward than I initially thought. I was thinking of doing (1) in the parser where //include is handled, but we can't do that there because the relative_to graph is not available at that stage. We need that graph in order to resolve the pose of the specified placement frame relative to its immediate parent model. This means we'll have to inject the placement_frame information into the nested model somehow so that it can be used later on in the parsing stages.

@scpeters
Copy link
Member

scpeters commented Jul 24, 2020

inject it as //model/@placement_frame? I'm not sure if that would work...

@scpeters
Copy link
Member

Is there a reason to reserve this functionality for //included models? If we added //model/@placement_frame and supported this for all models, it would simplify the parsing

@EricCousineau-TRI
Copy link
Collaborator Author

The only ephemeral reason I'd not like //model/@placement_frame is that the same effect could be achieved by posturing your canonical link w.r.t. the model frame... however, that is a bit awkward.

So yeah, I think //model/@placement_frame could fit directly inside the model.

By default, it's __model__, but then it can be overridden.
It won't physically change the pose relationship between __model__ (M) and the model's contained frames; it'll only be used for (re)posturing in the parent when //model/pose (X_WM or X_MpM) is consumed.

Gosh, that sounds a tad complicated writing it out... but I guess it'll work!

@azeey
Copy link
Collaborator

azeey commented Jul 28, 2020

Injecting it as //model/@placement_frame sounds good, but it won't work until we tackle making included models work as nested models (#284) because right now, the parser merges the contents of the child model into the parent model. The //model/@placement_frame of the child model won't be preserved.

I was thinking of injecting a //model/frame[@name=<model_name>__placement_frame__ attached to the specified placement frame during parsing and using it during Model::Load to update the <model_name>::__model__ frame.

@EricCousineau-TRI
Copy link
Collaborator Author

I'd be OK with the workaround as long as the interim hack / scaffolding were removed once #284 lands.
Could we condition this issue to close only once the scaffolding is removed?

@scpeters
Copy link
Member

scpeters commented Aug 4, 2020

I'd be OK with the workaround as long as the interim hack / scaffolding were removed once #284 lands.
Could we condition this issue to close only once the scaffolding is removed?

sounds good to me

@azeey azeey closed this as completed in #324 Aug 7, 2020
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 a pull request may close this issue.

3 participants