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

Accommodate new lighting and color management #155

Merged

Conversation

SeanCurtis-TRI
Copy link
Contributor

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

We've upgraded three.js from 0.149 to 0.156. This led to changes in how colors are managed and lighting calculations are performed. To account for those changes and give a qualitatively similar experience with the default values, we've made the following changes:

  • Light intensity controls now have units (candela) displayed.
  • The old (non physical) light intensity values have been boosted to be more or less equivalent to what they were before (see documentation in code).
  • The color space of the background images has been corrected (to sRGB).

This change is Reviewable

We've upgraded three.js from 0.149 to 0.156. This led to changes in how
colors are managed and lighting calculations are performed. To account for
those changes and give a *qualitatively* similar experience with the
default values, we've made the following changes:

 - Light intensity controls now have units (candela) displayed.
 - The old (non physical) light intensity values have been boosted to be
   more or less equivalent to what they were before.
 - The color space of the background images has been corrected (to sRGB).
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 review, unless you want to pass it along.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @jwnimmer-tri)

Copy link
Contributor

@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 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)


-- commits line 4 at r1:
nit Clarity

Suggestion:

We previously upgraded

@kielnino
Copy link

kielnino commented Mar 25, 2024

@jwnimmer-tri I would like to point out that this also affects the companion repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants