composition/proposal: Expand on interface-type API #3
Conversation
|
I've fleshed out the prototype a bit more. @scpeters @azeey Can you look at this from a |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
composition/proposal.md, line 554 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I make a comment.
I reject your comment.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
composition/proposal.md, line 577 at r1 (raw file):
~~~c++ class Model : private InterfaceModel { // ... ???
Per Addisu's comment, should instead consider composition + synchronization.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
composition/proposal.md, line 577 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per Addisu's comment, should instead consider composition + synchronization.
Alternatively, this is useless.
Per f2f with Addisu, given prototype in Drake, may not be necessary to try the "merge" the topologies via libsdformat.
Instead, should just rely on downstream code doing the assembly, esp. given that sub-models are encapsulated.
|
Updated! |
composition/proposal.md
Outdated
| // Use private inheritance to hide mutation methods. | ||
| }; | ||
|
|
||
| class Link : private InterfaceLink { ... }; |
There was a problem hiding this comment.
I don't think you've defined InterfaceLink
|
oh I forgot about reviewable |
|
Hm... Lemme see if I can respond to GitHub comments on Reviewable |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
composition/proposal.md, line 546 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
is this the content of the
<uri>element?
Yup! Will update.
composition/proposal.md, line 575 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
If the parent frame is specified as "",
it's not clear to me what "parent frame" means here; please clarify what "parent frame" means and also how and where it is specified, since this constructor only has
nameandSemanticPosearguments
Sorry, meant attached_to frame. Will fix.
composition/proposal.md, line 593 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
what type of string is passed to this function? is it the contents of the model file? it's not the file extension, right?
a doc-string for this type would help clarify things
My initial though was filename, but if it's coming over the write, it could possibly handle just contents, with a suggestion that the checking heuristic should be quick?
composition/proposal.md, line 613 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
I don't think you've defined
InterfaceLink
Er, I think I actually removed it. Will see if I can update it here.
|
Will re-review, and make sure I explicitly tag people when this is ready to review again. |
EricCousineau-TRI
force-pushed
the
feature-composition-interface-api
branch
from
0a95fe5 to
14a2cb4
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @scpeters)
composition/proposal.md, line 546 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yup! Will update.
Done.
composition/proposal.md, line 575 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Sorry, meant
attached_toframe. Will fix.
Done. (Moved up in doc too.)
composition/proposal.md, line 585 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
hte,incorprated
Done. Oops!
composition/proposal.md, line 593 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
My initial though was filename, but if it's coming over the write, it could possibly handle just contents, with a suggestion that the checking heuristic should be quick?
Done. I think filename predicate is the right way, but I actually think that's redundant if a custom parser can opt-out. Made that change.
composition/proposal.md, line 613 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Er, I think I actually removed it. Will see if I can update it here.
Done.
composition/proposal.md, line 663 at r3 (raw file):
**Alternatives Considered**: * Rather than use `sdf::FileNamePredicate`, it was considered to use something
FYI @sloretz @IanTheEngineer Given what we discussed today about registration and stuff.
These are my thoughts on the SDFormat spin on it.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI)
composition/proposal.md, line 571 at r3 (raw file):
/// \param[in] name The *local* name. /// \param[in] pose The semantic pose of the frame. /// If the attached_to frame is specified as "", then this will be a
Being able to reference the attached_to frame from a SemanticPose will require some changes, since the SemanticPose object in libsdformat9 only has a pointer to the PoseRelativeToGraph; it doesn't currently have any access to the FrameAttachedToGraph
- https://github.com/osrf/sdformat/blob/sdformat9_9.2.0/include/sdf/SemanticPose.hh#L86
- https://github.com/osrf/sdformat/blob/sdformat9_9.2.0/src/SemanticPose.cc#L43
So we could either modify the SemanticPose object in libsdformat11 or pass the attached_to name as a separate argument to this constructor. Currently the attached_tobody can only be resolved with Frame::ResolveAttachedToBody
composition/proposal.md, line 595 at r3 (raw file):
std::string name, sdf::InterfaceFramePtr model_frame = nullptr, sdf::InterfaceFramePtr canonical_link_frame = nullptr);
nit: the order of parameters doesn't match the doxygen order
composition/proposal.md, line 622 at r3 (raw file):
/// /// Custom model parsers are *never* checked if resolved file extension ends /// with `*.sdf`.
it is also common for sdf world files to have the extension .world, but I guess that's not relevant to a model parser
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 571 at r3 (raw file):
Previously, azeey (Addisu Taddese) wrote…
(FTR, if it's a separate defect, could you start a separate discussion?)
Yeah, I should have done that.
Done.
composition/proposal.md, line 577 at r9 (raw file):
Previously, azeey (Addisu Taddese) wrote…
custom interpretations may conflict with what SDFormat expects.
Good point. I wouldn't want the custom parser to reinterpret what libsdformat has already parsed.
Could it instead provide a list of custom sdf::ElementPtr?
Yeah, we can maybe wrap all the custom elements inside a new
Elementand pass that
Done. (Added some thing about Element)
composition/proposal.md, line 603 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Good point! This could specify the pose of the model frame relative to the canonical link.
Will update this.
Done. (Simplified model frame specification - in a slightly less redundant way)
composition/proposal.md, line 628 at r9 (raw file):
Previously, azeey (Addisu Taddese) wrote…
GetFrames->GetLinks
Done. Oops!
composition/proposal.md, line 719 at r9 (raw file):
Previously, scpeters (Steve Peters) wrote…
ineterfaceinterface
Done.
…eature-composition-interface-api
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 542 at r12 (raw file):
In order to do so, it is proposed that the following API hook be permitted to be registered in `libsdformat`. This is described in the following pseudocode, along with a specification of the proposed contract for custom parsers:
Will add mention of prototype PR, with a TODO that it should be replaced with real support in Drake.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @azeey and @scpeters)
a discussion (no related file):
Will rebase pending merge.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 542 at r12 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Will add mention of prototype PR, with a TODO that it should be replaced with real support in Drake.
Done. (See new section, 1.5.2, added in r12).
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
composition/proposal.md, line 1174 at r13 (raw file):
#### Short-form modifications for Interface API *TODO(eric.cousineau): Factor into above steps.*
Working on this now.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 1174 at r13 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working on this now.
OK Er, nvm. I would prefer to just get this merged in, then iterate in the future.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r14.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
composition/proposal.md, line 559 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Sounds good! I think my code-as is addresses that? (see the docstring for
uri)
Yup. Just to be sure, is the resolved_file_name an absolute path to the file or just the name of the file?
composition/proposal.md, line 603 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done. (Simplified model frame specification - in a slightly less redundant way)
I'm not sure where we're capturing a model's pose relative to its parent frame here. In SDFormat, for example, if I have a model
<model name="M1">
<pose>10 0 0 0 0 0</pose>
<link name="L1"/>
</model>and include it in another model
<model name="M2">
<include>
<uri>path/to/M1</uri>
</include>
</model>The pose X_M2M1 would be 10 0 0 0 0 0. If M1 was parsed via the Interface Elements API, I don't see how that pose makes its way to libsdformat.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 603 at r9 (raw file):
Previously, azeey (Addisu Taddese) wrote…
I'm not sure where we're capturing a model's pose relative to its parent frame here. In SDFormat, for example, if I have a model
<model name="M1"> <pose>10 0 0 0 0 0</pose> <link name="L1"/> </model>and include it in another model
<model name="M2"> <include> <uri>path/to/M1</uri> </include> </model>The pose X_M2M1 would be
10 0 0 0 0 0. If M1 was parsed via the Interface Elements API, I don't see how that pose makes its way to libsdformat.
In r14, when constructing an InterfaceModel, the user provides both canonical_link_name and X_LcM (pose of model frame w.r.t. canonical link's frame). These are accessed via GetCanonicalLinkName and GetModelFramePoseInCanonicalLinkFrame.
GetModelFramePoseInCanonicalLinkFrame() would encode X_M2M1, so I think we're good?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
composition/proposal.md, line 603 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
In
r14, when constructing anInterfaceModel, the user provides bothcanonical_link_nameandX_LcM(pose of model frame w.r.t. canonical link's frame). These are accessed viaGetCanonicalLinkNameandGetModelFramePoseInCanonicalLinkFrame.
GetModelFramePoseInCanonicalLinkFrame()would encodeX_M2M1, so I think we're good?
In this example, wouldn't X_LcM be zero? Unless the canonical link you're referring to is the canonical link of the parent model.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 603 at r9 (raw file):
Previously, azeey (Addisu Taddese) wrote…
In this example, wouldn't
X_LcMbe zero? Unless the canonical link you're referring to is the canonical link of the parent model.
Per Slack VC, X_LcM is encoded via //model[@name="M1"]/link[@name="L1"]/pose, not via //model/pose. As Addisu wrote above (that I didn't realize), //model/pose is actually something like X_MpM, where Mp is the parent Model.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
composition/proposal.md, line 603 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per Slack VC,
X_LcMis encoded via//model[@name="M1"]/link[@name="L1"]/pose, not via//model/pose. As Addisu wrote above (that I didn't realize),//model/poseis actually something likeX_MpM, whereMpis the parent Model.
I thought //model[@name="M1"]/link[@name="L1"]/pose represented X_MLc, the inverse of X_LcM, the pose of the canonical link's frame relative to the model frame
composition/proposal.md, line 639 at r14 (raw file):
/// the declared links. public: void AddLink(sdf::InterfaceLink link); /// Gets registered frames.
Gets registered links.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
composition/proposal.md, line 603 at r9 (raw file):
Previously, scpeters (Steve Peters) wrote…
I thought
//model[@name="M1"]/link[@name="L1"]/poserepresentedX_MLc, the inverse ofX_LcM, the pose of the canonical link's frame relative to the model frame
Done - updated. And yup, Steve, you're right! I was just saying that the info is there indirectly (but worded poorly).
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
composition/proposal.md, line 559 at r9 (raw file):
Previously, azeey (Addisu Taddese) wrote…
Yup. Just to be sure, is the
resolved_file_namean absolute path to the file or just the name of the file?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r16.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @azeey and @scpeters)
composition/proposal.md, line 639 at r14 (raw file):
Previously, scpeters (Steve Peters) wrote…
Gets registered links.
Done. Oops!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scpeters)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17.
Reviewable status: all files reviewed, 1 unresolved discussion
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Will rebase pending merge.
OK Will use Squash + Merge.
For more background, see existing proposal:
https://github.com/osrf/sdf_tutorials/blob/251b28804a6e79ac1b89001b8fd0036bc07e0d7d/composition/proposal.md
Basically, this interface API is to allow included models to either (a) be parsed by
libsdformat(and thus must be completely understood) or (b) be parsed by the downstream library, with the absolute minimum information passed back tolibsdformatfor links and frames (for posturing and connecting joints).Motivational WIP usage of phantom API:
RobotLocomotion/drake#13128
(Branch ported from BitBucket)
This change is