RobotLocomotion / drake

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

MbP methods could use optional model_instance (vs overloading) #10309

Open RussTedrake opened 5 years ago

RussTedrake commented 5 years ago

It seems like MBP and MBT have almost twice as many methods as they need to have. Almost every method (from num_positions() to GetJointByName()) has at least two variants -- one that takes a model_instance and one that does not. This starts to explode when, for instance GetJointByName has a templated version and a non-templated version (so 4 entry points). I need to now add the mutable variety... so 8 entry points!! And every one of them is repeated in MBP and MBT. (16 copies!). And there are lots of copies of the method documentation flying around.

Is there a reason we don't just take model_instance as an optional argument with a default value of ModelInstanceIndex{}, and then check is_valid() on the inside? It would cut down the entry points by half at least? And presumably the cost would be negligible?

EricCousineau-TRI commented 5 years ago

Relates #10194

Agree on this front, along the lines that we should reduce the number of overloads in the public API.

(Reworded title to point to just MBP; MBT should no longer be a considered a public API.)

EDIT: Per Sherm's comment below, definitely agree on usage of optional<ModelInstanceIndex>. It's used in some of the frame stuff: https://github.com/RobotLocomotion/drake/blob/76d9efe8f34b37433800245a13b5172e5928b571/multibody/tree/fixed_offset_frame.h#L55

sherm1 commented 5 years ago

Could use optional<ModelInstanceIndex> for that fresh up-to-date look!

RussTedrake commented 5 years ago

I agree that the optional version can work (have it on my branch), but I feel like it is extra unwarranted machinery, since the TypeSafeIndex already has the notion of invalid indices? But if people agree it's more clear, I'm ok with it.

SeanCurtis-TRI commented 5 years ago

FTR The TypeSafeIndex documentation explicitly calls out not using the invalid value as a sentinel value. If it wasn't for STL it wouldn't have one at all.

jwnimmer-tri commented 7 months ago

For the record, GetJointByName is already de-dup'd. Other things like GetJointActuatorByName remain overloaded.