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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmarcos
Copy link
Member

@dmarcos dmarcos commented Sep 17, 2024

Is this good? @vincentfretin @mrxz

@dmarcos
Copy link
Member Author

dmarcos commented Sep 18, 2024

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

@@ -20,7 +20,7 @@ module.exports.Component = registerComponent('light', {
color: {type: 'color', if: {type: ['ambient', 'directional', 'hemisphere', 'point', 'spot']}},
envMap: {default: '', if: {type: ['probe']}},
groundColor: {type: 'color', if: {type: ['hemisphere']}},
decay: {default: 1, if: {type: ['point', 'spot']}},
decay: {default: 2, if: {type: ['point', 'spot']}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change decay?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is @mrxz suggestion in #5546 (comment)

// Lights intensity had an implicit scaleFactor of PI now removed.
// It's reintroduced here since it's easier to think about integers vs multiples of PI.
var intensityScaleFactor = Math.PI;
var intensity = data.intensity * intensityScaleFactor;
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't change the intensity for scene that used renderer="physicallyCorrectLights: true" already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand the comment. Without the factor scene is dark. What should we do?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have aframe experiences with renderer="physicallyCorrectLights:true" and light intensity that was carefully set, those scenes are not dark, we don't want the intensity to change for those.

Copy link
Contributor

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

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

We could keep the physicallyCorrectLights property on renderer just to add a condition in the light component. But we should remove the lines

if (!data.physicallyCorrectLights) {
renderer.useLegacyLights = !data.physicallyCorrectLights;
}
and modify the tests.

#5546 for context.

@dmarcos
Copy link
Member Author

dmarcos commented Sep 18, 2024

We could keep the physicallyCorrectLights property on renderer just to add a condition in the light component. But we should remove the lines

if (!data.physicallyCorrectLights) {
renderer.useLegacyLights = !data.physicallyCorrectLights;
}

and modify the tests.
#5546 for context.

I think should be fine to remove the physicallyCorrectLights property in the renderer?

@mrxz
Copy link
Contributor

mrxz commented Oct 16, 2024

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.

@dmarcos
Copy link
Member Author

dmarcos commented Oct 21, 2024

Leaning towards deprecation

@dmarcos
Copy link
Member Author

dmarcos commented Oct 28, 2024

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.

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