Joint default values need to be stored in Joint

[joemasterjohn]

I'm working on parsing the <joint><axis><initial_position> tag to set the default angle for a RevoluteJoint and a PrismaticJoint in multibody/parsing/detail_sdf_parser.cc. Here is my branch (currently only parsing RevoluteJoint):

https://github.com/joemasterjohn/drake/tree/parsing_joint_initial_position

Right now RevoluteJoint and PrismaticJoint are deferring ownership of the their default positions (RevoluteJoint::set_default_angle() and PrismaticJoint::set_default_translation() respectively) to their Mobilizer. This disallows setting the initial position prior to the parent MultibodyTree being finalized because of a precondition in get_mutable_mobilizer(). I believe the default position should live in the Joint class. Thoughts?

[joemasterjohn]

\cc @amcastro-tri

[sherm1]

I agree, @joemasterjohn. All the parsing info ought to reside in the Joint so we can create and destroy the mobilizer graph as an afterthought.

[EricCousineau-TRI]

BTW This isn't our policy per se, but I do like to title issues that make it obvious they're an issue, not a PR (e.g. "Joint default values need to be deferred to mobilizer" or "Should make [...]").

[EricCousineau-TRI]

As @jwnimmer-tri pointed out, my commented here may make this issue moot: https://github.com/RobotLocomotion/drake/pull/13105#issuecomment-616814619

Can you edit this to remove the reference to initial_position? It will be deprecated when SDFormat 1.8 comes out: https://github.com/osrf/sdformat/blame/master/Changelog.md#L13 @scpeters, @azeey, and I discussed it (offline, unfortunately), and feel that is redundant to storing some level of state (e.g. in some sort of //state tag).

For more info, see PR link in changelog.

Sorry for not reading this more fully!

[EricCousineau-TRI]

FTR, it was deprecated as it was (a) redundant w.r.t. //world/state and (b) not used at all in Gazebo (or maybe anything else?).

Here is the spec for //world/state in SDFormat 1.7: http://sdformat.org/spec?ver=1.7&elem=state

Perhaps you can instruct detail_sdf_parser.cc how to recognize this and parse a subset (e.g. /state/model/joint/angle), but it may not be uber straightforward - also, it does not look like spec is super straightforward (e.g. only angle, even for prismatic or multi-DoF joints? it may be tailored only for maximal coordiantes, not minimal coordiantes?).

FYI @scpeters @azeey

[EricCousineau-TRI]

Basically, I think this issue should maybe be reworded to "Should teach SDFormat parser to read //world/state". (and state should ideally not be coupled to a given model?)

[sherm1]

//world/state looks like a mechanism for recording a trajectory -- that doesn't seem to cover the same territory as setting the default value for a joint's coordinates. (For example it wants simulation and real time stamps in nanoseconds!) It also records output values like acceleration which are not state at all, but rather simulation results.

initial_position seems orthogonal to //world/state. Can you explain why you think the latter is sufficient?

[EricCousineau-TRI]

I don't see anything in it that looks like it records a trajectory. It just asks for time as the state.

And FTR I'm not necessarily saying all of //world/state is best suited for our needs (e.g. maximal coordinates, non-state things like acceleration, real time), but I like the general intent - to record the state of the world, rather than try and couple it into a model's definition (not instantiation) like you would (but shouldn't?) do with //model/joint/axis/initial_position.

We can always take a subset of the element that works best for us, and then work on refining that element with OSRC (e.g. make quantities coupled to dynamics engines be tagged as such).

[sherm1]

See my comment in #13105. I do think the model definition should include initial_position when that is a property of the model rather than just an arbitrary setting. That occurs much more often with closed topology systems than with tree-structured ones.

[EricCousineau-TRI]

@sherm1 I'm all for abstracting away the zero configuration such that q==0. makes physical sense. However, to me that seems to be a separate item from specifying a initial configuration (e.g. q_0 which may not be 0 when you initialize the state).

Which are y'all advocating for the most?

To me, zero configuration may need set_my_magical_bias_thinger (but I am completely on board with specifying this at the model level), whereas initial configuration is in line with reaching SetDefaultState (but I'm not a fan as I feel that it may be an abuse of definition and redundant w.r.t. something global like //world/state).

BTW Is this the real root motivation for this issue? If so, would be super nice to have that stated in the overview!

[sherm1]

Is this the real root motivation for this issue?

I'm not sure which of things above you're referring to? My motivation would be to associate a reasonable non-zero default value for a joint's coordinates if that makes sense for the model (as opposed to just something application dependent). So SetDefaultState() would not necessarily set all the joint coords to zero. An app can always override that, the point is to have a reasonable default, and zero is not always reasonable. For closed topology systems, zero is often the "flatline" configuration from which it is nearly impossible to compute an assembled configuration.

[EricCousineau-TRI]

Ah, I see. Yes, if the default value is explicitly intended to be for the aspect of a model, and not be confused with trying to capture the model's state, then yeah, I guess I can see this purpose. I was initially thrown off by the mention of initial_position, which is a rather ambiguous name, and seems redundant w.r.t. //world/state.

My suggestion is to make this Drake-specific for now; do not use SDFormat's effectively unsupported (and soon-to-be-deprecated) //joint/axis/initial_position. Instead, define <drake:default_value/> (or whatever is more appropriately named for the joint).

I'm not sure which of things above you're referring to?

I was referring to the intended application of using the default value for use with closed-loop topologies. At first, since the intent was to use initial_position, I was not sure if it was an attempt to set initial conditions for a simulation (I very much want to make sure that this is not what the property is used for!).

[sherm1]

Gotcha, yes that makes sense. drake::default_position or something like that would be a better name.

[amcastro-tri]

This is a great discussion, thank you guys @sherm1 and @EricCousineau-TRI. What about something like this? which would allow in the future to also specify velocities without having to deprecate parsing tags.

<joint name = ....>
<drake:default_state>
  <!--As many entries as dofs per joint. -->
  <drake:position> 1.0 2.0 </drake:position>
  <drake:velocity> 1.0 2.0 </drake:velocity> <!-- For some future. -->
</drake:default_state>
</joint>

[joemasterjohn]

Yes, thanks @sherm1, @EricCousineau-TRI and @amcastro-tri . Sorry I'm late to the party and didn't make it clear that the intention for parsing the <initial_position> tag was to store a default configuration of a closed-loop topology where the zero configuration doesn't make much sense.

I'll try to make the issue and related PR(#13105) agnostic to the particular tag the parser would actually use.

[EricCousineau-TRI]

@joemasterjohn Sweet! That sounds great!

@amcastro-tri Er, can you explain more we why we'd want a non-zero default velocity? Non-zero default position makes sense in light of Sherm's explanation, but I'm not yet onboard with a non-zero default velocity - it sounds like it's creeping towards initial conditions, not accommodating model-specific attributes.

[amcastro-tri]

That's actually a good point @EricCousineau-TRI. I was trying to avoid some future deprecation. What do you think @sherm1 ?

[sherm1]

Yes, I was going to make the same point but @EricCousineau-TRI beat me to it. A recommended initial configuration makes sense as part of the modeler's work, but a non-zero initial velocity would be up to the particular application using that model.

[amcastro-tri]

ok, thanks for the clarification (sry for the close/open spam, github crushed on me this morning and I followed up by pressing the wrong buttons).

Then we'd like something like?:

<joint name = ....>
<drake:default_position> 1.0 2.0 </drake:default_position>
</joint>

[jwnimmer-tri]

If we agree that it's semantically valid for the xml to store a non-zero default position per the modeler, then I'm unclear on why http://sdformat.org/spec?ver=1.7&elem=joint#axis_initial_position is going to be deprecated, @EricCousineau-TRI? Can we just use the standard element and undo the deprecation? Or do we really want to highlight "initial" vs "default" somehow being different?

[EricCousineau-TRI]

I would like to converge on terminology that makes the purpose clear, so yes, I want to keep the deprecation.

[EricCousineau-TRI]

That being said, I will still discuss with @scpeters and @azeey on Thursday to see what they think.

[EricCousineau-TRI]

On @amcastro-tri's point, I don't have a concrete suggestion on the syntax. From reviewing the current docs, we don't have long-form example of what multi-axis joints look like: http://sdformat.org/tutorials?tut=spec_model_kinematics&cat=specification&#joint

So will have that in the Thurs meeting. @amcastro-tri @joemasterjohn Would y'all (or one of you) want to join the meeting to discuss this?

[joemasterjohn]

@EricCousineau-TRI sure thing, you can go ahead and invite me.

[EricCousineau-TRI]

Done!

[RussTedrake]

Seems like this could have been closed via #13105?

[RussTedrake]

Or perhaps the issue has taken on more meaning that the issue name suggests, and should be renamed to "set default joint positions via sdf" or something more up to date?

[RussTedrake]

@joemasterjohn -- can you update or close this issue (per my comment just above)?

[jwnimmer-tri]

The discussions around #15948 are somewhat related here. Now that model directives can specify default joint positions as of #17802, we will eventually need Drake's SDFormat parser to reach feature parity on that front so that our DMD->SDF conversion tool has a way to convert that spelling.

As such, we'll need some XML tag in our SDFormat parser for the initial joint position. That could be a custom tag like <drake:default_position> as discussed above, but ideally I would rather have us advocate for an upstream SDFormat-specified tag in this case, since this is an important feature that is not otherwise expressible in stock SDFormat -- in other words, this is not merely sugar.

If that process it too slow, we can start with the drake-custom tag for now, but I doesn't seem like it should be difficult to do it correctly from the get-go.

[jwnimmer-tri]

I would like to converge on terminology that makes the purpose clear, so yes, I want to keep the deprecation.

I think we need to get out in front of this. I assume it will take a little while to adjust SDFormat, prior to Drake being able to use the new spelling.

SDFormat needs a spec upgrade that defines your new preferred spelling (e.g., default_position), and ideally its 1_N.convert automatic converter recipe should respell initial_position to default_position automatically for people (when their file identifies as an earlier sdformat version where initial_position was available).

Is there a tracking issue (feature request) anywhere on the SDFormat side yet? I was not able to find one.

[jwnimmer-tri]

See also related discussion in #19230.