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

Consider defaulting to renderer.physicallyCorrectLights = true #2127

Closed
donmccurdy opened this issue Mar 16, 2022 · 13 comments
Closed

Consider defaulting to renderer.physicallyCorrectLights = true #2127

donmccurdy opened this issue Mar 16, 2022 · 13 comments
Labels
documentation to do with docs enhancement New feature or request

Comments

@donmccurdy
Copy link
Member

donmccurdy commented Mar 16, 2022

The current R3F defaults include...

  • renderer.outputEncoding = sRGBEncoding
  • renderer.toneMapping = ACESFilmicToneMapping

... both of which are generally good defaults for a physically-based rendering workflow (even if tonemapping is pretty subjective). For the same reason I think it'd be a good idea to consider defaulting to renderer.physicallyBasedLights = true. This is the only mode in which lights imported with a GLB model will look as intended. Eventually I'd like to see that become the default in three.js as well — see mrdoob/three.js#23614.

@bhouston
Copy link
Contributor

I agree. Everyone wants physicallyCorrectLights, and it was only added in this old fashion because we needed to not break backwards compatibility with people using incorrect lgihts.

@drcmda
Copy link
Member

drcmda commented Mar 17, 2022

sounds like a good idea. just as someone that has never used physical lights before, is it harder to control? even without knowing the math, these are just higher numbers, correct?

@gsimone
Copy link
Member

gsimone commented Mar 17, 2022

Since we are just going with a major soon, it would also be good timing to make the change. Would it need significant changes in existing demos/examples?

@donmccurdy
Copy link
Member Author

donmccurdy commented Mar 17, 2022

If I remember correctly, the main change is that you'd want to increase intensity of the lights by a factor of PI when making the change. The behavior of decay and distance also changes under physically-correct mode, less dramatically, as described here:

https://threejs.org/docs/index.html?q=light#api/en/lights/PointLight

Once the initial change is made, control should be easier and more natural. In particular I think the .distance field behaves much more usefully. Lights do not have physical units when physicallyCorrectLights=false, so trying to match lighting from other software or the real world will be a guessing game. With physicallyCorrectLights=true you can make conversions from other software or match reference values for real-world lights, e.g. with @bhouston's great spreadsheet here:

https://docs.google.com/spreadsheets/d/1Ce9XCC2Ub9eVjAQdYbYdLvlkDBJ_0GtAyRbSF0KJWzE/edit#gid=0

@CodyJasonBennett
Copy link
Member

I implemented this in #2148 but ended up reverting since the migration for point lights was rather non-trivial.

Are we able to give this the same treatment as color management? Maybe continue the discussion in three.

@donmccurdy
Copy link
Member Author

donmccurdy commented Mar 30, 2022

Correcting my last comment, the changes required to migrate a scene to physicallyCorrectLights = true "automatically" would typically be:

  1. increase light intensity by factor of Math.PI
  2. reduce light decay to or near 0 (or increase intensity further to compensate for decay)

Now obviously (2) is not a "physically correct" choice either (correct decay is 2, also non-default...), but at least you now have physical units for your lights, and any artistic tweaks are just in the light settings rather than the shading model. Lowering decay on lights seems OK to me.


Are we able to give this the same treatment as color management? Maybe continue the discussion in three.

three.js does expose this as a renderer setting but currently R3F does not, maybe the place to start would be to add that setting in R3F and document it with "recommended for new projects" or something like that?

Totally understand it might be too painful a migration at this point, I suspect we'll see the same objections on the three.js repo. Maybe a more practical path would be to ensure this property is on by default (or omitted entirely) on WebGPURenderer. At the moment I don't see the option there, not sure how it's coded though.

@donmccurdy
Copy link
Member Author

Summary of the practical differences here:

mrdoob/three.js#23897 (comment)

The main thing is that point and spotlights do not attenuate under default settings in the legacy mode, but they do attenuate by default in physical mode. Unfortunately that's a pretty hard breaking change.

@krispya krispya mentioned this issue Jun 24, 2022
18 tasks
@CodyJasonBennett CodyJasonBennett added enhancement New feature or request documentation to do with docs labels Sep 3, 2022
@donmccurdy
Copy link
Member Author

I don't think we can change defaults for lighting OOTB, problems are detailed in the linked issue. That might be a consideration for #2299 but the issue of decay not respecting physicallyCorrectLights remains. I think this is something to be considered in threejs itself.

Agreed.

@krispya @CodyJasonBennett continuing the thread from #2338 (comment), could I ask what you mean by "the issue of decay not respecting physicallyCorrectLights", and what change you are hoping to see in three.js upstream? Does this refer to changing the default of light.decay from 1 to 2?

Unfortunately, there is no acceptable "non-breaking" way to enable .physicallyCorrectLights by default. The scene lighting will change, unless .physicallyCorrectLights was already enabled by the user. I'm not sure what we can do upstream to ease that transition, if anything. 😕

@CodyJasonBennett
Copy link
Member

I would be referring to the default decay since we'd otherwise not touch decay in demos. That would be my main reservation against enabling this by default, but users can opt-in via <Canvas gl={{ physicallyCorrectLights: true }} />. I don't have a proposal for three, but I think it would be inappropriate for R3F to modify three's behavior here to hide the complexity.

@donmccurdy
Copy link
Member Author

I see, thanks – The change to the default decay is already proposed ...

... so I will bump that thread again, feel free to leave feedback there as well!

@krispya
Copy link
Member

krispya commented Sep 26, 2022

Yeah I feel the same way as Cody. I want to see physicallyCorrectLights become default (along with decay of 2) and will support that change but I feel like it should happen at three first so R3F doesn't start to diverge significantly based on our opinions.

@donmccurdy
Copy link
Member Author

I see. These defaults in three.js are what they are only for backward-compatibility, and to limit difficult migrations for users. physicallyCorrectLights=true would be a better default than false; that has been true ever since the option was added back in 2016. The challenge (with or without semver) is the same — deciding which "better" behaviors should be imposed on users by default, and when. Project maturity and growth tend not to make this any easier.

So I think my advice would be to think of this in terms of what behavior and timing for changes are best for R3F users, rather than in terms of divergence from three.js defaults. But I certainly understand whichever decision you go with.

@krispya
Copy link
Member

krispya commented Oct 2, 2022

The same is true for R3F users too. All our demos and tutorials are made with physicallyCorrectLights=false and making the change would mean at least updating all of those and educating users why their projects suddenly look wrong and why the same lighting tutorials and examples now look different.

Right now it isn't something that comes up in the Discord form our users very often even though it is a GLTF workflow issue. Maybe something will change that, like more asset tooling, and we can reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation to do with docs enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants