Introduce sdformat semantics for Drake's visual roles #22013
Conversation
There was a problem hiding this comment.
Take a look at the following google doc to see what my considerations were for this design.
Getting a baseline of consensus on something here will be the precursor for the initial prototype implementation.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Working
One XML-pointy-hat question we'll need to answer is whether the elements inside of a drake-namespaced element need to also be drake-namespaced.
The proposal currently says this:
<visual name="visual">
<geometry>
<mesh>
<uri>package://drake/geometry/render/test/meshes/box.obj</uri>
</mesh>
</geometry>
<drake:illustration_properties>
<geometry>
<sphere>
<radius>0.5</size>
</sphere>
</geometry>
</drake:illustration_properties>
</visual>It's possible that for XML legality reasons we might need to say this, insetad:
<visual name="visual">
<geometry>
<mesh>
<uri>package://drake/geometry/render/test/meshes/box.obj</uri>
</mesh>
</geometry>
<drake:illustration_properties>
<drake:geometry>
<drake:sphere>
<drake:radius>0.5</drake:radius>
</drake:sphere>
</drake:geometry>
</drake:illustration_properties>
</visual>
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
One XML-pointy-hat question we'll need to answer is whether the elements inside of a drake-namespaced element need to also be drake-namespaced.
The proposal currently says this:
<visual name="visual"> <geometry> <mesh> <uri>package://drake/geometry/render/test/meshes/box.obj</uri> </mesh> </geometry> <drake:illustration_properties> <geometry> <sphere> <radius>0.5</size> </sphere> </geometry> </drake:illustration_properties> </visual>It's possible that for XML legality reasons we might need to say this, insetad:
<visual name="visual"> <geometry> <mesh> <uri>package://drake/geometry/render/test/meshes/box.obj</uri> </mesh> </geometry> <drake:illustration_properties> <drake:geometry> <drake:sphere> <drake:radius>0.5</drake:radius> </drake:sphere> </drake:geometry> </drake:illustration_properties> </visual>
I was wondering that myself. I hope I can nest an sdformat element under a drake element. But I fear that I can't. :(
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I was wondering that myself. I hope I can nest an sdformat element under a drake element. But I fear that I can't. :(
I believe we have past examples in Drake where (conservatively) all sub-tags are drake-namespaced. I can't yet quote chapter and verse, but I don't think it's strictly necessary.
"From my POV as some random parser, all the crap inside the drake:blah tag is of unknown structure and semantics. It needs to be XML, so I can skip over it, but other than that, I can't really make any other claims on it."
If we decide to drake-namespace the nested tags, I don't think it would be a large increment of more work, but I don't know that we need to.
Having to wrangle this into the lovely half-sdformatlib-half-hobby-project sdformat parser will prove to be fun, either way 😉
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
I believe we have past examples in Drake where (conservatively) all sub-tags are drake-namespaced. I can't yet quote chapter and verse, but I don't think it's strictly necessary.
"From my POV as some random parser, all the crap inside the drake:blah tag is of unknown structure and semantics. It needs to be XML, so I can skip over it, but other than that, I can't really make any other claims on it."
If we decide to drake-namespace the nested tags, I don't think it would be a large increment of more work, but I don't know that we need to.
Having to wrangle this into the lovely half-sdformatlib-half-hobby-project sdformat parser will prove to be fun, either way 😉
We can certainly make a whole family of drake: prefixed instances and say they have the same semantics in Drake that the sdformat versions do.
However, that pales in comparison to what I was hoping to achieve (which was, at best naive and, at worst, blinkered). I had hoped that somehow we could leverage the existing parsing functionality. But, presumably, libsdformat is going to stop parsing the XML tree at <drake:foo_properties>. Whatever the elements below that element are called, libsdformat will never see them. That means for every tag we support, we'll have to reinvent the libsdformat parsing semantics.
With any luck, we can take it and trampoline into sdformat logic and not actually reinvent it. But, what a nuisance. Failing that, it's going to introduce an unfortunate amount of code. (URDF will be far more benign as it's just Drake's XML parsing and Drake doesn't care of the <geometry> tag has <visual> as a parent or <drake:perception_properties>. Definitely a Pyrrhic victory there.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
<geometry> <sphere> <radius>0.5</size>
Suggestion:
radius
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
<geometry> <sphere> <radius>0.5</size>
FWIW,
$ ./bazel-bin/multibody/parsing/parser_manual_test ./geometry/render/test/configure_visual_roles.sdf
I wouldn't bother trying to make the package:// link resolve.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
FWIW,
$ ./bazel-bin/multibody/parsing/parser_manual_test ./geometry/render/test/configure_visual_roles.sdfI wouldn't bother trying to make the package:// link resolve.
TIL Thanks!
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
TIL Thanks!
I'm a little concerned that the parser didn't whine about unknown drake: tags, but that's a worry for another day.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
I'm a little concerned that the parser didn't whine about unknown
drake:tags, but that's a worry for another day.
True, ideally, it should eat or complain about everything, even if it only complains about the root of a subtree. It should still complain.
This introduces a working example of an sdf file.
SeanCurtis-TRI
force-pushed
the
PR_specify_visual_role_in_model_files
branch
from
d04b4e9
to
b225b12
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 92 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
True, ideally, it should eat or complain about everything, even if it only complains about the root of a subtree. It should still complain.
nevermind, it did complain. The crashing-out from bad package:// link was obscuring the later complaint.
|
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Well, that's good. :) |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
With the understanding that everything below <drake:foo_properties> must be drake: namespaced, I'll go ahead and see what an implementation of this protocol looks like.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
|
I am still reviewing. I am not sure that |
There was a problem hiding this comment.
Okay, I found the time to read the background info and re-review this draft. There's just one big-picture question below -- I don't understand the motivation (i.e., upside) for nesting shapes inside of properties. (We can discuss it in that thread.)
Reviewable status: 6 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/test/configure_visual_roles.sdf line 8 at r2 (raw file):
<link name="both_roles_by_default"> <visual name="visual"> <!-- The mesh geometry only has illustration properties. -->
nit Is this comment mis-placed? It seems to me like the mesh geometry here has both illustration properties and perception properties.
geometry/render/test/configure_visual_roles.sdf line 20 at r2 (raw file):
<link name="both_roles_with_redundant_specification"> <visual name="visual"> <!-- The mesh geometry only has illustration properties. -->
nit Ditto. ((Is this comment mis-placed? It seems to me like the mesh geometry here has both illustration properties and perception properties.))
geometry/render/test/configure_visual_roles.sdf line 63 at r2 (raw file):
<link name="different_colors"> <visual name="visual"> <!-- The box geometry has different colors based on role. -->
... and in non-Drake loaders, the color would be white? Maybe worth pointing that out here specifically in the comment.
geometry/render/test/configure_visual_roles.sdf line 90 at r2 (raw file):
<drake:illustration_properties> <pose>1 0 0 0 0 0</pose> <geometry>
I do not understand the motivation for putting replacement geometries inside of the foo_properties elements? Nothing in the document seemed to motivate that choice.
Was it for ease of implementation? I don't think we should allow that to affect the design. Implementations will be the normal amount of xml hellishness no matter what we do for the spec. We should choose the best syntax for users.
Also, the current proposal for nesting geometry doesn't work, anyway. We can't re-use the visual name=... for two different shapes, because every shape must have a unique name. (Maybe there is a slightly different proposal for nesting that solves that wrinkle?)
Anyway, for me, having all geometry at the same level of nesting would be vastly more clear for users.
<!-- The perception geometry is a mesh. -->
<visual name="perception">
<geometry>
<mesh>
<uri>package://drake/geometry/render/test/meshes/box.obj</uri>
</mesh>
<drake:illustration_properties enabled="false" />
<drake:perception_properties enabled="true" />
</geometry>
</visual>
<!-- The illustration geometry is a sphere. -->
<drake:visual name="illustration">
<drake:pose>1 0 0 0 0 0</drake:pose>
<drake:geometry>
<drake:sphere>
<drake:radius>0.5</drake:radius>
</drake:sphere>
<drake:illustration_properties enabled="true" />
<drake:perception_properties enabled="false" />
</drake:geometry>
</drake:visual>The idea in the above is that <drake:visual> (and <drake:geometry> and etc.) would have exactly the same semantics as their non-drake spellings, except maybe that only a subset of features are supported (e.g., no "drake:laser_retro"). The user only needs to learn two new things for this feature:
- The
drake:visualelement is only loaded by Drake, and ignored elsewhere. - The
drake:foo_propertieselements customize the geometry properties for a geometry (including opting out of the role entirely).
WDYT?
BTW There could be another proposal something like the below (where only geometry but not visual is twinned into a drake: element), but I think specifying its semantics for users to understand probably comes out worse.
<visual name="perception">
<geometry drake:role="perception">
</geometry>
<drake:geometry role="illustration">
</drake:geometry>
</visual>
geometry/render/test/configure_visual_roles.sdf line 130 at r2 (raw file):
This gets a warning.
This is actually an error. In SDFormat, the visual element must have exactly one geometry child, or else the file is malformed.
This introduces a working example of an sdf file.
Relates: #13689
This change is