RobotLocomotion / drake

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

Improve error message for missing inertia #16992

Closed jwnimmer-tri closed 2 years ago

jwnimmer-tri commented 2 years ago

When a user forgets to add inertia to their links, Drake crashes with an obscure error message about a Delta > 0 failure. This can happen quite frequently for new users.

We should give a better diagnostic in this case (possibly at parsing-time). See https://github.com/RobotLocomotion/drake/issues/16181#issuecomment-1036424058 for details.

amcastro-tri commented 2 years ago

@rpoyner-tri, I know you've been working on better diagnostic at parsing. Would that be the right place where to emit a message?

rpoyner-tri commented 2 years ago

Possibly. Remember that there are currently 2 parsers (URDF and SDFormat), soon to be 3 (mujoco). We don't want to rely on some cut-and-paste splatter for the diagnostic. It may be that some common routine can be shared, or there may be some place further along (inside MbP, but not at the bare metal) that is a single chokepoint with appropriate context.

jwnimmer-tri commented 2 years ago

Possibly this needs to wait until Finalize time anyway? If I recall correctly, it's valid to have zero inertia when welded to the world, and we won't know that until the very end.

amcastro-tri commented 2 years ago

I see, then it is not that the user "forgot" to add the inertia, but more like the user defined a "zero" inertia. For SDF mass and rotational inertias are documented to be "1.0" by default. <inertial> is not required, IMO a defect if the user really intends to add a body (instead of just define a frame). This one is difficult to spot, since the user might have just forgotten or abused the usage of defaults for a "simpler" (though most likely wrong) model and there is nothing we can do if we see numbers coming in.

For URDF mass and rotational inertias are documented to be "0.0" by default. IMO also a defect if the user really meant to add a body instead of just a frame. This one in particular we can try to spot as you suggest @jwnimmer-tri at Finalize() if the body doesn't happen to be welded to another body with non-zero mass.

TL;DR. I believe this issue should be about non-valid numerical values like "0.0" rather than "missing" values since at the MBP level we must always supply these numbers but it is at specific parsers where values might be "missing". WDYT?

sherm1 commented 2 years ago

Drake has no problem with internal massless/inertialess links that are used only to provide frames for motion, actuation, or connections. For example, a 3-dof planar joint can be usefully replaced by this sequence: real-prismatic-massless-prismatic-massless-revolute-real allowing separate actuators for each dof. This can also be used to create multi-dof joints that we don't yet support. In these cases it is not better to provide small masses and inertias for the massless links.

We might want something like this:

The last warning could be suppressed for massless links that are welded to massful links. Ideally it would be suppressed for provably-internal massless links also but that is a little trickier.

jwnimmer-tri commented 2 years ago

@amcastro-tri I think the issue focus should be "Here's a common mistake that users often make. We should provide good feedback in this case." Possibly there are two or three kinds of common mistakes, not just one kind (e.g. calling MbP directly, versus writing a URDF, vs writing a SDFormat). I don't think the architectural layering matters at all to the issue scope. This is a user experience defect. The primary axis to think about for scoping should be the kinds of user input that cause the plant to print Delta > 0 due to modeling mistakes.

amcastro-tri commented 2 years ago

Yes, I understand @jwnimmer-tri. I am trying to trim the scope of the issue since the original Delta > 0 problem can be caused by a multitude of things. That being said, I like the final narrow list that @sherm1 put together.

jwnimmer-tri commented 2 years ago

Right. My proposal is that all common user mistakes whose only error message is Delta > 0 are the scope. If there are more then one, we should fix them all!

RussTedrake commented 2 years ago

FWIW -- I just had a student run into this error message again (it happens often). I agree it's important to fix.

xuchenhan-tri commented 2 years ago

@amcastro-tri Are you actively working on this one right now? Or is this now in drake adoption territory?

xuchenhan-tri commented 2 years ago

cc @mitiguy for awareness.

amcastro-tri commented 2 years ago

I am not working on it. This was actually my question we didn't get to in yesterday's meeting :-(. I'd be happy to work on it if no one else is. I'll place it in our TechDebt project. From now on, whatev I'm working on will be placed there. Everything else we can place in backlog as we make issues go up the priority queue. Please let me know if anyone else wants to work on this instead and I can grab something else.

sherm1 commented 2 years ago

Per yesterday's discussion, this would be a good Paul item. I'll assign it to @mitiguy for comment.

mitiguy commented 2 years ago

I'll start this now.

RussTedrake commented 2 years ago

FWIW, I've had a number of students run into this error message, too: https://github.com/RobotLocomotion/drake/blob/077b353deed5a64f735d924e1d5afeb9a92d6bcc/multibody/tree/body_node.h#L1072-L1075

They tend to find this post: https://stackoverflow.com/questions/65093710/extra-body-with-nans-in-context-of-a-multibodyplant-causing-error-while-simulati , but don't always find a way out from there. Perhaps you could judge whether that message needs improving, too.

amcastro-tri commented 2 years ago

Before I close this issue. The problem Russ mentions above is caused again by a free floating body with zero mass properties. This issue keeps converging to the same root issue. We must warn (or abort really) about zero inetia links that were not welded to some other non-zero mass link.

amcastro-tri commented 2 years ago

We'll close this long discussion in exchange for the more curated issue #17081.