Reintroduce light intensity scale factor removed in THREE r165 after WebGLRenderer.useLegacyLights deprecation (fix #5556, #5546)
Is this good? @vincentfretin @mrxz
Colors of the hello world look same as before after applying the intensity scale factor
We could keep the physicallyCorrectLights property on renderer just to add a condition in the light component. But we should remove the lines
https://github.com/aframevr/aframe/blob/2aca98cbbc5f1f03431ce07f107709baa1718df8/src/systems/renderer.js#L38-L40
and modify the tests. #5546 for context.
I think should be fine to remove the physicallyCorrectLights property in the renderer?
As @vincentfretin pointed out, there are already scenes that have physicallyCorrectLights enabled and should not have their intensities scaled. If we get rid of the property on renderer, then there won't be a way to distinguish the two.
Besides that, applying the scaling only in the light component will miss lights created in other ways. Components might directly create lights or a glTF file might include them. Even the reflection component manually sets light intensities, which would need to be updated as well (reflection.js#L20).
Instead of this PR, I think we should either:
- Follow Three.js and deprecate the "legacy lights" entirely. The default light setup and things like the environment-component can be updated. Document what users need to do to upgrade their scenes.
- Retain the old behaviour by re-introducing
useLegacyLightsinsuper-three. This ensures that everything behaves like before, instead of only thelightcomponent.
Personally not a fan of diverging from Three.js, but forcing upgrading users to make changes with no way to opt-out is also far from ideal.
Leaning towards deprecation
Going back and forth with this:
- Keeping old behavior is a big depart from mainstream THREE and code is complex to maintain forward.
- Adapting to new model requires modifying examples and 3rd party components and corresponding examples to recreate same look. And code ends up with random ugly numbers that are hard to understand. Dev experience suffers and unclear how the change benefits most of A-Frame users.
Not saying this change in THREE won't be worth long term but short tem dev experience suffers.
I multiplied the light defaults and values in examples by 3.14
https://github.com/aframevr/aframe/commit/8813d3d2818634ed4900409d99544b60587aa300
Also updated the environment component defaults and published a new version: 1.4.0
Any other outstanding 3rd party components that we should adjust?
@dmarcos can you please also remove the physicallyCorrectLights option from the renderer system?
@vincentfretin done. thanks 🫡