Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[workspace] Upgrade meshcat to latest commit #20904

Merged

Conversation

mwoehlke-kitware
Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware commented Feb 7, 2024

This change is Reviewable

@mwoehlke-kitware mwoehlke-kitware added the release notes: fix This pull request contains fixes (no new features) label Feb 7, 2024
@mwoehlke-kitware
Copy link
Contributor Author

Someone else should perform the full test, as I skipped the XR/AR and gamepad portions. Here are my results/observations:

The blue box should spin clockwise about the +z axis.

The blue box, as viewed from above (which is closest to the camera position set by the script), spins counterclockwise.

The lights should have dimmed.

If anything, the light seems brighter. (I wasn't expecting a change, however, and have no way of directly comparing. Also, at this point I didn't realize that things don't always update until the camera is moved and I don't think I moved the camera before advancing to the next stage.)

An environment map has been loaded from a png.

It did, but was not visible until I rotated the camera.

The Cornell box has been replaced by a room with brick walls loaded from a jpg.

Again, the background was white until I moved the camera.

Reloading the page should always succeed.

Reloading caused Firefox to crash. 😒

Things seemed to be loading back to the previous state just before the crash, however. Also, after restarting, everything appears to be as it was before trying to reload. (I guess restarting the browser is a better 'did it reload' test than just a forced refresh, anyway...)

Click the ButtonTest button a few times.

This does not seem to do anything. (However, later the test reports that the buttons were clicked several times.)

Next, we'll test gamepad (i.e., joystick) features.

Besides that I don't have a handy gamepad, "Gamepads are not supported outside of a secure context".

Now we'll test the WebXR functionality.

Nope, not installing a German mystery extension.

@jwnimmer-tri
Copy link
Collaborator

Here are my results/observations: ...

It would be worth disambiguating which observations are new to this branch compared to master, versus which ones also happen on master. Things that are novel here deserve a close look, but things that also exist on master are probably latent problems that we haven't noticed yet, which could be filed and deal with aside from this PR.

@mwoehlke-kitware
Copy link
Contributor Author

For comparison, my results on master...

  • The blue box rotates in the same direction (which I would describe as "counterclockwise").
  • I suspect I'm seeing the same behavior with the "dimmer" lights. (See analysis, below.)
  • The Cornell Box background is not visible (I think it is not visible in the shiny box's reflections, either, although it's difficult to tell without moving the camera) until a repaint is forced (e.g. by moving the camera or resizing the browser). To be sure I wasn't just seeing lag, I waited over two full minutes.
  • Same with the bricks.
  • Considering my previous results, I did not test reloading.
  • Knowing what to expect, I can verify that the number of times the buttons are pressed is correctly recorded. As before, there was no immediate feedback.
  • I also did not retest results which were not previously anomalous, or which I didn't test previously.

With respect to the lights, this time I have prepared "before" and "after" screenshots. Here is the previous state:
Screenshot_20240208_121129

Here is the state after the lights became "dimmer":
Screenshot_20240208_121309

Careful pixel-peeping (and use of a color picker) shows that the lighting is, in fact, dimmer, however the effect is extremely subtle and the optical illusion of the lighter background makes it very hard to tell using only the Eyeball Mk-I.

Therefore, for the tests I did perform, it appears that the upgrade did not change anything.

@jwnimmer-tri jwnimmer-tri added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Feb 8, 2024
@jwnimmer-tri
Copy link
Collaborator

The sha bump here, even with manual testing involved, is still fine for just a single reviewer.

+@EricCousineau-TRI for both reviews per schedule, please.

@mwoehlke-kitware
Copy link
Contributor Author

@EricCousineau-TRI, reminder, please run the manual meshcat tests before approving. (Especially the gamepad/XR/AR parts.)

@SeanCurtis-TRI
Copy link
Contributor

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the shopping list:

  • blue box rotation: agreed, definitely counter clockwise. It's clear from the code that it is getting positive rotation around the Wz axis. So, perhaps it would be most precisely articulated thus.
  • dimming light: yep. While it unambiguously reduces the intensity of the ambient light, that is almost offset by the environment going white. When we added environment maps, that's the end result we got. I may have to revisit that logic in meshcat.
  • For the numerous occasions where reported visual changes weren't observed until something forced a redraw, while I very rarely experience that on chrome on my machine, I wonder if it's a more common problem in firefox. But we should definitely have a note saying, "Depending on your browser, the background may not redraw until you bump the camera." (or some such thing)

Otherwise: :LGTM: X2

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee SeanCurtis-TRI(platform)

@SeanCurtis-TRI SeanCurtis-TRI merged commit 199717d into RobotLocomotion:master Feb 8, 2024
10 checks passed
@mwoehlke-kitware mwoehlke-kitware deleted the upgrade-meshcat branch February 8, 2024 22:12
@mwoehlke-kitware
Copy link
Contributor Author

While it unambiguously reduces the intensity of the ambient light...

Yes, that was my impression also; the ambient light is (subtly) reduced, but the illumination where directional light is present is nearly unchanged. (Especially as those parts are mostly over-exposed.)

@SeanCurtis-TRI
Copy link
Contributor

RE: the environment. If the scene had only consisted of objects with a "certain kind of material", they would've become dimmer even as the background got lighter. But the green box presented front and center is most affected by the white background; it gets illuminated, spoiling the effect.

I've submitted an issue for meshcat and would love any and all thoughts:

meshcat-dev/meshcat#175

jwnimmer-tri pushed a commit to jwnimmer-tri/drake that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features) status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants