aframe icon indicating copy to clipboard operation
aframe copied to clipboard

Reintroduce light intensity scale factor removed in THREE r165 after WebGLRenderer.useLegacyLights deprecation (fix #5556, #5546)

Open dmarcos opened this issue 1 year ago • 3 comments

Is this good? @vincentfretin @mrxz

dmarcos avatar Sep 17 '24 23:09 dmarcos

Colors of the hello world look same as before after applying the intensity scale factor

dmarcos avatar Sep 18 '24 01:09 dmarcos

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?

dmarcos avatar Sep 18 '24 16:09 dmarcos

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:

  1. 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.
  2. Retain the old behaviour by re-introducing useLegacyLights in super-three. This ensures that everything behaves like before, instead of only the light component.

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.

mrxz avatar Oct 16 '24 11:10 mrxz

Leaning towards deprecation

dmarcos avatar Oct 21 '24 03:10 dmarcos

Going back and forth with this:

  1. Keeping old behavior is a big depart from mainstream THREE and code is complex to maintain forward.
  2. 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.

dmarcos avatar Oct 28 '24 01:10 dmarcos

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 avatar Nov 12 '24 04:11 dmarcos

@dmarcos can you please also remove the physicallyCorrectLights option from the renderer system?

vincentfretin avatar Nov 12 '24 18:11 vincentfretin

@vincentfretin done. thanks 🫡

dmarcos avatar Nov 12 '24 23:11 dmarcos