Makes multibody/constraint private within examples/rod2d

[amcastro-tri]

Resolves #21156.

We move the entire multibody/constraint into the only place it gets used examples/rod2d.
The docs are removed from our public Doxygen modules to avoid this old confusion users have that we have an LCP solver in Drake. The docs are now a local README file in examples/rod2d/constraint.

---

This change is 

[amcastro-tri]

+@SeanCurtis-TRI for feature review please.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @amcastro-tri)

[amcastro-tri]

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @amcastro-tri)

---

multibody/constraint/constraint_docs.md line 6 at r1 (raw file):

dynamics with contact. We never got ConstraintSolver to perform well for
robotics applications, and therefore we no longer use the software in this
directory.

fyi @SeanCurtis-TRI, I only updated this intro paragraph, so that if anyone stumbles onto this, they don't mistakenly think it is our current strategy.

[jwnimmer-tri]

In #21156, we proposed removing the code. Demoting the documentation off of the website is a good start, but if we're going to remove the code anyway we might as well just remove the docs outright? Or if we're going go keep the code, the docs should stay on the website (but move to a different section, or be part of the class overview).

Could you reply (here or in that issue) about what the plan is for this code?

If you'd like to refer to the docs later, forevermore they will appear here. The catch-phrase we use is "git remembers". The only things on master should be things currently useful. If we don't use it, we should delete it and use git to read the old copies of it.

[SeanCurtis-TRI]

I second @jwnimmer-tri's comment. Why leave this in master at all?

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[amcastro-tri]

Right now this code is only used by an example in examples/rod2d. I think it does have a nice pedagogical value (like many of other examples do).
Since this is the only place where that code is used, probably we could move the constraints directoryintoexamples/rod2d`, and still keep the nice documentation.

As stated in #21156, the biggest issue is the missleading documenation of our contact solvers in a very public manner.

WDYGT?

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[jwnimmer-tri]

I think it does have a nice pedagogical value ...

Could you elaborate on this? I think this is where we differ. I might be missing something, though.

For my money, if we don't think this algorithm is useful in practice for robotics, then neither the algorithm nor its example should survive on master, and we should retire it into "git remembers".

[amcastro-tri]

I hear you. Definitely an overkill for just and example, but right now the Rod2D is the only one example in Drake that shows three different modeling alternatives, namely: piecewise DAE, discrete and continuous.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[jwnimmer-tri]

If the example is really crucial and the only sticking point, then another solution is to move the contact_solver code into the examples/rod2d directory. Since we think it's undesirable for robotics users to call it, having it be part of the installed packages is also undesirable. Moving it to examples allows the demo to still run, but undoes the problem where it looks like something users should try to use.

[amcastro-tri]

Sounds good Jeremy, I like this proposal. I'll move everything constraints to be constained withing the rod2d demo, without any confusion to users on whether this is something we support or no.
Working...

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[amcastro-tri]

All right, ready for review.
I did my best with package visibilities, but I definitely can use your advise on best practices here.
The visibility of examples/rod2d/constraint is now private and the Doxygen module was removed.
I kept the docs intact as an .md in examples/rod2d/constraint (only the first paragraph was updated to give a better intro for this now local and private piece of code)

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[jwnimmer-tri]

Once @SeanCurtis-TRI is happy with the new version (you two can iterate as needed), then I'll circle back here and add the deprecation shims so that this isn't a breaking change.

[SeanCurtis-TRI]

As we're dumping the readme into the example, I read through it to make sure there were no outstanding artifacts consistent with its new home. See my notes.

Otherwise and its ready for the deprecation shims.

Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: 6 unresolved discussions, needs at least two assigned reviewers

---

examples/rod2d/BUILD.bazel line 48 at r3 (raw file):

        "rod2d.h",
    ],
    visibility = ["//examples/rod2d/constraint:__pkg__"],

btw: This can be simplified (probably without harm).

Alternatively, you could change the package visibility:

package(default_visibility = [":__subpackages__"])

and omit this target-specific declaration entirely. To be perfectly frank, I'm not sure what the preferred spelling would be.

Suggestion:

visibility = [":__subpackages__"],

---

examples/rod2d/constraint/README.md line 5 at r3 (raw file):

This documentation describes `ConstraintSolver`, a prototype solver for mixed
linear complementarity problems (MLCPs). In particular, problems corresponding
to rigid multi-body systems with bilateral and/or unitlateral constraints as

nit: typo

Suggestion:

unilateral

---

examples/rod2d/constraint/README.md line 6 at r3 (raw file):

linear complementarity problems (MLCPs). In particular, problems corresponding
to rigid multi-body systems with bilateral and/or unitlateral constraints as
well polyhedral approximations of friction cone constraints.

nit: grammar

Suggestion:

well as polyhedral

---

examples/rod2d/constraint/README.md line 7 at r3 (raw file):

to rigid multi-body systems with bilateral and/or unitlateral constraints as
well polyhedral approximations of friction cone constraints.

BTW This is being relegated to examples because it wasn't deemed suitable for robotics. It seems capturing some explanation of that would be useful. It'd be a shame for someone to see this example and try to hoist it into their code, only to fail for the reasons we've already observed.

---

examples/rod2d/constraint/README.md line 285 at r3 (raw file):

  - Constraint regularization/softening is effected through `gammaN`
  - Constraint stabilization is effected through `kN`
  - Note that *neither Baumgarte Stabilization nor constraint regularization/

nit: This has a prefixed * that partner with another to bracket the text that needs emphasis. There is no partner. I suspect, it should go after "...regularization/softening".

Code quote:

*neither

---

examples/rod2d/constraint/README.md line 326 at r3 (raw file):

## <a id="discretization"></a>A stable discretization strategy

To be written. Refer to

nit: This should be updated.

1. We don't want to imply that this will undergo further development.
2. The referenced PR has merged, so it's not clear what significance the reference has.
3. Finally, this "section" is referenced twice (search for #discretization). In resolving this section, we should address the sites that reference it.

[jwnimmer-tri]

Reviewable status: 6 unresolved discussions, needs at least two assigned reviewers

---

examples/rod2d/BUILD.bazel line 48 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw: This can be simplified (probably without harm).

Alternatively, you could change the package visibility:

package(default_visibility = [":__subpackages__"])

and omit this target-specific declaration entirely. To be perfectly frank, I'm not sure what the preferred spelling would be.

See 21401 ("avoid subpackages"). I have yet to enforce that with a linter, so it's not surprising that Sean didn't know.

The bottom line is that nested directories in examples is bad form. It's way too complex. No example has so many files that it needs multiple subdirs to group them.

Put the relocated solver files into the rod2d folder directly. That's the best way to simplify.

[SeanCurtis-TRI]

Reviewable status: 6 unresolved discussions, needs at least two assigned reviewers

---

examples/rod2d/BUILD.bazel line 48 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

See 21401 ("avoid subpackages"). I have yet to enforce that with a linter, so it's not surprising that Sean didn't know.

The bottom line is that nested directories in examples is bad form. It's way too complex. No example has so many files that it needs multiple subdirs to group them.

Put the relocated solver files into the rod2d folder directly. That's the best way to simplify.

I was thinking flattening it as well. :)

Thanks for the pointer to #21401.

[amcastro-tri]

Reviewable status: needs at least two assigned reviewers

---

examples/rod2d/BUILD.bazel line 48 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I was thinking flattening it as well. :)

Thanks for the pointer to #21401.

ah.... I figured this might have been it. Done.

---

examples/rod2d/constraint/README.md line 7 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This is being relegated to examples because it wasn't deemed suitable for robotics. It seems capturing some explanation of that would be useful. It'd be a shame for someone to see this example and try to hoist it into their code, only to fail for the reasons we've already observed.

I agree, good idea. PTAL.

---

examples/rod2d/constraint/README.md line 285 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This has a prefixed * that partner with another to bracket the text that needs emphasis. There is no partner. I suspect, it should go after "...regularization/softening".

actually there is a matching * right after "... g(.)'s Jacobian".

Now, when I look at this, I have no idea why the emphasis. I removed it and split the sentence for clarity.

---

examples/rod2d/constraint/README.md line 326 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This should be updated.

1. We don't want to imply that this will undergo further development.
2. The referenced PR has merged, so it's not clear what significance the reference has.
3. Finally, this "section" is referenced twice (search for #discretization). In resolving this section, we should address the sites that reference it.

good catch. Removed, since without it, right now these notes are self-contained.

Side note: In retrospect, this might be one of the reasons this never worked. Stabilization must be implicit in order for it to be stable. I wonder if that's what bit Evan. We know anyway we don't like LCP-like solvers for contact, so no real reason why to even look into this.

[amcastro-tri]

Ready for the deprecation bits @jwnimmer-tri, no rush though.

Reviewable status: needs at least two assigned reviewers

[jwnimmer-tri]

Okay, I pushed the deprecation. The best way to do it was to keep the original source files in place. You're welcome to squash/force this as you wish.

+@jwnimmer-tri for platform review (tomorrow). Hopefully one or both of your can count yourselves as feature-review for my push.

Reviewed 2 of 2 files at r2, 1 of 10 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

examples/rod2d/constraint_solver.h line 16 at r4 (raw file):

namespace drake {
namespace multibody {

This namespace is wrong. It must be either drake::examples or drake::examples::rod2d. Ditto for all of the moved files.

https://drake.mit.edu/styleguide/cppguide.html#Namespace_Names

[amcastro-tri]

Thank you Jeremy! I would've never guessed how to speel those deprecations.

Question. As a lil experiment I brought back in the multibody/constraint:constraint_solver_test from master.
When I try to compile, the very first compiler error I get is:

bazel-out/k8-opt/bin/examples/rod2d/_virtual_includes/constraint_solver/drake/examples/rod2d/constraint_problem_data.h:35:8: error: conflicting declaration of template 'template<class T> struct drake::multibody::constraint::ConstraintAccelProblemData'
   35 | struct ConstraintAccelProblemData {
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~

which I believe is caused because the test depends on examples/rod2d, which brings in the drake/examples/rod2d/constraint_solver.h and the family.

However, after a big mountain of C++ error messages I get the first deprecation error:

bazel-out/k8-opt/bin/examples/rod2d/_virtual_includes/rod2d/drake/examples/rod2d/rod2d.h:478:65: error: 'using ConstraintAccelProblemData = struct drake::multibody::constraint::internal::ConstraintAccelProblemData<T>' is deprecated: \nDRAKE DEPRECATED: This class is being removed from Drake.\nThe deprecated code will be removed from Drake on or after 2024-09-01. [-Werror=deprecated-declarations]
  478 |     multibody::constraint::ConstraintAccelProblemData<T>* data) const;

So, it works, but not sure if you'd consider that a problem or not.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

multibody/constraint/constraint_problem_data.h line 519 at r5 (raw file):

template <class T>
using ConstraintAccelProblemData
    DRAKE_DEPRECATED("2024-09-01", "This class is being removed from Drake.")

shoudl we instead say?
ditto for others

Suggestion:

This class is being moved into examples/rod2d.

[SeanCurtis-TRI]

For deprecation stuff.

A question on deprecation strategy. Usually the deprecated functions keep their unit tests (to prevent regression during the deprecation period). In this case, we've lost the unit tests on the deprecated code. Is that intentional? Oversight?

Alejandro -- have you fixed the name spaces yet?

Reviewed 10 of 10 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

examples/rod2d/README.md line 12 at r5 (raw file):

ill-conditioned numerically, leading to catastrophic failures in practice.
Moreover, we found out that performance does not scale well with the number of
constraints. This lack of rubustness and performance is the reason why you find

nit typo

Suggestion:

robustness

---

examples/rod2d/constraint/README.md line 326 at r3 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

good catch. Removed, since without it, right now these notes are self-contained.

Side note: In retrospect, this might be one of the reasons this never worked. Stabilization must be implicit in order for it to be stable. I wonder if that's what bit Evan. We know anyway we don't like LCP-like solvers for contact, so no real reason why to even look into this.

Given you removed the references to this section, I assumed you'd also remove the section.

[jwnimmer-tri]

... which I believe is caused because the test depends on examples/rod2d, which brings in the drake/examples/rod2d/constraint_solver.h and the family.

Right. To backport the example to the deprecated code, you should remove all include statements of the examples code. Also the wrong-namespaces in the example make the errors worse. With proper namespaces, the errors would be less verbose.

A question on deprecation strategy. Usually the deprecated functions keep their unit tests (to prevent regression during the deprecation period). In this case, we've lost the unit tests on the deprecated code. Is that intentional? Oversight?

Intentional, but I did test the new aliases manually. It's always a cost-benefit for how much effort to invest in the deprecation stuff. Since this code is dying (as opposed to, e.g., being renamed) any current users are SOL no matter what. Having the aliases is nice but one way or another they need to do major surgery on their repo, so if the aliases ever stop working it's not that different of a reality for them.

Reviewed 9 of 10 files at r4.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

multibody/constraint/constraint_problem_data.h line 519 at r5 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

shoudl we instead say?
ditto for others

Remember that users cannot access Drake's examples via #include statements. All examples (other than a select few like the acrobot) are private to Drake, accessible only when building from source. Therefore, I don't think it helps to point them at code they can't use.

[amcastro-tri]

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

examples/rod2d/constraint_solver.h line 16 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This namespace is wrong. It must be either drake::examples or drake::examples::rod2d. Ditto for all of the moved files.

https://drake.mit.edu/styleguide/cppguide.html#Namespace_Names

ah.. I didn't realize. Done.

---

examples/rod2d/constraint/README.md line 326 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Given you removed the references to this section, I assumed you'd also remove the section.

obviously 🤦

[jwnimmer-tri]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: 7 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

examples/rod2d/BUILD.bazel line 114 at r6 (raw file):

)

add_lint_tests(enable_clang_format_lint = False)

nit Since you just repaved the entire PR with clang-format, we should turn on enforcement of it now to avoid more whitespace wars down the road.

Suggestion:

add_lint_tests()

---

examples/rod2d/rod2d.cc line 47 at r6 (raw file):

    // variables.
    auto state_index = this->DeclareContinuousState(Rod2dStateVector<T>(), 3, 3,
                                                    0);  // q, v, z

BTW Possibly you want to clean up the comment so it's more clear that it's referring to the 3,3,0 thing.

---

examples/rod2d/rod2d.cc line 272 at r6 (raw file):

  Matrix2<T> R_WC;
  // R_WC's elements match how they appear lexically: one row per line.
  R_WC << 0, ((xaxis_velocity > 0) ? 1 : -1), 1, 0;

BTW Is this line wrapping OK? You might want // clang-format {off,on} around it.

---

examples/rod2d/rod2d.cc line 289 at r6 (raw file):

  Matrix2<T> R_WC;
  // R_WC's elements match how they appear lexically: one row per line.
  R_WC << 0, 1, 1, 0;

BTW Is this line wrapping OK?

---

examples/rod2d/rod2d.cc line 415 at r6 (raw file):

  // Setup the generalized inertia matrix.
  Matrix3<T> M;
  M << mass_, 0, 0, 0, mass_, 0, 0, 0, J_;

BTW Is this line wrapping OK?

---

examples/rod2d/rod2d.cc line 771 at r6 (raw file):

Matrix3<T> Rod2D<T>::GetInertiaMatrix() const {
  Matrix3<T> M;
  M << mass_, 0, 0, 0, mass_, 0, 0, 0, J_;

BTW Is this line wrapping OK?

---

examples/rod2d/rod2d.cc line 780 at r6 (raw file):

Matrix3<T> Rod2D<T>::GetInverseInertiaMatrix() const {
  Matrix3<T> M_inv;
  M_inv << 1.0 / mass_, 0, 0, 0, 1.0 / mass_, 0, 0, 0, 1.0 / J_;

BTW Is this line wrapping OK?

[SeanCurtis-TRI]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: 8 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

---

examples/rod2d/rod2d.cc line 130 at r6 (raw file):

  const T inv_J = 1.0 / get_rod_moment_of_inertia();
  Matrix3<T> iM;
  iM << inv_mass, 0, 0, 0, inv_mass, 0, 0, 0, inv_J;

BTW Formatting change here as well. You might want // clang-format {off,on} around it.

[SeanCurtis-TRI]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)

[jwnimmer-tri]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri)