Skip to content

Conversation

@SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Oct 27, 2023

Note: This qualitatively changes the output renderings. Meshcat's rendered result has moved from linear-srgb to srgb. Things are generally less saturated now as part of three.js's move towards more realistic lighting.

We've amended meshcat.html to accommodate some of the fundamental changes:

  • Silence a console warning.
  • Beef up the lighting so that it's more aesthetically pleasing.

TODO: After the meshcat PR has merged, update repository.bzl to refer to
the correct upstream source.


This change is Reviewable

@SeanCurtis-TRI SeanCurtis-TRI added release notes: fix This pull request contains fixes (no new features) status: do not merge labels Oct 27, 2023
Copy link
Contributor Author

@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.

+@jwnimmer-tri for both reviews, please.

+(status: do not merge) until upstream meshcat PR merges.

+(release notes: fix)

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @SeanCurtis-TRI)

@SeanCurtis-TRI
Copy link
Contributor Author

Waiting on meshcat-dev/meshcat#155 to merge.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @SeanCurtis-TRI)


geometry/meshcat.html line 124 at r1 (raw file):

    // spotlight do. The default lighting is now dominated by the non-decaying
    // light sources. So, we're going to significantly bump the decaying sources
    // to give them illuminating power and increase image contrast.

What's the thinking with this being a Drake-specific fix? Previously, the default lighting was 100% an upstream property. Is there a true need to fork it now, where Drake must use non-default values?

Copy link
Contributor Author

@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.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @SeanCurtis-TRI)


geometry/meshcat.html line 124 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

What's the thinking with this being a Drake-specific fix? Previously, the default lighting was 100% an upstream property. Is there a true need to fork it now, where Drake must use non-default values?

A fair question. At the end of the day, I'm hard-pressed to provide a rigorous answer. But it rests on two things:

  1. The three.js documentation on migrating indicates that there is no mechanical path toward upgrading lighting. At the end of the day, point and spotlights are now decaying and they strong advocate not messing with that. The reintroduction of the lost factor is a starting point (which I've done in meshcat). But actually defining a meaningful, physical value is not.
  2. The values in this html file that (I think) look good in meshcat manual test (and other, typical drake applications) look horrible in meshcat's various test html files. So, I'm reluctant to export them broadly.

(And as we're already messing with the default gradient to not be meshcat's default, playing with the lights didn't seem particularly bad, either.)

@SeanCurtis-TRI
Copy link
Contributor Author

geometry/meshcat.html line 124 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

A fair question. At the end of the day, I'm hard-pressed to provide a rigorous answer. But it rests on two things:

  1. The three.js documentation on migrating indicates that there is no mechanical path toward upgrading lighting. At the end of the day, point and spotlights are now decaying and they strong advocate not messing with that. The reintroduction of the lost factor is a starting point (which I've done in meshcat). But actually defining a meaningful, physical value is not.
  2. The values in this html file that (I think) look good in meshcat manual test (and other, typical drake applications) look horrible in meshcat's various test html files. So, I'm reluctant to export them broadly.

(And as we're already messing with the default gradient to not be meshcat's default, playing with the lights didn't seem particularly bad, either.)

(Well, "horrible" in some cases.)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @SeanCurtis-TRI)


geometry/meshcat.html line 124 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

(Well, "horrible" in some cases.)

Okay, I buy (2). That justification should go into the comment here for future maintainers.

I pull this locally soon to run some manual tests.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

LGTM pending updated git sha.

I think it's worth getting one more reviewer, so +@xuchenhan-tri for platform review, please.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), labeled "do not merge" (waiting on @SeanCurtis-TRI)


geometry/meshcat.html line 113 at r1 (raw file):

    // days past.
    viewer.set_property(['Background', '<object>'], "top_color", [.95, .95, 1.0])
    viewer.set_property(['Background', '<object>'], "bottom_color", [.32, .32, .35])

nit Both of the prior lines are > 80 cols. Given that we've wrapped the next line to obey <= 80, it seems like we should be consistent. Ditto for the new light changes a dozen lines later.

Another way to be consistent would be to unwrap the line immediately below, and keep the rest the same.

Note: This qualitatively changes the output renderings. Meshcat's rendered
result has moved from linear-srgb to srgb. Things are generally less
saturated now as part of three.js's move towards more realistic lighting.

We've amended meshcat.html to accommodate some of the fundamental changes:
  - Silence a console warning.
  - Beef up the lighting so that it's more aesthetically pleasing.
Copy link
Contributor Author

@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.

Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), labeled "do not merge"

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Waiting on meshcat-dev/meshcat#155 to merge.

Merged and reference updated.

-(status: do not merge)


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

Copy link
Contributor

@xuchen-han xuchen-han left a comment

Choose a reason for hiding this comment

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

Platform :lgtm:

BTW, I ran meshcat_manual_test and didn't see any observable change. Is that expected?

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion


geometry/meshcat.html line 112 at r3 (raw file):

    // Set background to match the legacy ``drake_visualizer`` application of
    // days past.
    viewer.set_property(['Background', '<object>'], "top_color",

BTW, the '<object>' parameter is added to suppress the warning I presume?

Copy link
Contributor Author

@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.

Nothing that will jump out at you. Blue didn't become orange. But if you did a before/after comparison there are easily discernible differences. This is merely trying to minimize those differences in a meaningful way.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),xuchenhan-tri(platform)


geometry/meshcat.html line 112 at r3 (raw file):

Previously, xuchenhan-tri wrote…

BTW, the '<object>' parameter is added to suppress the warning I presume?

Correct. It updated a while ago such that you can reference background directly, but shouldn't and we'd never really gotten around to updating here.

@SeanCurtis-TRI SeanCurtis-TRI merged commit 40e116d into RobotLocomotion:master Oct 30, 2023
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_upgrade_meshcat branch October 30, 2023 17:36
WawasCode pushed a commit to WawasCode/drake that referenced this pull request Oct 31, 2023
Note: This qualitatively changes the output renderings. Meshcat's rendered
result has moved from linear-srgb to srgb. Things are generally less
saturated now as part of three.js's move towards more realistic lighting.

We've amended meshcat.html to accommodate some of the fundamental changes:
  - Silence a console warning.
  - Beef up the lighting so that it's more aesthetically pleasing.
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants