Unify RotationalInertia validity tests and exception messages.

[mitiguy]

This PR is work towards a unified and logical sequence for inertia validity checks.
This PR is specifically for RotationalInertia().
A subsequent one will follow for SpatialInertia() and its cousin in the Geometry class.

All of the testing for validity is delegated to one function, namely GetInvalidityReport().
It reports an empty string if all seems well, otherwise it returns a string with why the inertia is invalid.
This process will be repeated for each of three classes and provide a better mechanism for error handling and for ensuring that inertia validity checks provide consistently sensible messages.

---

This change is 

[mitiguy]

Feature review +@sherm1

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @mitiguy)

[sherm1]

Feature pending some minor comments
+@SeanCurtis-TRI for platform review per rotation, please

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @mitiguy)

---

multibody/tree/rotational_inertia.h line 986 at r1 (raw file):

  // inequality. For symbolic type T, tests are rudimentary (e.g., test for NaN
  // moments or products of inertia).
  std::string GetInvalidityReport() const;

minor: should use more straightforward std::optional<std::string> rather than using an empty string as a bespoke signal.

---

multibody/tree/rotational_inertia.h line 986 at r1 (raw file):

  // inequality. For symbolic type T, tests are rudimentary (e.g., test for NaN
  // moments or products of inertia).
  std::string GetInvalidityReport() const;

minor: this name needs an upgrade. As I understand it, it is actually investigating this RotationalInertia for potential invalidity. The current name sounds like it is passively returning an existing report. Consider something that reflects the work being done better, .e.g. CheckValidity() or CheckValidityAndReportIfInvalid() or some such.

[SeanCurtis-TRI]

It's not quite complete. See below.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @mitiguy)

---

multibody/tree/rotational_inertia.h line 986 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: this name needs an upgrade. As I understand it, it is actually investigating this RotationalInertia for potential invalidity. The current name sounds like it is passively returning an existing report. Consider something that reflects the work being done better, .e.g. CheckValidity() or CheckValidityAndReportIfInvalid() or some such.

How about, simply MakeValidityReport()? CreateValidityReport()?

---

multibody/tree/rotational_inertia.h line 1000 at r1 (raw file):

  template <typename T1 = T>
  typename std::enable_if_t<scalar_predicate<T1>::is_bool>
  ThrowIfNotPhysicallyValid(const char* func_name) {

BTW As long as you're here, you might consider the following:

Take all of this:

  // SFINAE for numeric types. See ThrowIfNotPhysicallyValidImpl().
  template <typename T1 = T>
  typename std::enable_if_t<scalar_predicate<T1>::is_bool>
  ThrowIfNotPhysicallyValid(const char* func_name) {
    ThrowIfNotPhysicallyValidImpl(func_name);
  }

  // SFINAE for non-numeric types -- does nothing;
  template <typename T1 = T>
  typename std::enable_if_t<!scalar_predicate<T1>::is_bool>
  ThrowIfNotPhysicallyValid(const char*) {}

and simply replace it with:

  // We don't attempt validation for T = Symbolic.
  void ThrowIfNotPhysicallyValid(const char* func_name) {
    if constexpr (scalar_predicate<T>::is_bool) {
      ThrowIfNotPhysicallyValidImpl(func_name);
    }
  }

The primary argument for not doing this in this PR is that you still have a number of old-school SFINAE stuff on other functions in the file, so rolling all of those into their own PR would be perfectly reasonable. But it seems that while you're messing around in these files, we can modernize and compact this code.

---

multibody/tree/rotational_inertia.cc line 225 at r1 (raw file):

    error_message = fmt::format("\nNaN detected in RotationalInertia.");
  } else if constexpr (scalar_predicate<T>::is_bool) {
    if (!AreMomentsOfInertiaNearPositiveAndSatisfyTriangleInequality()) {

One thing we talked about on the whiteboard was that this function would get digested into this function.

The reasoning is as follows:

1. This test currently has information about invalidity -- i.e., non-positive values, non-triangle inequality.
2. In the future, you were recommending a cascading sequence of tests starting as close to the actual values in memory before operating on a transformed value.

None of that information is available to the caller. We'd discussed moving three functions in to the one report and this was one of them.

That need not be the only solution. This could likewise return a std::optional<string> (nullopt for valid, non-null with information if there was a problem). And the caller could make use of that. Furthermore, the caller wouldn't have to blindly recompute the principle moments again, because if they were relevant, the AreMomentsOfInertia....() function would have already included them in the error message.

So, this PR is a step in the right direction, but it didn't take all the steps yet.

[SeanCurtis-TRI]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @mitiguy)

---

multibody/tree/rotational_inertia.cc line 225 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

One thing we talked about on the whiteboard was that this function would get digested into this function.

The reasoning is as follows:

1. This test currently has information about invalidity -- i.e., non-positive values, non-triangle inequality.
2. In the future, you were recommending a cascading sequence of tests starting as close to the actual values in memory before operating on a transformed value.

None of that information is available to the caller. We'd discussed moving three functions in to the one report and this was one of them.

That need not be the only solution. This could likewise return a std::optional<string> (nullopt for valid, non-null with information if there was a problem). And the caller could make use of that. Furthermore, the caller wouldn't have to blindly recompute the principle moments again, because if they were relevant, the AreMomentsOfInertia....() function would have already included them in the error message.

So, this PR is a step in the right direction, but it didn't take all the steps yet.

The phrase "this function would get digested into this function" is horrible. I'll try again.

The function AreMomentsOfInertia....() would disappear and its contents would become part of this.

(And with that correction, all the rest of the previous content might make more sense.)