-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
base: master
Are you sure you want to change the base?
Conversation
…WebGLRenderer.useLegacyLights deprecation (fix aframevr#5556, aframevr#5546)
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']}}, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
aframe/src/systems/renderer.js
Lines 38 to 40 in 2aca98c
if (!data.physicallyCorrectLights) { | |
renderer.useLegacyLights = !data.physicallyCorrectLights; | |
} |
#5546 for context.
I think should be fine to remove the |
As @vincentfretin pointed out, there are already scenes that have Besides that, applying the scaling only in the Instead of this PR, I think we should either:
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:
Not saying this change in THREE won't be worth long term but short tem dev experience suffers. |
Is this good? @vincentfretin @mrxz