Joint limits cannot be changed post-finalize in simulations

[calderpg-tri]

MBP exposes a method to change joint limits at any time via Joint::set_position_limits(). However, for a plant used in simulation the joint limits are cached at finalize by MultibodyPlant::SetUpJointLimitsParameters() and later calls to Joint::set_position_limits() have no effect.

cc @sammy-tri

[jwnimmer-tri]

This sounds somewhat similar to #14841.

[sherm1]

Assigning to Joe for disposition, please.

[jwnimmer-tri]

This sounds like a correctness bug, for which at minimum we should fail-fast if we can't function correctly? Seems like someone should be digging into this?

[sherm1]

Thoughts, @joemasterjohn ?

[jwnimmer-tri]

Option (1):

One relatively complicated proposal for fixing this would be to store the declared joint limits as numeric Parameters in the Context, like we do with other things like actuator gear ratio or fixed offset frame pose or etc. The current default ("model") value limits would be copied from the Joint member data into the Context when a Context is created. Then, the MbP's JointLimitsParameters would move from a member field into a cache entry instead, with prereqs of the tree's joint parameters, so that stiffness and damping would be calculated on demand upon first use, instead of during Finalize.

In that way, not only would the values actually be correct at all times, but the user could even use different limits in different contexts, in case they wanted to try a lot of alternatives at once.

Option (2):

If we don't need to customize multiple per-Context limits at once, then another way to fix this would still be to put the JointLimitsParameters in the context, but to calculate its value during CreateDefaultContext instead of during Finalize, and never change it in that particular Context. That would typically mean more unnecessary recalculations (re-doing the math once per Context), but from a quick skim it doesn't seem like the damping & etc math is that many flops.

You could also imagine memoizing this so that Finalize takes a guess and then CreateDefaultContext skips the math if the joints' limits haven't changed since Finalize.

Option (3):

Change Joint so that setting limits post-Finalize throws an error. This would probably be annoying for users, but at least we would not be calculating incorrect results.

---

@sherm1 can you weigh in here?

As we start adding planning code into Drake, people will expect to be able to customize the joint limits and they will be surprised when nothing happens.

It seems to me like (2) would be a plausible quick fix?

[sherm1]

IMO this is a special case of our ongoing wish that Drake permit remodeling. Every interesting parameter should be treated identically, in the manner @joemasterjohn did beautifully for reflected inertia in JointActuator. The API for setting a parameter thing (say of type double for exposition) should be:

double default_thing() const;  // The value thing will have in a default context
void set_default_thing(double);  // Non-const; writing into the System
double thing(const Context&) const;  // The value thing has in the given context (used for all computation)
void set_thing(Context*, double) const;  // Invalidates appropriate cache entries

Invoking the non-const set_default_thing() method should be considered a fairly dramatic change (Simbody calls those "topological changes") and for MbP should be prohibited after Finalize() or (better, but not currently feasible I believe) restore MbP to its pre-Finalize state and require re-finalization.

The quickest fix that moves us in the direction of the correct architecture is Option (3) above -- don't allow calls to non-const methods in MbP post-finalize. We already have many checks of that kind in MbP but missed this one.

Then the right fix is Option (1), which is not very difficult (Joe is already an expert).

[jwnimmer-tri]

Every interesting parameter should be treated identically [e.g.] in JointActuator ... Invoking the non-const set_default_thing() method should be considered a fairly dramatic change ... and for MbP should be prohibited after Finalize().

That's not true today for any of the simple parameters (e.g., both JointActuator::set_default_rotor_inertia and MbP::SetDefaultFreeBodyPose are currently permitted to happen post-Finalize), and in fact it would be a terrible user experience to require all simple parameter tuning to occur pre-Finalize.

The only unique thing for joint limits is that MbP is pre-computing a little bit of math ahead of time which is derived from the tree's default values, yet did so unsoundly. I think that's an implementation defect, not a design defect with the contract.

It might still be worth doing (3) quickly to avoid lying to users, but I disagree that our architectural premise should be to essentially never permit changing any default values except via the Context.

[sherm1]

If the intended semantics of those is "only affects the next created DefaultContext" then I agree that's fine. Having default in the function name makes that clear. But IMO a function like set_position_limits() that doesn't actually change position limits when called is unnecessarily confusing. Renaming to set_default_position_limits() would be good, along with fixing the bug if it doesn't even do that currently!