-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add exposure to AFrame #5029
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
Add exposure to AFrame #5029
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ module.exports.System = registerSystem('renderer', { | |
| maxCanvasWidth: {default: 1920}, | ||
| maxCanvasHeight: {default: 1920}, | ||
| physicallyCorrectLights: {default: false}, | ||
| exposure: {default: 1, if: {type: ['ACESFilmic', 'Linear', 'Reinhard', 'Cineon']}}, | ||
| toneMapping: {default: 'No', oneOf: ['No', 'ACESFilmic', 'Linear', 'Reinhard', 'Cineon']}, | ||
|
||
| precision: {default: 'high', oneOf: ['high', 'medium', 'low']}, | ||
| sortObjects: {default: false}, | ||
| colorManagement: {default: false}, | ||
|
|
@@ -31,6 +33,7 @@ module.exports.System = registerSystem('renderer', { | |
| var renderer = sceneEl.renderer; | ||
| renderer.sortObjects = data.sortObjects; | ||
| renderer.physicallyCorrectLights = data.physicallyCorrectLights; | ||
| renderer.toneMapping = THREE[this.data.toneMapping + 'ToneMapping']; | ||
|
|
||
| if (data.colorManagement || data.gammaOutput) { | ||
| renderer.outputEncoding = THREE.sRGBEncoding; | ||
|
|
@@ -48,6 +51,13 @@ module.exports.System = registerSystem('renderer', { | |
| } | ||
| }, | ||
|
|
||
| update: function () { | ||
| var data = this.data; | ||
| var sceneEl = this.el; | ||
| var renderer = sceneEl.renderer; | ||
| renderer.toneMappingExposure = data.exposure; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no experience with exposure or tone mapping so this might be dumb. Are there use cases for different values of tone mapping and exposure? Should we always match them automatically?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tone toneMapping option is a list of functions that act on the colour of each pixel according to the exposure value specified. The different functions each have a slightly different aesthetic. You control the exposure for when the user moves into a bright environment you lower it to make the dark areas darker and make the outside less blinding bright. Usually it's just used to control the lighting on a global level to get the right feeling for the environment the developer is building. |
||
| }, | ||
|
|
||
| applyColorCorrection: function (colorOrTexture) { | ||
| if (!this.data.colorManagement || !colorOrTexture) { | ||
| return; | ||
|
|
||
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.
Can name values with lowercase if not an acronym for consistency:
linear,reinhard,cineonCan capitalize first letter below:
string.charAt(0).toUpperCase() + string.slice(1);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.
I think the default shouldn't be
1There 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.
"1" is the THREE.js default of no change.
https://threejs.org/docs/#api/en/renderers/WebGLRenderer.toneMappingExposure
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.
Probably can replace 1 with something more user friendly and translate internally.
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.
Maybe
noastoneMappingfor consistency?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.
Well it's a float it should be set to something like 0.5, or 1.1
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.
Like "intensity" on the light component
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.
Ah I see that the list is an
ifnot aoneOfMisread. SorryUh oh!
There was an error while loading. Please reload this page.
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 should be:
toneMappinginstead oftypeThere 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.
Thank you! Great spot!