Drake: Un-intuitive behavior of AddModelFromFile and WeldFrames when <static> tag is included in sdf

[quanvuong]

I added a gist to demo the behavior in the link below.

https://gist.github.com/quanvuong/c11c96dd51e3a62195073cdf2beb4442

visualize.py fails with:

RuntimeError: AddJoint(): Duplicate joint name 'WorldBody_welds_to_box_link'.

I guess this is because a static tag is included in the sdf file, and a WeldJoint is added when the sdf is added into the plant. However, there's no error or warning and it was hard to trace the error back to this. If you remove the tag, the code should execute ok.

[rpoyner-tri]

Taking an initial look.

[rpoyner-tri]

https://github.com/rpoyner-tri/drake/tree/debug14518 reproduces the original complaint, as a throwaway branch for further investigation.

[rpoyner-tri]

Possibly related: closed drake issue #12227.

[rpoyner-tri]

Paging @EricCousineau-TRI for expertise on whether the current implementation of //model/static makes sense.

[rpoyner-tri]

/cc @joemasterjohn since it could be that a solution to this may intersect with remodeling efforts.

[joemasterjohn]

Drake's parsing behavior as it stands is to weld all links to the world for any model marked static. The problem in the example is that each link is being procedurally welded to the world after parsing. The fix would be to remove those lines in the for loop.

I believe Drake's behavior to be correct, but maybe the error message is un-intuitive. MBP::WeldFrames is just a wrapper for AddJoint<WeldJoint> so the error propagates up from there. Perhaps modifying WeldFrames to check that the frames passed in haven't already been welded would be the best course of action.

[rpoyner-tri]

@joemasterjohn that is a possible fix, but conflicts with @EricCousineau-TRI 's original design, per this comment .

Another possibility is to issue some warning from the parser when static is used, since the current "fail-fast" implementation seems to defy user intuition.

[rpoyner-tri]

Summary of f2f with @joemasterjohn:

• the existing sdf parser implementation of static produces hidden/un-intuitive topology
  
  
  
  • this differs from other systems' implementation, which is more like a "get out of physics free" card than weld joints to the world
  
  
• however, apparently, Drake reserves the right to select topology details for various practical reasons (more detail here, @joemasterjohn ?)
• the "parse then pose" workflow is possible now thanks to recent work on parameters
  
  
  
  • but maybe this is not obvious since it is new API workflow
  
  

Questions:

• SDF parser details -- where/how are they documented?
• There is a possible workflow. Is it the one we want? Is it adequately documented?

Did I forget anything @joemasterjohn ?

[quanvuong]

I just wanted to say that drake is great and has helped me a lot in my own work. The attention and care you put into my issue really demonstrates why drake is so great :+1:

[EricCousineau-TRI]

@quanvuong Rico and Joe's assessments are correct, and I think what you mentioned here provides for the best course for correction:

However, there's no error or warning and it was hard to trace the error back to this.

I think the best solution would be to make it more obvious that the weld joint comes from //model/static tag (e.g. naming the joint as {name}_sdformat_model_static).
If we wanna go overboard, we can check this situation in MultibodyPlant::AddWeld or even AddJoint and warn about this specifically, and perhaps refer to the workaround:
https://drake.mit.edu/release_notes/v0.12.0.html#breaking-changes-since-v0-11-0

More acutely, Quan, in your case, I think the better solution is to remove the <static/> tag if you haven't already?

---

SDF parser details -- where/how are they documented?

Pending per #13103
As is my normal excuse, I probably won't be able to do this any time soon :(

There is a possible workflow. Is it the one we want? Is it adequately documented?

Only in release notes at the moment. See above statements about Quan's acute pain point.

this differs from other systems' implementation [...]

"system" is a tad ambiguous in this context :P Can you specify? (e.g. other physics engines, like ODE?)

[rpoyner-tri]

I like the elaborate weld joint name as a resolution for now. It gives a decent clue, and is neither noisy nor weirdly out of band. I can try to roll a PR for that.

Longer term, probably the SDF Parser could be taught to use the forthcoming joint locking feature. This would minimize the amount of made-up topology and perhaps be a bit more comprehensible.

re: "system" from above -- ambiguous on purpose :) . I suppose physics engine or simulator was what I meant. I was going to look into the bullet and other implementations mentioned in #12227, but the code links were stale, and I didn't dig further.

[rpoyner-tri]

Still need to write a proper test to have a complete PR, but the error message in the https://github.com/rpoyner-tri/drake/tree/debug14518 branch i starting to look like this:
rico@Puget-161804-10:~/checkout/drake$ bazel run //temp:visualize INFO: Invocation ID: 3f712fd3-e10d-441f-8964-8e02434f7475 INFO: Analyzed target //temp:visualize (4 packages loaded, 618 targets configured). INFO: Found 1 target... Target //temp:visualize up-to-date: bazel-bin/temp/visualize INFO: Elapsed time: 111.667s, Critical Path: 71.26s INFO: 222 processes: 144 remote cache hit, 9 internal, 69 linux-sandbox. INFO: Build completed successfully, 222 total actions INFO: Build completed successfully, 222 total actions Number of frame to draw 5 Traceback (most recent call last): File "/home/rico/.cache/bazel/_bazel_rico/7f8997f28c9253517a55d673c67a6c74/execroot/drake/bazel-out/k8-opt/bin/temp/visualize.runfiles/drake/temp/visualize.py", line 61, in <module> visualize_gripper_frames(transforms) File "/home/rico/.cache/bazel/_bazel_rico/7f8997f28c9253517a55d673c67a6c74/execroot/drake/bazel-out/k8-opt/bin/temp/visualize.runfiles/drake/temp/visualize.py", line 36, in visualize_gripper_frames pose RuntimeError: This MultibodyTree already has a joint 'sdformat_model_static_WorldBody_welds_to_box_link' connecting 'WorldBody' to 'box_link'. Therefore adding joint 'WorldBody_welds_to_box_link' is not allowed.

@quanvuong seem like an adequate error message?

[EricCousineau-TRI]

Also, at the present incantation of Rico's #14534 (r1 in Reviewable), this will throw immediately when the joint is added.

Thus, if you use adjust the logging verbosity, you should be able to see the immediate SDFormat file that has the redundant joint via a weld.

[quanvuong]

@rpoyner-tri Thanks! That looks good to me. It's a lot more intuitive to trace the error back to the static tag with the new error message. Please feel free to close the issue when you think it's apt.

[rpoyner-tri]

@quanvuong thanks for taking a look. The issue will auto-close when the PR lands. Looks like a bit of implementation discussion there still to settle.