Closed ggould-tri closed 3 years ago
The python code that brought about this error was:
import math
import time
from pydrake.systems.framework import (DiagramBuilder,)
from pydrake.systems.primitives import (ConstantVectorSource,)
from pydrake.geometry import (Box,
DrakeVisualizer)
from pydrake.math import (RigidTransform, RollPitchYaw)
from pydrake.multibody.plant import (AddMultibodyPlantSceneGraph, CoulombFriction, Propeller, PropellerInfo)
from pydrake.multibody.tree import (SpatialInertia, RotationalInertia, PlanarJoint)
from pydrake.systems.analysis import Simulator
def make_box(mbp, name):
inertia = SpatialInertia.MakeFromCentralInertia(1, [0, 0, 0], RotationalInertia(1/600, 1/120, 1/120))
body = mbp.AddRigidBody(name, inertia)
shape = Box(1, 0.1, 0.1)
mbp.RegisterVisualGeometry(
body=body, X_BG=RigidTransform(), shape=shape, name=f"{name}_visual",
diffuse_color=[1., 0.64, 0.0, 0.5])
body_friction = CoulombFriction(static_friction=0.6,
dynamic_friction=0.5)
mbp.RegisterCollisionGeometry(
body=body, X_BG=RigidTransform(), shape=shape,
name="{name}_collision", coulomb_friction=body_friction)
return body
def make_mbp(builder):
mbp, sg = AddMultibodyPlantSceneGraph(builder, time_step=0.001)
robot_body = make_box(mbp, "robot")
planar_joint = mbp.AddJoint(
PlanarJoint(name="robot_planar_joint",
frame_on_parent=mbp.world_frame(),
frame_on_child=robot_body.body_frame(),
damping=[1,1,0.9]))
blocker1 = make_box(mbp, "blocker1")
mbp.WeldFrames(B=mbp.world_frame(), A=blocker1.body_frame(), X_AB=RigidTransform([1,0.5,0]))
return mbp, sg
def add_thrusters(builder, mbp):
robot_body_index = mbp.GetBodyByName("robot").index()
robot_tail_forward = RigidTransform(p=[-0.5, 0, 0], rpy=RollPitchYaw([math.pi/2, 0, 0]))
robot_tail_clockwise = RigidTransform(p=[-0.5, 0, 0], rpy=RollPitchYaw([math.pi/2, 0, math.pi/2]))
thrusters = builder.AddSystem(
Propeller([PropellerInfo(robot_body_index, X_BP=robot_tail_forward),
PropellerInfo(robot_body_index, X_BP=robot_tail_clockwise)]))
builder.Connect(thrusters.get_spatial_forces_output_port(), mbp.get_applied_spatial_force_input_port())
builder.Connect(mbp.get_body_poses_output_port(), thrusters.get_body_poses_input_port())
return thrusters
def run(sleeptime=0.001, thrust = [10, 2]):
builder = DiagramBuilder()
mbp, sg = make_mbp(builder)
mbp.Finalize()
DrakeVisualizer.AddToBuilder(builder, sg)
thrusters = add_thrusters(builder, mbp)
cmd = builder.AddSystem(ConstantVectorSource(thrust))
builder.Connect(cmd.get_output_port(), thrusters.get_command_input_port())
diagram = builder.Build()
simulator = Simulator(diagram)
for step in range(1000):
simulator.AdvanceTo(0.001 * step)
mbp_context = diagram.GetSubsystemContext(mbp, simulator.get_context())
time.sleep(sleeptime)
run()
The python error stack was:
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
<ipython-input-54-e04982a460db> in <module>
67 time.sleep(sleeptime)
68
---> 69 run()
<ipython-input-54-e04982a460db> in run(sleeptime, thrust)
53 builder = DiagramBuilder()
54 mbp, sg = make_mbp(builder)
---> 55 mbp.Finalize()
56 DrakeVisualizer.AddToBuilder(builder, sg)
57
RuntimeError: This multibody tree already has a mobilizer connecting these two frames. More than one mobilizer between two frames is not allowed
The C++ stack trace of the error was:
(gdb) bt
#0 0x00007f1d4aa7ad1d in __cxa_throw () from /usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/libstdc++.so.6
#1 0x00007f1d49d8619b in drake::multibody::internal::MultibodyTreeTopology::add_mobilizer(drake::TypeSafeIndex<drake::multibody::FrameTag>, drake::TypeSafeIndex<drake::multibody::FrameTag>, int, int) ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
#2 0x00007f1d49da2f01 in drake::multibody::internal::QuaternionFloatingMobilizer<double> const& drake::multibody::internal::MultibodyTree<double>::AddMobilizer<drake::multibody::internal::QuaternionFloatingMobilizer>(std::unique_ptr<drake::multibody::internal::QuaternionFloatingMobilizer<double>, std::default_delete<drake::multibody::internal::QuaternionFloatingMobilizer<double> > >) ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
#3 0x00007f1d49da314f in drake::multibody::internal::MultibodyTree<double>::AddQuaternionFreeMobilizerToAllBodiesWithNoMobilizer() ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
#4 0x00007f1d49da34ba in drake::multibody::internal::MultibodyTree<double>::Finalize() ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
#5 0x00007f1d49e3a7fd in drake::multibody::internal::MultibodyTreeSystem<double>::Finalize() ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
#6 0x00007f1d49f58fa1 in drake::multibody::MultibodyPlant<double>::Finalize() ()
from /home/rico/checkout/drake/bazel-bin/bindings/pydrake/py/wtf_weld.runfiles/drake/bindings/pydrake/common/../../../_solib_k8/_U_S_Stools_Sinstall_Slibdrake_Cdrake_Ushared_Ulibrary___Utools_Sinstall_Slibdrake/libdrake.so
Renaming the parameters "parent" and "child" like any other joint would have a high value/effort ratio! As currently implemented a Weld is just a 0-dof joint, and it maps to a 0-dof mobilizer in the multibody tree. While there are certainly other ways it could be implemented, this implementation makes all the other Joint methods work, preserves the body numbering, allows for reaction forces at the weld to be calculated with no special code, etc.
Maybe but I am not sure: The effort must include deprecation as these names are exposed as part of the python API.
In my view, I see two defects here (so possibly two issues?). The first is that WeldFrames() is apparently error-prone. It seems to me that the parameter names came from monogram notation, and perhaps that is failing us here.
The second is that the mobilizer error message is terrible; every instance I am aware of where people hit it, we end up at least filing an issue. Also, even if we fix (1), the WeldFrames() naming, I suspect there are still other paths from public API (mis)use that can reach it.
So, possible work scope:
cc original author @amcastro-tri in case you have any thoughts
FWIW, when I worked on https://github.com/RobotLocomotion/drake/pull/14534, I relied on the existing message as a semantic backstop, and source of truth about how the system wants to work. My brief look at improving the existing message convinced me that the layer where it live now lacks a lot of useful context information to make a more informative message. The condition enforced is fine, but we may have to catch it (perhaps redundantly) at another scope to give a decent message.
Thanks, Rico! I think you're exactly right.
Since parameter names are only deprecation-sensitive at the bindings layer, it might be possible to implement the deprecation at that layer. If not, there's probably no good way to do deprecation, since the old and new method have identical signatures for C++ purposes. And unfortunately there's a decent chance that the current signature is relied on downstream in eg examples from Russ's class; callers will call by kwarg because it's the only way to not get your transform backward.
(I actually like the current parameter names -- they are unambiguous with the transform and match the intuition that a weld is symmetric and doesn't have "direction". Unfortunately this "obvious fact" is a lie. But if it were possible the far better solution would be to automatically resolve inboard/outboard on welds before mobilizing them. This is a case where there is exactly one right way to do it, the computer knows, and the human doesn't, so it would be much cleaner to be automatic if we could :-/ )
I like the idea of making the inboard/outboard correction automatic; definitely saves deprecation effort/risk and gives a friendlier API experience. Does there need to be a run-time warning when direction corrections happen, or will updated documentation suffice?
Weld joints are not fully symmetric when you consider results reporting. If you ask for reaction forces at the weld (which act equally and opposite on the two bodies) there is a convention (same as for all joints) as to which of those two forces is reported and in which frame (parent or child) they are expressed.
I like the idea of the automatic reversal for weld joints. @sherm1, would the concept of reverse mobilizers solve the problem of reporting reaction forces? (i.e. if a mobilizer is reversed, then we shift forces back to the inboard frame of the original mobilizer).
Yes, having the weld joint map to one of two weld mobilizers depending on parent/child ordering could be used to keep track of the mapping back to the joint for results. Technically all that is needed though is a single reversed
bit stored with the mobilizer to clarify how the results mapping is to be performed. More generally, joints with dofs are trickier to reverse automatically so having a library of forward and reverse mobilizers is an easy and computationally efficient way to deal with them and having a reverse weld mobilizer would avoid special-casing for welds.
Section 2.4 of this paper shows how to automatically create reversed mobilizers in general without implementing reverse versions, but there is a computational cost to doing it that way. (Simbody implements the general reversal idea.)
I'll admit I'm starting to lose the thread re automatic reversal; it's starting to sound like scope creep to me. Am I wrong about that?
Agreed. In my view the ideal fix is to treat Weld like any other joint (that is, so it connects a parent body to a child body). Then it follows the same rules as any other joint so would avoid confusion. I actually hate the idea of special-casing it -- that's what caused the present problem. So: rename the A/B bodies to parent/child like other joints, fix the comments, close the issue, and see if that is enough to avoid trouble in the future.
I'm still inclined to treat this as needing two fixes:
Ah, nothing like code reading.
So the WeldJoint constructor already has naming "parent_frame_P", "child_frame_C", "X_PC". All of this seems like an improvement until we get to this: https://github.com/RobotLocomotion/drake/blob/master/bindings/pydrake/multibody/test/plant_test.py#L1524-L1525
So it seems that even at the lower layer we are still fighting a battle between monogram notation and English. Other joints don't use monogram-influenced parameter names because they don't require an arbitrary transform as input.
In the spirit of "make Welds like other joints" suggested above, here is a proposal.
The parent/child direction of a weld matters (which is itself odd, since its arguments are "A" and "B" rather than "parent" and "child"). Worse, if you apply a weld backward (welding world to object instead of object to world) then the error message you get is the cryptic and decidedly non-weldlike
(Python code that demonstrates this in the comment below, along with a stack trace)
See also: https://app.slack.com/client/T0JNB2DS4/threads/thread/C2WBPQDB7-1610670547.003800
The best outcome would be for welds to just adopt the unique correct direction automatically. A reasonable outcome would be an error message that names the two frames and two mobilizers (however, see comment below, the mobilizer name is in fact unhelpful craziness) plus a rename of the parameters of the AddWeld method.