RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.35k stars 1.27k forks source link

Difficult for callers to satisfy prereqs of `MultibodyPlant::GetXByName` #12934

Open ggould-tri opened 4 years ago

ggould-tri commented 4 years ago

https://app.slack.com/client/T0JNB2DS4/C2WBPQDB7/thread/C2WBPQDB7-1585062720.104100 (mentioning @jwnimmer-tri here since he's on that thread)

The underlying issue is that MBP assumes that its callers already knew everything about the tree -- of course a caller knows what type that joint is, they created it! But code that has to run over a number of different SDFs (which might have joints of the same name but different type) really does not have that information at compile-time.

The result is recurring, un-factorable (because the string and type name covary undetectably at compile-time) code like this:

  if (plant->HasJointNamed("dishwasher_lower_rack_spring", id) &&
      (plant->GetJointByName("dishwasher_lower_rack_spring", id).type_name ==
      "prismatic") &&
      plant->HasJointNamed("dishwasher_lower_rack_joint", id) &&
      (plant->GetJointByName("dishwasher_lower_rack_joint", id).type_name ==
       "prismatic")) {
    const PrismaticJoint<double>* lower_rack_vertical_joint =
        plant->GetJointByName<PrismaticJoint>(
            "dishwasher_lower_rack_spring", id);
    const PrismaticJoint<double>* lower_rack_horizontal_joint =
        plant->GetJointByName<PrismaticJoint>(
            "dishwasher_lower_rack_joint", id);

Note that each of those if conditions is required by the exceptions clause of the GetJointByName documentation. Note in particular that it is impossible for a user to write the function template <JointType> JointType* GetJointByNameOrNull(name, id).

I can see a number of ways to fix this, including:

sherm1 commented 4 years ago

@ggould-tri I couldn't follow all of that -- how could type_name() be static? Can you sketch what you think would be an ideal version of the code you wrote above?

cc @amcastro-tri

ggould-tri commented 4 years ago

The ideal version wouid be something like

const PrismaticJoint<double>* lower_rack_vertical_joint =
        plant->GetJointByNameOrNull<PrismaticJoint>(
            "dishwasher_lower_rack_spring", id);
const PrismaticJoint<double>* lower_rack_horizontal_joint =
        plant->GetJointByNameOrNull<PrismaticJoint>(
            "dishwasher_lower_rack_joint", id);
if (lower_rack_vertical_joint && lower_rack_horizontal_joint) {

However that imaginary GetJointByNameOrNull is currently impossible to write correctly under GSG.

If there were a static type name available then I could write GetJointByNameOrNull<J>() by comparing J::type_name() to GetJointByName<Joint>().type_name(). But as it is to get type_name from a template parameter I have to be able to construct an object of that type, and there is no common ctor signature for all Joints so that's impossible.

jwnimmer-tri commented 4 years ago

FWIW I think all joints have Joint::kTypeName constant. (Not that we should use it.)

amcastro-tri commented 4 years ago

And Joint::kTypeName is public, so that'd allow us to write the GSG compliant GetJointByNameOrNull()?

ggould-tri commented 4 years ago

https://drake.mit.edu/doxygen_cxx/classdrake_1_1multibody_1_1_joint.html does not show kTypeName. kTypeName does not appear in joint.h or its superclass header. If a static type name constant is meant to be part of the joint concept, I think we need to say so somewhere?

jwnimmer-tri commented 1 year ago

As I read it, the only call to action here is to fix Doxygen to tell users that kTypeName exists, possibly near GetJointByName.