[meshcat] Add realtime rate display to MeshcatVisualizer

[pathammer]

This commit adds a stip chart displaying Drake simulator RTR in the meshcat visualizer web page. We depend on Stats.js to draw the chart

.

Edited to add: relates to issue #16486.

---

This change is 

[jwnimmer-tri]

@drake-jenkins-bot ok to test

[svenevs]

FYI I was in the middle of a focal image upgrade on EC2, your jobs are now running 👍

[jwnimmer-tri]

Thanks for working on this!

+@jwnimmer-tri to begin review (for the build system parts, at least). It's fine if this isn't ready for review yet, you can set aside any comments I'll be posting until this is fully ready.

I'll mark this +(release notes: yes) so that it will appear in the notes for whichever release this first appears in.

[jwnimmer-tri]

Thanks for working on this!

I will open various discussion threads below, using the Reviewable interface. We definitely recommend using the Reviewable link (the purpose button at the top of the pull request) to discuss, rather than the GitHub interface.

A few quick tips for using Reviewable:

• You can draft replies to multiple discussions, but wait to bulk-publish them all as a batch (via the green button atop the screen). Even though each individual discussion has the option to immediately publish the single reply, we generally don't use that. Pushing a whole tranche of replies at once cuts down on the notification load.
• It's fine to push code updates for only some of the discussions; no need to fix everything in a single round. In that case, you only need to reply to discussion threads where something has changed. There's no particular need to reply with "working on it" or "agreed"; you can merely leave the discussion open, your reviewer will assume that it's still in-progress, while the discussion thread remains open.
• For a discussion I've marked with "nit", if you push a fix then you can just click "Resolve" with no reply. For other discussions, typically you'd reply with "Done." after the fix (or sometimes, ask a follow-up question instead).

For any of the discussions, if you would prefer that I push a commit with the fix, just let me know.

Besides what I've posted below, I also have a few open questions that I'm still investigating -- so there might be a few more discussions opened a bit later, once I've learned a bit more.

Reviewed 10 of 15 files at r1, 5 of 6 files at r2, all commit messages.
Reviewable status: 21 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @pathammer)

a discussion (no related file):
While running this locally and reading the code, one thing I see is that the built-in Stats.js charts (for browser performance -- fps, ms, mem) are still enabled. (They can be seen by left-clicking on the chart.) Is that by design? In other words, do you expect meshcat users to want to examine some or all of those extra charts?

I could see a case for "rtr" reflecting the MeshcatVisualizer simulation performance while fps is reflecting both simulation performance also coupled with the Meshcat message bus performance. Is that your intention?

We'll need to clarify this in the documentation (possibly in a follow-up pull request), but before I can give any suggestions on that front, I need to understand the design goal.

---

a discussion (no related file):
As best I can tell, the rtr chart is shown unconditionally?

Some users of Drake use the Meshcat window to create simulation videos, and would not want to have the chart shown up in the video. We need some way to disable the chart, maybe as a Meshcat or MeshcatVisualizer API, or maybe via some web browser affordance like a controls panel toggle, or maybe just clicking on the rtr chart, or etc.

---

---

geometry/meshcat.h line 162 at r2 (raw file):

                 const Rgba& rgba = Rgba(.9, .9, .9, 1.));

  /** Sets the realtime that is displayed in the meshcat visualizer strip chart.

nit Possible typo?

Suggestion:

Sets the realtime rate that is displayed

---

geometry/meshcat.h line 164 at r2 (raw file):

  /** Sets the realtime that is displayed in the meshcat visualizer strip chart.
   * @param rate the realtime rate value to be displayed, will be converted to
   *             a percentage (multiplied by 100)

nit The prevailing style in this file is not to repeat the opening comment asterisk on subsequent lines. We should be consistent here.

Suggestion:

   @param rate the realtime rate value to be displayed, will be converted to
               a percentage (multiplied by 100)

---

geometry/meshcat.h line 166 at r2 (raw file):

   *             a percentage (multiplied by 100)
   */
  void SetRealtimeRate(const double& rate);

nit Per Drake's C++ style guide ("GSG"), we pass double parameters by-value.

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

Suggestion:

double rate

---

geometry/meshcat.h line 166 at r2 (raw file):

   *             a percentage (multiplied by 100)
   */
  void SetRealtimeRate(const double& rate);

All of the changes to this class require direct unit testing in drake/geometry/test/meshcat_test.cc or nearby. Minimally, we need to test that these new function calls do not crash.

---

geometry/meshcat.cc line 613 at r2 (raw file):

  }

  void SetRealtimeRate(const double& rate) {

nit Ditto to match the header file.

Suggestion:

double rate

---

geometry/meshcat.cc line 615 at r2 (raw file):

  void SetRealtimeRate(const double& rate) {
    DRAKE_DEMAND(IsThread(main_thread_id_));
    DRAKE_DEMAND(loop_ != nullptr);

I suspect this loop-check is a merge artifact. None of the other Impl functions have such a check anymore (it's recently been consolidated into the Defer() helper function).

---

geometry/meshcat.cc line 999 at r2 (raw file):

    });
  }

We should maintain a consistent layout of function ordering to the extent practical.

In both the header file and the non-Impl section at the bottom of this cc file, the SetRealtimeRate() function sits inbetween Delete() and SetProperty(). The new Impl::SetRealtimeRate() function definition should re-locate to this position in the file, for consistency.

---

geometry/meshcat.cc line 2035 at r2 (raw file):

}

void Meshcat::SetRealtimeRate(const double& rate) {

nit Ditto to match the header file.

Suggestion:

double rate

---

geometry/meshcat.html line 26 at r2 (raw file):

    } else {
      stats.showPanel(2);
    }

Trying to guess how many panels already existed seems like it's prone to bitrot over time, or to failure in different browser environments.

I think we can calculate the final panel index directly?

Suggestion:

    stats.showPanel(stats.dom.children.length - 1)

---

geometry/meshcat_types.h line 370 at r2 (raw file):

};

struct RealtimerateData {

A new developer reading over this code might be confused by a subtlety here.

All of the other structs in this file match up with meshcat.js data types (and the comment atop the file suggests as much). Since this struct is an outlier in that regard we should have a class comment here explaining the caveat.

Something along these lines:

// Note that this struct is unique to Drake's integration of meshcat; it is not
// part of upstream meshcat.js. We handle it directly within meshcat.html,
// without ever feeding it into meshcat.js.
struct RealtimerateData {

---

geometry/meshcat_types.h line 372 at r2 (raw file):

struct RealtimerateData {
  std::string type{"realtime_rate"};
  double rate;

nit Per Drake's C++ style guide ("GSG"), we require initialization of non-const POD member fields.

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

Suggestion:

  double rate{};

---

geometry/meshcat_visualizer.h line 218 at r2 (raw file):

  /* Calculate latest realtime rate and send to meshcat */
  void UpdateRealtimeRate(const double& sim_time) const;

nit https://drake.mit.edu/styleguide/cppguide.html#Pass_By_Value_Primitives

Suggestion:

double sim_time

---

geometry/meshcat_visualizer.h line 218 at r2 (raw file):

  /* Calculate latest realtime rate and send to meshcat */
  void UpdateRealtimeRate(const double& sim_time) const;

All of the changes to this class require direct unit testing in drake/geometry/test/meshcat_visualizer_test.cc. Minimally, we need to test that these new function calls do not crash.

---

geometry/meshcat_visualizer.h line 222 at r2 (raw file):

  mutable double prev_sim_time{0};
  mutable std::chrono::time_point<std::chrono::steady_clock> prev_wall_time{
      std::chrono::steady_clock::now()};

Class member field must always end with _ in the name.

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

Suggestion:

  mutable double prev_sim_time_{0};
  mutable std::chrono::time_point<std::chrono::steady_clock> prev_wall_time_{
      std::chrono::steady_clock::now()};

---

geometry/meshcat_visualizer.cc line 106 at r2 (raw file):

template <typename T>
void MeshcatVisualizer<T>::UpdateRealtimeRate(const double& sim_time) const {
  const auto current_wall_time = std::chrono::steady_clock::now();

I think the very first event here might lead to a spurious rtr reading?

The constructor initializes prev_wall_time member to now(), but in general there might be a long delay between constructing a simulation, and later starting to run it.

I think what we need here is to have prev_wall_time be null-ish upon construction (either a sentinel value, or optional<>), and then here during the first event, if the prev time was null-ish then the only thing we do is set it to now(), without updating the rtr chart.

---

geometry/meshcat_visualizer.cc line 110 at r2 (raw file):

      current_wall_time - prev_wall_time;
  if (const auto delta_wall_sec = delta_wall_dur.count()) {
    meshcat_->SetRealtimeRate((sim_time - prev_sim_time) / delta_wall_sec);

In the prior drake_visualizer implementation, we had a moving window filter to smooth out the reported rate.

I am OK to accept this change without any smoothing yet, but I'll want to add some smoothing down the road; the chart seems fairly noisy for my taste.

Thus, my question is whether it's important to you to not have any smoothing? If yes, then I'll need to preserve a configuration option to disable it (or use a very small window).

---

geometry/meshcat_visualizer.cc line 110 at r2 (raw file):

      current_wall_time - prev_wall_time;
  if (const auto delta_wall_sec = delta_wall_dur.count()) {
    meshcat_->SetRealtimeRate((sim_time - prev_sim_time) / delta_wall_sec);

In the prior drake_visualizer implementation, we had logic to detect time moving backwards (and thus, resetting the rtr). We will need that here as well.

In general, a user can make a simulator, run it for a while, reset the Context back to default values (including time == 0), and then resume running it again. When we detect a negative sim delta, we should not report a negative rtr in the chart.

Code quote:

sim_time - prev_sim_time

---

tools/workspace/default.bzl line 58 at r2 (raw file):

load("@drake//tools/workspace/meshcat:repository.bzl", "meshcat_repository")
load("@drake//tools/workspace/statsjs:repository.bzl", "statsjs_repository")
load("@drake//tools/workspace/msgpack_lite_js:repository.bzl", "msgpack_lite_js_repository")  # noqa

nit For these two new lines as well the corresponding lines below, please keep the lists of externals alphabetically sorted. (We don't have a linter to detect this yet, nor tool to auto-fix it; it's a manual process.)

---

tools/workspace/statsjs/repository.bzl line 12 at r2 (raw file):

        name = name,
        repository = "mrdoob/stats.js",
        commit = "5f0f917354ea2184456e5e361da5df6166306fbf",

This git commit id seems to be the r17 release (the most recent release, though dated from 2016), which lacks the bugfix in mrdoob/stats.js#79, for example.

Can we use the the latest revision from the master branch (1ecb62cd10f30789b540dcdbbd473f1de6eed614) instead?

In general, we try to use the latest revision for third-party modules across the board. If the package has a regular release cadence, we'll use their official tagged releases; but when they do not provide up-to-date releases, we tend to directly use their main branch instead.

[pathammer]

Reviewable status: 13 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 @jwnimmer-tri and @pathammer)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

While running this locally and reading the code, one thing I see is that the built-in Stats.js charts (for browser performance -- fps, ms, mem) are still enabled. (They can be seen by left-clicking on the chart.) Is that by design? In other words, do you expect meshcat users to want to examine some or all of those extra charts?

I could see a case for "rtr" reflecting the MeshcatVisualizer simulation performance while fps is reflecting both simulation performance also coupled with the Meshcat message bus performance. Is that your intention?

We'll need to clarify this in the documentation (possibly in a follow-up pull request), but before I can give any suggestions on that front, I need to understand the design goal.

The remaining charts are typical for debugging ThreeJS renderers. I think it's useful to know the other stats as a health check of the visualizer. Since the rendering code is outside Drake's scope, I'm open to disabling them if that seems appropriate.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

As best I can tell, the rtr chart is shown unconditionally?

Some users of Drake use the Meshcat window to create simulation videos, and would not want to have the chart shown up in the video. We need some way to disable the chart, maybe as a Meshcat or MeshcatVisualizer API, or maybe via some web browser affordance like a controls panel toggle, or maybe just clicking on the rtr chart, or etc.

Done. I added a flag to MeshcatParams to hide the plot.

---

---

geometry/meshcat.h line 162 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Possible typo?

Done

---

geometry/meshcat.h line 164 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The prevailing style in this file is not to repeat the opening comment asterisk on subsequent lines. We should be consistent here.

Done

---

geometry/meshcat.h line 166 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Per Drake's C++ style guide ("GSG"), we pass double parameters by-value.

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

Done

---

geometry/meshcat.h line 166 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

All of the changes to this class require direct unit testing in drake/geometry/test/meshcat_test.cc or nearby. Minimally, we need to test that these new function calls do not crash.

Done

---

geometry/meshcat.cc line 613 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Ditto to match the header file.

Done

---

geometry/meshcat.cc line 615 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I suspect this loop-check is a merge artifact. None of the other Impl functions have such a check anymore (it's recently been consolidated into the Defer() helper function).

Done

---

geometry/meshcat.cc line 999 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We should maintain a consistent layout of function ordering to the extent practical.

In both the header file and the non-Impl section at the bottom of this cc file, the SetRealtimeRate() function sits inbetween Delete() and SetProperty(). The new Impl::SetRealtimeRate() function definition should re-locate to this position in the file, for consistency.

Done

---

geometry/meshcat.cc line 2035 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Ditto to match the header file.

Done

---

geometry/meshcat.html line 26 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Trying to guess how many panels already existed seems like it's prone to bitrot over time, or to failure in different browser environments.

I think we can calculate the final panel index directly?

Done. Great suggestion!

---

geometry/meshcat_types.h line 370 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

A new developer reading over this code might be confused by a subtlety here.

All of the other structs in this file match up with meshcat.js data types (and the comment atop the file suggests as much). Since this struct is an outlier in that regard we should have a class comment here explaining the caveat.

Something along these lines:

// Note that this struct is unique to Drake's integration of meshcat; it is not
// part of upstream meshcat.js. We handle it directly within meshcat.html,
// without ever feeding it into meshcat.js.
struct RealtimerateData {

Done

---

geometry/meshcat_types.h line 372 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Per Drake's C++ style guide ("GSG"), we require initialization of non-const POD member fields.

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

Done

---

geometry/meshcat_visualizer.h line 218 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit https://drake.mit.edu/styleguide/cppguide.html#Pass_By_Value_Primitives

Done

---

geometry/meshcat_visualizer.h line 218 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

All of the changes to this class require direct unit testing in drake/geometry/test/meshcat_visualizer_test.cc. Minimally, we need to test that these new function calls do not crash.

Done. I moved the realtime rate calculation logic into a separate class to test in isolation.

---

geometry/meshcat_visualizer.h line 222 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Class member field must always end with _ in the name.

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

Done

---

geometry/meshcat_visualizer.cc line 106 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think the very first event here might lead to a spurious rtr reading?

The constructor initializes prev_wall_time member to now(), but in general there might be a long delay between constructing a simulation, and later starting to run it.

I think what we need here is to have prev_wall_time be null-ish upon construction (either a sentinel value, or optional<>), and then here during the first event, if the prev time was null-ish then the only thing we do is set it to now(), without updating the rtr chart.

Good catch. I initialize previous time now on the first call to calculate RTR

---

geometry/meshcat_visualizer.cc line 110 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In the prior drake_visualizer implementation, we had a moving window filter to smooth out the reported rate.

I am OK to accept this change without any smoothing yet, but I'll want to add some smoothing down the road; the chart seems fairly noisy for my taste.

Thus, my question is whether it's important to you to not have any smoothing? If yes, then I'll need to preserve a configuration option to disable it (or use a very small window).

I agree that smoothing would make sense. I suggest we add it in a follow on PR.

---

geometry/meshcat_visualizer.cc line 110 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In the prior drake_visualizer implementation, we had logic to detect time moving backwards (and thus, resetting the rtr). We will need that here as well.

In general, a user can make a simulator, run it for a while, reset the Context back to default values (including time == 0), and then resume running it again. When we detect a negative sim delta, we should not report a negative rtr in the chart.

Done. The InstantaneousRealtimeRateCalculator class handles this now and is unit tested.

---

tools/workspace/default.bzl line 58 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit For these two new lines as well the corresponding lines below, please keep the lists of externals alphabetically sorted. (We don't have a linter to detect this yet, nor tool to auto-fix it; it's a manual process.)

Done

[jwnimmer-tri]

I've resolved many of the threads now. I'll circle and review the updated code soon.

Reviewed 1 of 17 files at r3, 1 of 7 files at r4, all commit messages.
Reviewable status: 3 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 @jwnimmer-tri and @pathammer)

a discussion (no related file):

Previously, pathammer (Patrick Hammer) wrote…

The remaining charts are typical for debugging ThreeJS renderers. I think it's useful to know the other stats as a health check of the visualizer. Since the rendering code is outside Drake's scope, I'm open to disabling them if that seems appropriate.

Great, then I agree they are worth keeping.

My main worry is whether users of Drake's MeshCat will understand what they are seeing in the charts. I'm thinking about things like a hover-tooltip over the charts, or an explanation in the MeshcatVisualizer API class overview prose.

In any case, making it more clear could be a follow-up, instead of a blocker for pull request. I'll mark this resolved for now.

---

---

tools/workspace/default.bzl line 219 at r4 (raw file):

    if "meshcat" not in excludes:
        meshcat_repository(name = "meshcat", mirrors = mirrors)
    if "statsjs" not in excludes:

nit Both of these two new stanzas need alpha-sorting (within this function).

[pathammer]

Reviewable status: 2 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 @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Great, then I agree they are worth keeping.

My main worry is whether users of Drake's MeshCat will understand what they are seeing in the charts. I'm thinking about things like a hover-tooltip over the charts, or an explanation in the MeshcatVisualizer API class overview prose.

In any case, making it more clear could be a follow-up, instead of a blocker for pull request. I'll mark this resolved for now.

That makes sense. To review, that leaves two UI cleanup tasks as follow on - filtering the RTR signal and adding tooltips to summarize each plot.

---

---

tools/workspace/default.bzl line 219 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Both of these two new stanzas need alpha-sorting (within this function).

Done

---

tools/workspace/statsjs/repository.bzl line 12 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This git commit id seems to be the r17 release (the most recent release, though dated from 2016), which lacks the bugfix in mrdoob/stats.js#79, for example.

Can we use the the latest revision from the master branch (1ecb62cd10f30789b540dcdbbd473f1de6eed614) instead?

In general, we try to use the latest revision for third-party modules across the board. If the package has a regular release cadence, we'll use their official tagged releases; but when they do not provide up-to-date releases, we tend to directly use their main branch instead.

Done

[jwnimmer-tri]

Reviewed 8 of 17 files at r3, 6 of 7 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 25 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 @jwnimmer-tri and @pathammer)

a discussion (no related file):

Previously, pathammer (Patrick Hammer) wrote…

That makes sense. To review, that leaves two UI cleanup tasks as follow on - filtering the RTR signal and adding tooltips to summarize each plot.

SGTM.

---

---

common/BUILD.bazel line 604 at r5 (raw file):

drake_cc_library(
    name = "time",

nit Nominally, the library name and h/cc basename would be identical.

There might be thought to having a :time compound library with multiple header components, but my preference would be to defer that aggregation until we need it.

Note that this also affects the unit test label name and filename.

Suggestion:

timer

---

common/timer.h line 6 at r5 (raw file):

/// @file
/// Provides drake::Timer interface and drake::SteadyTimer for timing events

nit Missing punctuation at the end of a full sentence.

https://drake.mit.edu/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

(We often abbreviate this review comment as just "punc" during reviews.)

Suggestion:

for timing events.

---

common/timer.h line 10 at r5 (raw file):

namespace drake {

/// Abstract base class for timing utility.

In the member function documentation (and/or possibly the class overview), we need to be more clear about the semantics of a "running" timer. Does this class model the "is running" concept at all, or does it only model "most recent start time"?

For example, the outcome of doing Start twice in a row should be made clear. One option would be to throw upon the second one ("error: already started") but probably kinder would be to just restart the timer, without any hassle. That's how it's used in the rate calculator, in any case.

Similarly, for Start + Stop + Stop, what is the behavior of the second Stop? Does it error out, return zero, or return the same as the first Stop?

Similarly, for Stop + Tick, is the tick zero, or an error?

(... later ...)

After reading more of the pull request, I wonder if we need Stop at all? It seems unnecessary, and would clean up the semantics here if the timer was always "running". Users could do optional<Timer> in case they wanted to model a stoppable timer.

---

common/timer.h line 11 at r5 (raw file):

/// Abstract base class for timing utility.
struct Timer {

nit This needs a DRAKE_NO_COPY_... macro (or similar).

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

Ditto for the SteadyTimer subclass.

---

common/timer.h line 12 at r5 (raw file):

/// Abstract base class for timing utility.
struct Timer {
  using duration = std::chrono::duration<double>;

I am of two minds here.

On the one hand, std::chrono::duration<double> very clearly communicates the units that we're using (seconds), and highlights that this is a time difference rather than absolute time.

On the other hand, #include <chrono> is one of the worst standard library headers in terms of impact on compilation times (https://github.com/s9w/cpp-lit). It's really kind of a nightmare.

Since this new interface class is a common abstraction likely to be used widely with Drake, and anything we choose here as a return type will be locked in for a long time, I'm wondering if we should change the return type of this functions to be just a plain double, and rely on documentation to express the units and semantics? Do you think that would make the code inordinately difficult to maintain?

In order to fully evict <chrono> the SteadyTimer would either need to move into a separate h/cc file pair, or else switch to using a pimpl or similar. That could be a TODO comment, though. Setting the API signature correct is the important point for now.

---

common/timer.h line 14 at r5 (raw file):

  using duration = std::chrono::duration<double>;
  virtual ~Timer() = default;
  /// begin timing

nit Wrong "verb mood" (and missing capitalization & punctuation on a sentence).

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

https://drake.mit.edu/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar

Ditto on "Stop", immediately below.

Suggestion:

Begins timing.

---

common/timer.h line 17 at r5 (raw file):

  virtual void Start() = 0;
  /// stop timing
  /// @return the ammount of time since the timer started

nit typo

Same typo immediately below.

Code quote:

ammount

---

common/timer.h line 19 at r5 (raw file):

  /// @return the ammount of time since the timer started
  virtual duration Stop() = 0;
  /// obtain multiple measurements from the same baseline

nit cap,punc

Suggestion:

/// Obtains multiple measurements from the same baseline.

---

common/timer.h line 25 at r5 (raw file):

/// Implementation of timing utility that uses monotonic
/// std::chrono::steady_clock

nit punc

Suggestion:

std::chrono::steady_clock.

---

common/timer.h line 29 at r5 (raw file):

 public:
  using clock = std::chrono::steady_clock;
  void Start() override { start_time = clock::now(); }

It might be plausible that clock::now() is cheap enough to inline, but my default approach would be to define this function in the cc file, since it's unlikely that we are gaining any meaningful improvement via inlining, since the call is almost always made virtually through the base class.

Ditto for the moving constructor's definition into the cc file (for the member field now() initialization).

I'm happy to hear evidence or discussion to the contrary, though.

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

---

common/test/time_test.cc line 9 at r5 (raw file):

  std::unique_ptr<Timer> timer = std::make_unique<SteadyTimer>();
  timer->Start();
  // Assert that some positive time has passed

nit punc

Ditto below.

---

common/test/time_test.cc line 10 at r5 (raw file):

  timer->Start();
  // Assert that some positive time has passed
  EXPECT_GT(timer->Tick().count(), 0.0);

I think it would also be worth testing that when calling Tick twice in a row, the second tick is greater than the first tick? That seems like the important distinction between Tick vs Stop.

---

common/test/time_test.cc line 14 at r5 (raw file):

}

GTEST_TEST(TimeTest, ConstructorStartsTimer) {

Aha! I did not realize from the header that "start the timer immediately upon construction" is the contract. The API documentation should say that.

---

geometry/meshcat.h line 52 at r4 (raw file):

  /** Determines whether or not to display the stats plot including realtime
   * rate display. */

nit The prevailing style in this file is not to repeat the opening comment asterisk on subsequent lines. We should be consistent here.

Suggestion:

  /** Determines whether or not to display the stats plot including realtime
  rate display. */

---

geometry/meshcat.h line 53 at r5 (raw file):

  /** Determines whether or not to display the stats plot including realtime
   * rate display. */
  bool hide_stats_plot{false};

It's somewhat of a mix between personal preference and style rule, but I tend to push back against options that are phrased in the negative, from a readability perspective.

At call sites when we see hide_stats_plot = true it's fine, but where we see hide_stats_plot = false the reader has to go through a mental dance to figure out what's going on.

On the contrary, show_stats_plot = true and show_stats_plot = false are both easily readable without a hiccup.

Ditto throughout the new message-struct name, etc.

Suggestion:

bool show_stats_plot{true};

---

geometry/meshcat.cc line 1688 at r5 (raw file):

    // Tell client if the realtime rate plot should be hidden
    internal::HideRealtimeRate rtr_hidden_msg{};

nit "msg" is not a permitted form of abbreviation:

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

(Note that it's fine when done for compatibility with third-party api like saying msgpack:: in names.)

Code quote:

_msg

---

geometry/meshcat.html line 20 at r5 (raw file):

    var stats = new Stats();
    var rtrPanel = stats.addPanel( new Stats.Panel( 'rtr%', '#ff8', '#221' ) );
    document.body.appendChild( stats.dom );

If a user is using Meshcat but not MeshcatVisualizer, does this still display the charts? If so, we might need the rate-toggle message from MeshcatVisualizer to be opt-in, instead of opt-out.

(I dont' remember if the chart shows up from the start, or only once it gets its first rate report.)

---

geometry/meshcat_visualizer.h line 224 at r5 (raw file):

  bool set_transforms_while_recording_{true};

  mutable systems::InstantaneousRealtimeRateCalculator rtr_calculator_{};

Abbreviating "realtime rate" by omitting letters from the middle of a word is not allowed.

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

Code quote:

rtr

---

geometry/meshcat_visualizer.cc line 100 at r5 (raw file):

  }
  SetTransforms(context, query_object);
  std::optional<double> rtr = rtr_calculator_.CalculateRealtimeRate(

nit Ditto on GSG-invalid abbreviation.

Code quote:

rtr

---

geometry/meshcat_visualizer.cc line 193 at r5 (raw file):

  return systems::EventStatus::Succeeded();
}

nit A spurious blank line snuck in here.

---

systems/analysis/instantaneous_realtime_rate_calculator.h line 11 at r5 (raw file):

namespace systems {
/// Utility class that computes the realtime rate achieved between time steps.
class InstantaneousRealtimeRateCalculator {

nit Missing DRAKE_NO_COPY... or similar.

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

---

systems/analysis/instantaneous_realtime_rate_calculator.h line 22 at r5 (raw file):

  std::optional<double> CalculateRealtimeRate(double current_sim_time);

  /* (Internal use for unit testing only) Used to mock the monatomic wall time

typo

Suggestion:

monotonic

---

systems/analysis/instantaneous_realtime_rate_calculator.cc line 6 at r5 (raw file):

std::optional<double>
drake::systems::InstantaneousRealtimeRateCalculator::CalculateRealtimeRate(

nit Our custom is to place the definitions with namespace ... blocks, instead of writing out the qualifiers throughout.

---

systems/analysis/instantaneous_realtime_rate_calculator.cc line 12 at r5 (raw file):

    const double wall_delta{timer_->Tick().count()};
    const double sim_time_delta{current_sim_time - prev_sim_time_.value()};
    // avoid divide by zero and negative RTR

nit cap,punc

Also, eschew abbreviations / initialisms
https://drake.mit.edu/styleguide/cppguide.html#General_Naming_Rules

Suggestion:

Avoid divide by zero and negative rate.

---

systems/analysis/test/instantaneous_realtime_rate_calculator_test.cc line 23 at r5 (raw file):

GTEST_TEST(InstantaneousRealtimeRateCalculatorTest, BasicTest) {
  auto timer = std::make_unique<MockTimer>();
  const double SLEEP_S = 0.1;

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

Suggestion:

kSleepSec

[jwnimmer-tri]

feature review, modulo the open discussion threads.

Reviewed 1 of 17 files at r3.
Reviewable status: 27 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @pathammer)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

SGTM.

FYI I just added these as a checklist into #16486.

---

---

geometry/meshcat.cc line 1407 at r2 (raw file):

        {" src=\"meshcat.js\"", "/meshcat.js"},
        {" src=\"stats.min.js\"", "/stats.min.js"},
        {" src=\"msgpack.min.js\"", "/msgpack.min.js"},

@RussTedrake as the PR comes closer to the finish line, I'm going to ask you to test whether the Meshcat::StaticHtml function still meets your needs. I am slightly worried that the changes here might disrupt that workflow in a way that I wouldn't notice during my own local testing.

---

geometry/meshcat.html line 66 at r2 (raw file):

      url = url.replace("/index.html", "/")
      url = url.replace("/meshcat.html", "/")
      connection = new WebSocket(url);

The way that we're defining new commands that are outside of the scope of meshcat.js here is somewhat awkward. I think it's fine as a stepping stone, but I'd like to discuss here and come up with a TODO comment that we can get into this pull request, to resolve down the road.

For one, we're manually pulling in msgpack-lite ourselves, even though it's already in the meshcat.js bundle (obscured). That's a second copy for the user to download, and also means we either need to keep our workspace-pinned version in sync with meshcat's pinned version manually, or else risk the two diverging possibly leading to unexpected behavior.

I also worry that the secret sauce here (binaryType, decode UInt8Array) that we've minim'd from meshcat might evolve in meshcat.js and we'd be left behind.

I think the TODO that I'm probably looking for is something like "TODO(jwnimmer-tri) Find a way to add custom .type handlers to meshcat's parsing and dispatch, so we can go back to using it's built-in message handler."

I wonder if it's possible to monkey-patch viewer.handle_command instead with our little hook, instead of what's here now?

Or else, it might make sense to go upstream to meshcat itself and ask for an API to register custom callbacks. That would probably be most durable.

What to you think?

I'd like to see the TODO somewhere in this file, to interested people will see it while poking around. Maybe somewhere near function handle_message above?

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @pathammer)

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The original test did that. However, the test couldn't actually confirm that the Start() method actually did anything. My original point was that the test would (and did) pass if Start() were a no-op.

I had some fear about sleeping and thread scheduling but had hopes that, even under load, the thread would still get back in time (hence the order of magnitude difference in sleep time).

Do you have an alternate solution for confirming that Start() actually works? For loops of two different sizes so that we just burn through cycles without surrendering execution? Something else?

I think I have a solution:

In timer.cc change the constructor to:

SteadyTimer::SteadyTimer() {
  Start();
}

Then, if the timer starts running upon construction, we know Start() does something. The test can explicitly say that it relies on the fact that the constructor calls Start() for the tests's correctness. Then we can throw out the whole of chrono and thread in this test.

---

common/test/timer_test.cc line 24 at r13 (raw file):

  const double t = timer.Tick();
  EXPECT_GT(t, 0.0);
  EXPECT_GE(t, std::chrono::duration<double>(kShortInterval).count());

nit: This can/should simply be kShortInterval.count().

This seems to return the test to its original state. If I run this test with Start()'s implementation emptied out. The test passes. We can return to its original state and achieve the same thing.

When a timer starts automatically, it's difficult to detect the efficacy of Start().

See the other thread around here for my alternate proposal.

[jwnimmer-tri]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @pathammer and @SeanCurtis-TRI)

---

-- commits line 1 at r9:

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

We'll probably also mention the inclusion of new dependencies. :) But this is the nice functional core.

Just to be clear, only statjs is a new dependency. The msgpack-like was already a (transitive) dependency.

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I think I have a solution:

In timer.cc change the constructor to:

SteadyTimer::SteadyTimer() {
  Start();
}

Then, if the timer starts running upon construction, we know Start() does something. The test can explicitly say that it relies on the fact that the constructor calls Start() for the tests's correctness. Then we can throw out the whole of chrono and thread in this test.

Something like this would work?

Timer dut;
sleep(/* 1000 ms */);
const tick1 = dut.Tick();
dut.Start();
const tick2 = dut.Tick();
EXPECT_LT(tick2, tick1);

I suppose that's approximately what's was there in r11 that I defected, except that there is no "short sleep" interval. But that's a big difference! Going to sleep for 10 ms is exactly the kind of the that the kernel will ding you for. Being de-scheduled as we were reading the steady clock twice in a row seems vanishingly unlikely.

Or, the other way to test this? Agree that the implementation under test 1 line of code so we don't need to care so much. One way to double-down on that idea would be to change the constructor to call Start(), instead of manually inlining it. Then as long as the constructor does something we know (via glass-box testing) that Start is a-ok.

[SeanCurtis-TRI]

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Something like this would work?

Timer dut;
sleep(/* 1000 ms */);
const tick1 = dut.Tick();
dut.Start();
const tick2 = dut.Tick();
EXPECT_LT(tick2, tick1);

I suppose that's approximately what's was there in r11 that I defected, except that there is no "short sleep" interval. But that's a big difference! Going to sleep for 10 ms is exactly the kind of the that the kernel will ding you for. Being de-scheduled as we were reading the steady clock twice in a row seems vanishingly unlikely.

Or, the other way to test this? Agree that the implementation under test 1 line of code so we don't need to care so much. One way to double-down on that idea would be to change the constructor to call Start(), instead of manually inlining it. Then as long as the constructor does something we know (via glass-box testing) that Start is a-ok.

Heh...I beat you by three minutes. :)

[pathammer]

Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri, @pathammer, and @SeanCurtis-TRI)

---

geometry/meshcat_visualizer.cc line 104 at r9 (raw file):

Previously, pathammer (Patrick Hammer) wrote…

I moved to meshcat.h which I think is a more generic home for it and there already other TODO comments in that header.

Done.

---

systems/analysis/instantaneous_realtime_rate_calculator.h line 23 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Yeah. I'd thrown out "realtime" but left in "rate". I like your suggestion.

Done.

[SeanCurtis-TRI]

Reviewed 7 of 7 files at r14, 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) (waiting on @jwnimmer-tri and @pathammer)

---

-- commits line 1 at r9:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Just to be clear, only statjs is a new dependency. The msgpack-like was already a (transitive) dependency.

Good to know.

[pathammer]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @pathammer)

---

geometry/meshcat.h line 52 at r12 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Your IDE is probably conspiring against you to introduce the leading * in your comments. We should remove them so the documentation format is consistent.

Done.

[pathammer]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri, @pathammer, and @SeanCurtis-TRI)

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Heh...I beat you by three minutes. :)

Done. I added both the ctor change and made test improvements.

[jwnimmer-tri]

Reviewed 6 of 7 files at r14, 1 of 1 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @pathammer)

[jwnimmer-tri]

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

---

geometry/meshcat.cc line 1407 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@RussTedrake as the PR comes closer to the finish line, I'm going to ask you to test whether the Meshcat::StaticHtml function still meets your needs. I am slightly worried that the changes here might disrupt that workflow in a way that I wouldn't notice during my own local testing.

Withdrawn (no response).

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @pathammer)

---

common/timer.h line 16 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Then it should be expressed as guidance and not as a fact. Sadly, I can't think of a way to enforce this -- the whole "don't call virtual methods from constructors" thing.

I'd put:

/// Properly implemented Timers start timing upon construction.

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, pathammer (Patrick Hammer) wrote…

Done. I added both the ctor change and made test improvements.

I think part of the point of the ctor change is to trivialize the test. We don't have to play sleep games. The fact that the timer is running from the beginning is proof that Start() works.

So, we can even return to a simpler version of your original test:

GTEST_TEST(TimeTest, Everything) {
  SteadyTimer timer;

  // Clock starts upon construction (via calling Start()). A positive value is
  // proof of a running clock. This proves two things: 1) Start() works and
  // 2) constructor is calling it.
  const double T1 = timer.Tick();
  EXPECT_GT(T1, 0.0);

  // Subsequent calls show monotonic increases in time.
  const double T2 = timer.Tick();
  EXPECT_LT(T1, T2);

  EXPECT_GT(timer.Tick(), T2);
}

[pathammer]

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

---

common/timer.h line 16 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd put:

/// Properly implemented Timers start timing upon construction.

Done.

---

common/test/timer_test.cc line 3 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

A thought on the test.

Start() could be a no-op and this test would pass. You've already established that calling timer.Tick() after construction reports a positive time interval. So, if Start() did nothing, this test would still pass. Ideally, we would want a test that would detect whether Start() is a no op.

To that end, I cobbled up a version of the test that attempts to do that. (This single test would replace the two tests currently defined.)

#include <chrono>
#include <thread>

GTEST_TEST(TimeTest, Everything) {
  using std::chrono::duration;
  const duration<double> kLongInterval = std::chrono::milliseconds(100);
  const duration<double> kShortInterval = std::chrono::milliseconds(10);

  SteadyTimer timer;

  // Clock starts upon construction. Allow some time to pass and confirm
  // positive measurement.
  std::this_thread::sleep_for(kLongInterval);
  const double T1 = timer.Tick();
  EXPECT_GT(T1, 0.0);

  // Start resets the clock. Allow a smaller time to pass and confirm positive
  // measurement that is less than T1.
  timer.Start();
  std::this_thread::sleep_for(kShortInterval);
  const double T2 = timer.Tick();
  EXPECT_GT(T2, 0.0);
  EXPECT_LT(T2, T1);

  // As time progresses, the tick value monotonically increases.
  EXPECT_GT(timer.Tick(), T2);
}

Fixed as discussed

[jwnimmer-tri]

Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I think part of the point of the ctor change is to trivialize the test. We don't have to play sleep games. The fact that the timer is running from the beginning is proof that Start() works.

So, we can even return to a simpler version of your original test:

GTEST_TEST(TimeTest, Everything) {
  SteadyTimer timer;

  // Clock starts upon construction (via calling Start()). A positive value is
  // proof of a running clock. This proves two things: 1) Start() works and
  // 2) constructor is calling it.
  const double T1 = timer.Tick();
  EXPECT_GT(T1, 0.0);

  // Subsequent calls show monotonic increases in time.
  const double T2 = timer.Tick();
  EXPECT_LT(T1, T2);

  EXPECT_GT(timer.Tick(), T2);
}

For my part, I am fine with the test as it stands now. I think the CI hazard is low. I'll leave it up to the two of you to decide.

[jwnimmer-tri]

At the finish line now, we'll double-check that everything is still happy for macOS:

@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please

[SeanCurtis-TRI]

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

---

common/test/timer_test.cc line 28 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For my part, I am fine with the test as it stands now. I think the CI hazard is low. I'll leave it up to the two of you to decide.

I will likewise mark myself as satisfied (with a vague bias in favor of the most concise test, of course).

Either way, we should defer any changes until the mac test finishes. :)

[pathammer]

It appears the macOS build failed with messages about /statsjs/build/stats.min.js missing.

I don't know much about the macOS build so any advice on how to fix would be greatly appriciated.

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @pathammer)

[jwnimmer-tri]

Thanks, we'll take a look.

For the record, here's the error message (it's a bit buried in the Jenkins output):

[2022-07-07T22:44:52.817Z] ERROR: geometry/BUILD.bazel:395:8: Executing genrule //geometry:stats_js_genrule failed: missing input file 'external/statsjs/build/stats.min.js', owner: '@statsjs//:build/stats.min.js'

[jwnimmer-tri]

Wow, crazy times. I had to run on a local macOS test machine to figure this one out. Here's the defect in current Drake (not new in this PR):

drake/tools/workspace/github.bzl

Lines 175 to 179 in 855e1b8

	if repository_ctx.attr.build_file:
	for name in ["BUILD", "BUILD.bazel"]:
	if repository_ctx.path(name).exists:
	repository_ctx.execute(["/bin/mv", name, name + ".ignored"])
	repository_ctx.symlink(repository_ctx.attr.build_file, "BUILD.bazel")

It's because macOS is a case-insensitive filesystem, and we mistake the directory named build for a file named BUILD (as-if the upstream package had Bazel support natively), and so move the directory out of the way to make room for our replacment file package.BUILD.bazel.

I'll work up and fix and push it into this PR we'll re-test this after it merges. Stay tuned.

[jwnimmer-tri]

@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please