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

WebGLRenderer: .physicallyCorrectLights -> .useLegacyLights #24975

Merged
merged 4 commits into from
Feb 10, 2023

Conversation

WestLangley
Copy link
Collaborator

This is a first step in the transition to make physically-correct behavior the default.

For now, renderer.useLegacyLights defaults to true.

In a follow-up PR, and when it seems appropriate, the default value will be changed to false.

The docs will be updated shortly in a separate PR.

@WestLangley WestLangley added this to the r147 milestone Nov 17, 2022
Repository owner locked and limited conversation to collaborators Nov 17, 2022
Repository owner unlocked this conversation Nov 17, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 17, 2022

After the decay change I've tested physicallyCorrectLights = true as a default and verified the examples. Just wanted to note here that such a change produces a severe lighting updates in existing scenes.

We definitely need a guide (maybe in the forum) that explain users how to restore the lighting with physicallyCorrectLights = true or useLegacyLights = false.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 17, 2022

We definitely need a guide (maybe in the forum) that explain users how to restore the lighting...

Will need more explanation, but some references in:

This is a first step in the transition to make physically-correct behavior the default.

@WestLangley I'm not sure I understand the motivation here yet — if our goal is to change the default lighting model, and then remove the property entirely some time after that, what is the benefit of asking everyone to switch to a new name for the property during that transition? Is it that if people are switching back to the old mode during the transition, we want it to have a name that is more obviously not recommended long-term? We could also use a deprecation warning for that.

@WestLangley
Copy link
Collaborator Author

WebGPURenderer supports only the PBR lights.

WebGLRenderer supports both the legacy lights and the PBR lights. The change to the PBR lights is occurring now. The legacy lights will be supported for the foreseeable future.

This PR requires no change to the screenshots.

In a later PR, renderer.useLegacyLights will default to false, and 300+ examples will require new screenshots. That would be a good time for a user tutorial.

Whether that change occurs immediately (this release), or in a future release, is up to @mrdoob.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 17, 2022

In a later PR, renderer.useLegacyLights will default to false, and 300+ examples will require new screenshots.

Small addition: Each example has to be checked and updated when this change is going to be made. Without updates the lighting of many examples will look awful.

Besides, I also share concerns about renaming the property. TBH, it feels like an unnecessary change. I would prefer to keep the property physicallyCorrectLights, avoid any migration tasks now and set it at some point in the future to true. And some time later, the property can be completely removed.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 17, 2022

We are asking all users already on PBR lights to update their code, replacing...

renderer.physicallyCorrectLights = true;

... with ...

renderer.useLegacyLights = false;

What benefit do you see for imposing this migration cost?

The legacy lights will be supported for the foreseeable future.

My assumption has been that .physicallyCorrectLights can be removed within about a year of changing the default. By choosing .intensity, .decay, and .distance appropriately, users should be able to get their legacy lighting model (or very close to it) regardless of that removal. I'm not sure that we need a separate property to restore the legacy lighting model, in the long- or even medium-term.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Nov 17, 2022

From my discussions with @mrdoob, he wishes to ensure that books, courses, and user examples are not broken. Having a flag useLegacyLights will ensure that. WebGLRenderer will support both lighting models for the foreseeable future.

The new lighting model is the one that all other engines use. The transition is occurring NOW, with this PR.

The .physicallyCorrectLights flag is gone.

We can set renderer.useLegacyLights to false now, too, if you want, and the transition will be complete.

Or you can wait until the next release.

If you wait, then you are imposing a minor migration cost for the users who use PBR lights (and in our cases, 5 of 350 three.js examples).

@LeviPesin
Copy link
Contributor

This PR looks like the reverse of #24940. I think we should decide on one API over another - either useLegacyLights and legacyMode or enabled and physicallyCorrectLights...

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 17, 2022

From my discussions with @mrdoob, he wishes to ensure that books, courses, and user examples are not broken. Having a flag useLegacyLights will ensure that.

I was not aware that @mrdoob wants to keep a property to toggle different lighting. From #23897, I thought the respective flag will eventually be removed so there is only a single lighting mode.

Considering this, having a flag that enables the "legacy lighting" is fine with me.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 17, 2022

@WestLangley I'm really hoping for a few words explaining why ".useLegacyLights" is a preferable name to ".physicallyCorrectLights". The roadmap and next steps you describe are fine with me. But I'd like to be able to explain to users why the property has been renamed from A to B.

@WestLangley
Copy link
Collaborator Author

The user does not get physically-correct lighting simply by setting the physicallyCorrectLights flag to true.

Recall that we impose SI units for light intensities. So,

  1. The lighting will not be physically correct unless the scene is scaled so 1 distance unit = 1 meter.
  2. The lighting will not be physically correct unless decay is set to 2.

As you know, the renderer allows for any decay and any distance units.

For these reasons, I feel the term physicallyCorrectLights is misleading.

I prefer, instead, to refer to "PBR materials".

I also prefer not to require the user to set a flag at all -- unless the user wants to use the legacy light models.

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

@WestLangley I'm happy with that reasoning — thank you!

@WestLangley
Copy link
Collaborator Author

So, if this PR is merged, the next step is to decide if renderer.useLegacyLights should default to false now, or after the next release.

If it is set to false now, the transition is complete, and there is no churn for users currently using the physically-based lights.

How does everybody feel about that?

@donmccurdy
Copy link
Collaborator

I think r147 is too close to change the default. I'd be open to making both changes together in r148, if we can update more of the examples by then and write up a short migration guide. My concern is that if we change the default without having that advice ready, fewer users will opt to keep the new default.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 18, 2022

I suggest to defer this change even one more release. In r148 we want to remove example/js. This will be a major change for certain users and I'm afraid adding even more breaking changes are too much.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 18, 2022

I'm fine either way on that. In semver-land it's considered preferable to pack breaking changes together into larger (major) releases. But if it's too much for us to test and prepare for the same release on time, that's another matter.

@mrdoob mrdoob modified the milestones: r147, r148 Nov 30, 2022
@mrdoob mrdoob modified the milestones: r148, r149 Dec 22, 2022
@WestLangley
Copy link
Collaborator Author

/bump

@donmccurdy
Copy link
Collaborator

Weighing these two options:

  • (A) we rename without changing the default (default: .useLegacyLights = true)
  • (B) we rename, change the default, and write a new "Lighting" guide in the manual to help with the migration

I'm leaning toward (B), and would be happy to help write the guide if we are going that direction. I'm not as keen to write a guide focused on the legacy lighting behavior if not. 😇

@mrdoob mrdoob removed this from the r149 milestone Jan 26, 2023
@mrdoob mrdoob added this to the r150 milestone Jan 26, 2023
@mrdoob
Copy link
Owner

mrdoob commented Feb 10, 2023

Hmmm, I'll merge this for now and I'll do a new PR changing the default to see how many examples break...

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.

5 participants