Add ResolveConstraints logic to JointSliders (=> ModelVisualizer) #22496
Conversation
RussTedrake
added
release notes: newly deprecated
There was a problem hiding this comment.
+@jwnimmer-tri for feature review, please.
This PR contains #22495 as a first commit...; the request here is to review the second commit.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
There was a problem hiding this comment.
Just a high-level skim for now ...
Reviewed 13 of 13 files at r1, 2 of 10 files at r2, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
multibody/meshcat/joint_sliders.h line 199 at r2 (raw file):
systems::DiscreteStateIndex constrained_values_index_; systems::DiscreteStateIndex result_index_; mutable solvers::MathematicalProgram prog_;
This use of prog_ is not thread safe. We could either guard it with a mutex, place it into the context as a scratch value, or Clone the const program in ResolveConstraints (which would also then simplify the scope-cleanup logic).
multibody/meshcat/joint_sliders.h line 201 at r2 (raw file):
mutable solvers::MathematicalProgram prog_; solvers::VectorXDecisionVariable q_; std::unique_ptr<solvers::SolverInterface> solver_{};
nit The missing const here was very confusing (when evaluating thread-safety of the callbacks).
Suggestion:
std::unique_ptr<const solvers::SolverInterface> solver_{};multibody/meshcat/joint_sliders.h line 203 at r2 (raw file):
std::unique_ptr<solvers::SolverInterface> solver_{}; std::unique_ptr<systems::Context<double>> plant_context_{}; std::unique_ptr<MultibodyPlant<double>> owned_plant_{};
BTW The owned_plant_ could be done away with by changing plant_ into a shared_ptr<const MbP> which can store both an owned or unowned plant depending on how the shared_ptr is constructed.
multibody/inverse_kinematics/add_multibody_plant_constraints.h line 35 at r2 (raw file):
*/ std::vector<solvers::Binding<solvers::Constraint>> AddMultibodyPlantConstraints( const MultibodyPlant<double>& plant,
The plant must be passed by const pointer, since the function captures it. Actually a shared_ptr<const MbP> would even more clearly communicate what's happening, without requiring a special warning in the docs. The python
bindings also seem like they are missing the lifetime annotation.
I'll prepare a separate pull request with those fixes.
multibody/meshcat/joint_sliders.cc line 81 at r2 (raw file):
if (joint.num_positions() > 1) { description = fmt::format("{}_{}", joint.name(), joint.position_suffix(j));
BTW I'll open a new PR to enable clang-format in this directory in a moment. We'll either need to merge that change ahead of this PR, or else you'll need to revert the unrelated clang re-formatting here.
multibody/meshcat/joint_sliders.cc line 288 at r2 (raw file):
solvers::Binding<solvers::QuadraticCost> cost = prog_.AddQuadraticErrorCost(1.0, target_values, q_); std::vector<solvers::Binding<solvers::Constraint>> constraints;
As soon as we've declared constraints, we should use https://github.com/RobotLocomotion/drake/blob/master/common/scope_exit.h to arm the "remove from program" cleanup logic, so that if there are any exceptions our invariants will still hold.
Or in the alternative, clone the program into a local variable, mutate it in place, and then just throw it away when we're done. MP cloning is shallow, so should be pretty fast.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
multibody/inverse_kinematics/add_multibody_plant_constraints.h line 35 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The
plantmust be passed by const pointer, since the function captures it. Actually ashared_ptr<const MbP>would even more clearly communicate what's happening, without requiring a special warning in the docs. The python
bindings also seem like they are missing the lifetime annotation.I'll prepare a separate pull request with those fixes.
=> #22502
multibody/meshcat/joint_sliders.cc line 81 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW I'll open a new PR to enable clang-format in this directory in a moment. We'll either need to merge that change ahead of this PR, or else you'll need to revert the unrelated clang re-formatting here.
=> #22501
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
multibody/meshcat/joint_sliders.cc line 81 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
=> #22501
Done.
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
multibody/inverse_kinematics/add_multibody_plant_constraints.h line 35 at r2 (raw file):
Actually if you want to just remove this newly-added sentence ...
The
plantmust stay alive for the lifetime of the added constraints.
... from this PR, then we can decouple the two changes.
There was a problem hiding this comment.
i'll rebase and finish addressing your comments as soon as #22502 lands.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
multibody/inverse_kinematics/add_multibody_plant_constraints.h line 35 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
=> #22502
Thank you.
|
Both prerequisite PRs have landed on master now; this is ready for a rebase. |
There was a problem hiding this comment.
Reviewed 4 of 10 files at r2.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
bindings/pydrake/visualization/_model_visualizer.py line 525 at r2 (raw file):
self._diagram.ForcedPublish(self._context) sliders_context = self._sliders.GetMyContextFromRoot(self._context)
I guess I should have written it in a comment, because this has been unclear in a couple of changes to model_visualizer after I first wrote it.
Inside of Run, we must be extremely parsimonious with local variables that point into mutable objects, to make sure that the pointer is up-do-date. The best way to make sure we're using the right pointer is just to never save it in a local.
So, please remove this two locals. They are way too brittle to maintain over time. Yes, the code will be repetitive but at least it will be easy to inspect for correctness, without needing to reason about all possible control flow paths.
bindings/pydrake/visualization/_model_visualizer.py line 575 at r2 (raw file):
updated = self._sliders.EvalUniquePeriodicDiscreteUpdate( sliders_context) sliders_context.SetDiscreteState(updated)
BTW In particular, this line is a bug due to the local pointer variable. The sliders_context is a const context, not a GetMyMutableContextFromRoot context. This runs the risk of breaking caching.
multibody/meshcat/joint_sliders.h line 165 at r2 (raw file):
/** Returns true if the sliders have constraints to solve, not including the slider range limits which are enforced automatically. */ bool has_constraints_to_solve() const { return has_constraints_to_solve_; }
BTW Possibly we should add this to the bindings, too?
multibody/meshcat/BUILD.bazel line 86 at r2 (raw file):
srcs = ["joint_sliders.cc"], hdrs = ["joint_sliders.h"], deps = [
BTW It's probably time to split out implementation_deps = [] (for the cc file) from these deps = [] (for the header file).
The transitive include graph for solvers stuff is not thin.
RussTedrake
force-pushed
the
joint_sliders_ik2
branch
from
69b0cf8 to
a2d540b
Compare
There was a problem hiding this comment.
I've done the rebase.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/meshcat/joint_sliders.h line 201 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit The missing
consthere was very confusing (when evaluating thread-safety of the callbacks).
Done.
There was a problem hiding this comment.
Reviewed 5 of 9 files at r3, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/meshcat/BUILD.bazel line 86 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW It's probably time to split out
implementation_deps = [](for the cc file) from thesedeps = [](for the header file).The transitive include graph for solvers stuff is not thin.
Retracted. A lot of it is used in the header, now that look more carefully.
Updates JointSliders to have discrete state (previously it was stateless). Additionally, if the new ResolveConstraints logic is active, we publish the (rounded) resolved constraints back to Meshcat, so that the other sliders are updated. Since ModelVisualizer uses JointSliders, now ModelVisualizer also satisfies the multibody constraints. Resolves RobotLocomotion#18917. Deprecates `JointSliders::SetPositions(q)`, which has been replaced with `JointSliders::SetPositions(context, q)`.
RussTedrake
force-pushed
the
joint_sliders_ik2
branch
from
a2d540b to
4259465
Compare
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/inverse_kinematics/add_multibody_plant_constraints.h line 35 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Thank you.
Done.
multibody/meshcat/joint_sliders.h line 165 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Possibly we should add this to the bindings, too?
Done.
multibody/meshcat/joint_sliders.h line 199 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This use of
prog_is not thread safe. We could either guard it with a mutex, place it into the context as a scratch value, or Clone the const program inResolveConstraints(which would also then simplify the scope-cleanup logic).
Done. I've gone with the clone approach.
multibody/meshcat/joint_sliders.h line 203 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW The
owned_plant_could be done away with by changingplant_into ashared_ptr<const MbP>which can store both an owned or unowned plant depending on how the shared_ptr is constructed.
Done.
multibody/meshcat/joint_sliders.cc line 288 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
As soon as we've declared
constraints, we should use https://github.com/RobotLocomotion/drake/blob/master/common/scope_exit.h to arm the "remove from program" cleanup logic, so that if there are any exceptions our invariants will still hold.Or in the alternative, clone the program into a local variable, mutate it in place, and then just throw it away when we're done. MP cloning is shallow, so should be pretty fast.
Done.
bindings/pydrake/visualization/_model_visualizer.py line 525 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I guess I should have written it in a comment, because this has been unclear in a couple of changes to model_visualizer after I first wrote it.
Inside of Run, we must be extremely parsimonious with local variables that point into mutable objects, to make sure that the pointer is up-do-date. The best way to make sure we're using the right pointer is just to never save it in a local.
So, please remove this two locals. They are way too brittle to maintain over time. Yes, the code will be repetitive but at least it will be easy to inspect for correctness, without needing to reason about all possible control flow paths.
Done.
bindings/pydrake/visualization/_model_visualizer.py line 575 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW In particular, this line is a bug due to the local pointer variable. The sliders_context is a const context, not a
GetMyMutableContextFromRootcontext. This runs the risk of breaking caching.
Done. (by that token, the existing call to setpositions below was also wrong)
There was a problem hiding this comment.
There is a large amount of subtlety here, with some nuanced bugs (or at the very least, code that is difficult to follow and prove that it's correct) and so instead of trying to fix it all in once place, instead we should take smaller, more careful steps to reach the goal. We need a first PR that adds sliders state and deprecates the set function (without considering any constraints), and then add the constraints in a second PR. We can keep this PR as the second step (since has the solving stuff). I'll work on the first PR myself, since it's just a framework-refactor which is in my wheelhouse. I'll expect to post the draft by end of day.
Reviewed 3 of 5 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
bindings/pydrake/visualization/_model_visualizer.py line 575 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. (by that token, the existing call to setpositions below was also wrong)
=> #22550 to fix it fully.
multibody/meshcat/joint_sliders.cc line 242 at r4 (raw file):
bool removed_joint_limits = false; for (const auto& binding : prog_.bounding_box_constraints()) { if (binding.evaluator()->get_description() == "Joint limits") {
Is it important to remove the old constraint? If sliders' limits are monotonically tighter, it seems like just adding the new limit is safe and effective, and requires less brittle / overly-coupled code.
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
it's ok to not remove the constraint. optimizers are more brittle than other code, and in general it is good practice to write minimal optimization formulations. But i agree that this one is probably benign. |
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Working
... take smaller, more careful steps to reach the goal. We need a first PR that adds sliders state and deprecates the set function ...
Actually after working on it, we need three PRs:
(1) Refactor to disavow the possibility that joints (and thus sliders) are not a full covering of the plant => #22553. The code here was not correct in the presence of non-joint dofs (i.e., like when the plant used to have floating bodies not use joints). Instead of fixing it to handle that case, it's vastly simpler to forbid it.
(2) Add state, events, and deprecate the non-context setter. (I'll prepare this one, too.)
(3) Add constraint handling (this PR).
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/meshcat/joint_sliders.h line 150 at r4 (raw file):
*/ DRAKE_DEPRECATED("2025-05-01", "Use SetPositions(context, q) instead.") void SetPositions(const Eigen::VectorXd& q);
Actually, why are we deprecating this? Isn't it still useful to be able to tweak the initial_value passed to the constructor after construction, but before context-creation?
Even if a context has been created, this will move the sliders and then its next discrete update will solve for the constraints and update the context.
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
i considered leaving it and could be convinced. I just don't want it to be confusing having both, with slightly different implications |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
multibody/meshcat/joint_sliders.h line 150 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
i considered leaving it and could be convinced. I just don't want it to be confusing having both, with slightly different implications
Ok, thanks. In my next PR I'll have a proposal for keeping it, and we can consider the details there.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
... take smaller, more careful steps to reach the goal. We need a first PR that adds sliders state and deprecates the set function ...
Actually after working on it, we need three PRs:
(1) Refactor to disavow the possibility that joints (and thus sliders) are not a full covering of the plant => #22553. The code here was not correct in the presence of non-joint dofs (i.e., like when the plant used to have floating bodies not use joints). Instead of fixing it to handle that case, it's vastly simpler to forbid it.
(2) Add state, events, and deprecate the non-context setter. (I'll prepare this one, too.)
(3) Add constraint handling (this PR).
Actually, nevermind. After chasing down (2) and seeing how difficult it is to get it correct (I'm not even done yet), I think it's time to take a step back.
Here, we're using state for "the last thing my visualizer show to the user". That's not state. State is for things inside of the diagram. The "last/current thing my visualizer is showing" is not state, that's a (mutable, mutexed) member field. Even if we have 100 contexts, there is only one slider, so having 100 copies of its "last value" is architecturally wrong.
As already noted in the class overview doc, if the user wants to have a consistent view when multiple systems are reading out the positions, then the user should do that by connecting a ZOH after the output.
In order to obey constraints, the most we would need is a publish event that reads the slider values, checks for changes, and then writes back the new values. Or even simpler, we can probably just have the existing CalcOutput function do the solving (and then write back any changes to rounded values).
That loses the ability to resolve constraints to a finer precision than the slider step, but having the output port convey a value different than what's shown on the screen always seemed like a dubious idea to me. For simple constraints like mirrored finger joints, it won't matter since both fingers are presumably the same slider min/max/step anyway.
I don't think that's viable. For mimic joints yes. But even unit quaternion joints, no. Certainly for loop joints (like in Cassie's leg), etc, that's not ok. |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
That loses the ability to resolve constraints to a finer precision than the slider step
I don't think that's viable. For mimic joints yes. But even unit quaternion joints, no. Certainly for loop joints (like in Cassie's leg), etc, that's not ok.
In any case, the proposal can still be tweaked to output the precise values. CalcOutput can still do the solve, write back any changes to rounded values to Meshcat, and then output the precise positions on the port.
There are two (main) discrete states. One is I understand your objection to be on the |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
I think you're ok calling that state?
I'm not. I don't think this class should have any state at all.
This is similar to the way that an lcm subscriber latches its incoming messages as state.
The sliders here already took a different path. Our class docs explicitly say to put a ZOH afterwards if you want a consistent value. I do think that's better, or at least changing it at this point is way too much of a headache with no upside.
The story for LCM is a bit different since the messages arrive asynchronously, so having the source also encapsulate the latching can be helpful in terms of timing. Here, the value doesn't change until we probe for the new one, so we can do it as often as we like. Also the size of the value here is tiny compared to LCM messages (re: copying it perhaps more than necessary).
If you think a mutable member field is better, that's surprising to me, but I'm certainly open to hear your proposal.
There is only one set of sliders. Storing anything that affects the slider computation in N contexts is absolutely wrong, since there is only 1 set of sliders, not N. It's the job of the ZOH to latch the sliders into any particular context's state, and that's the most state we should tolerate (it's a one-way flow of information -- state is only ever output, it's not part of the dynamics step).
Having shadow state values that are not shown in the UI still feels wrong to me. If Meshcat limits us to have discretely stepped values, then that's what our output port should have. If we need sliders to have full range of floats, then we should just fix Meshcat (to store both the rounded and unrounded values after a call to SetSliderValue), not work around it at every call site like this. The "extra resolution during SetValue" should be encapsulated by the slider in Meshcat, if anything.
I understand that constraints will project the q into non-stepped values, but that's the most voodoo I can entertain. The output port should be the nearest instantaneous constraint-meeting projection of the stepped slider values. Adding history / hysteresis in JointSliders is incredibly complex to reason about, and I don't understand what it's important. If it's only job was working around Meshcat being deficient, then we should just fix Meshcat. If there is some other reason for hysteresis, I don't understand it yet.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
is incredibly complex to reason about
can you help me understand what cases you are working through?
One JointSliders and one meshcat in the browser seems clean.
Then we have one JointSliders with multiple meshcat browsers; but this is handled by the abstraction that Meshcat::GetSliderValue() provides (aside: i could have made the docstring better there, the GetButtonClicks is more explicit about what happens if multiple browsers are open). And SetSliderValue will publish to all open browsers.
Then we could potentially have one (or multiple) browsers with multiple JointSliders running with different contexts. But meshcat is not allowed to run in anything but the main thread... so there are some walls around this use case, too? So I'm not totally sure how to think about the N contexts case?
Here, the value doesn't change until we probe for the new one, so we can do it as often as we like.
I was worried that the call to GetSliderValue requires grabbing a mutex, so could potentially be expensive? "we can call it as often as we like" was not my mental model.
we should just fix Meshcat
I'm not against that. But you're significantly expanding the scope on me.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
we should just fix Meshcat
Did you mean meshcat.cc or meshcat.js? It's not clear to me that pushing this to the gui actually makes sense?
There was a problem hiding this comment.
We need a first PR that adds sliders state ... I'll work on the first PR myself, since it's just a framework-refactor which is in my wheelhouse. I'll expect to post the draft by end of day.
Retracted. I tried it and it didn't work at all. I no longer think we need to split this up. Instead, we should not have State.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
I hadn't even considered multiple browsers, but I don't imagine that's a problem given how Meshcat works.
can you help me understand what cases you are working through?
The fundamental challenge is 1 system with N contexts. The "Run" function isn't the only (or even the primary) API.
It's all about the event handling in multiple contexts, and which contexts end up being copied back into state or not, published, and how all of that sequences out. The bottom line is that there is only one copy of the slider values in Meshcat, and trying to keep that "synchronized" with N contexts in a way that is sound is terrible pain. Even synchronizing with 1 context is fairly difficult.
For example, if I call context.set_state() on the third context, and then force a publish event, what should happen in the first two contexts? What happens on the next timed event for each of them.
State update events are speculative. They are supposed to have no observable effect until the simulator applies them to a context (by copying them). They are data, not actions. Having a state update event publish data to external programs (change the sliders) is against the design premise. Only publish events (where the simulator has guaranteed that the context is up-to-date, no more speculation) should move the sliders.
I'm still not clear what we are trying to solve by making anything here be per-context. We can obey constraints without adding State. We can display the constraint-altered slider values in the slider GUI without adding State.
But you're significantly expanding the scope on me.
My goal is to protect the Drake team from a maintenance nightmare. I still don't buy that we need sub-tick resolution to persist (instead, the user should set the tick resolution to match their UX requirements), but if we do need it then implementing it in a maintainable way is just the cost of doing business.
It's not clear to me that pushing this to the gui actually makes sense?
I don't understand your requirements, so I can't say for sure, but yes I was thinking it would have an effect on the UI. If we really must preserve 10 sig figs for sliders, then text on the screen with the number shown next to the slider needs to display all 10 sig figs, and the user needs to be able to type in 10 sig figs on the screen and hit enter and we keep their full precision intact. Having the 10 sig figs hidden in some sticky storage but showing in the screen is misleading.
multibody/meshcat/joint_sliders.h line 150 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ok, thanks. In my next PR I'll have a proposal for keeping it, and we can consider the details there.
Assuming we ditch having State, then I assume we won't deprecate this.
multibody/meshcat/joint_sliders.cc line 242 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
it's ok to not remove the constraint. optimizers are more brittle than other code, and in general it is good practice to write minimal optimization formulations. But i agree that this one is probably benign.
In that case let's not hard-code the magic string from a far-away function here.
multibody/meshcat/joint_sliders.h line 186 at r4 (raw file):
std::shared_ptr<geometry::Meshcat> meshcat_; const MultibodyPlant<T>* const plant_; std::shared_ptr<const MultibodyPlant<double>> double_plant_{nullptr};
nit const for clarity.
Suggestion:
const std::shared_ptr<const MultibodyPlant<double>> double_plant_;
Updates JointSliders to have discrete state (previously it was
stateless). Additionally, if the new ResolveConstraints logic is
active, we publish the (rounded) resolved constraints back to Meshcat,
so that the other sliders are updated.
Since ModelVisualizer uses JointSliders, now ModelVisualizer also
satisfies the multibody constraints.
Resolves #18917.
Deprecates
JointSliders::SetPositions(q), which has been replaced withJointSliders::SetPositions(context, q).Screen.Recording.2025-01-19.at.2.44.09.PM.mov
This change is