-
Notifications
You must be signed in to change notification settings - Fork 819
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 configurable shadow intensity, change default #388
Conversation
aed5e01
to
51e90ac
Compare
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.
Some nitpicking thoughts, I don't feel strongly one way or another -- LGTM!
src/features/environment.js
Outdated
@@ -167,6 +176,16 @@ export const EnvironmentMixin = (ModelViewerElement) => { | |||
this[$needsRender](); | |||
} | |||
|
|||
[$updateShadow]() { | |||
const {shadowStrength} = this; | |||
const shadowStrengthIsNumber = |
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.
Doesn't lit-element handle some of this?
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.
Lit won't stop the value from being NaN
when a non-number value is set in the attribute. And, any value may be assigned to the property, Lit does not do any filtering or validation there.
src/features/environment.js
Outdated
@@ -38,10 +41,16 @@ export const EnvironmentMixin = (ModelViewerElement) => { | |||
...super.properties, | |||
backgroundImage: {type: String, attribute: 'background-image'}, | |||
backgroundColor: {type: String, attribute: 'background-color'}, | |||
experimentalPmrem: {type: Boolean, attribute: 'experimental-pmrem'} | |||
experimentalPmrem: {type: Boolean, attribute: 'experimental-pmrem'}, | |||
shadowStrength: {type: Number, attribute: 'shadow-strength'} |
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 shadow-intensity
may be more in line with how shadow "strength" is normally defined
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 am planning to add some "intensity" configurations that control lighting. Since shadow is the absence of light, intensity seemed like a weird word to use.
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.
Changed to intensity, as @johnpallett points out we may want a complimentary value in the future called "softness" which seems weird to pair with "strength." Then again, must one always be either soft or strong, or is this just a bias instilled in me by social prejudice?
src/features/environment.js
Outdated
@@ -16,8 +16,10 @@ | |||
import {BackSide, BoxBufferGeometry, Color, Mesh, ShaderLib, ShaderMaterial, UniformsUtils} from 'three'; | |||
|
|||
import {$needsRender, $onModelLoad, $renderer, $scene, $tick} from '../model-viewer-base.js'; | |||
import {BASE_SHADOW_OPACITY} from '../three-components/StaticShadow.js' |
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 may be biased due to my history with features/environment
, but could the shadow feature live in a different feature? This is the first attribute that's outside of the skybox -- while it handles IBL, I wonder if a lights
feature would be appropriate for the shadow setting, as well as other future attributes? Just spitballing (more than OK for now)
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.
Yeah, will move per suggestion below
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.
Also re: lights, the thought occurred to me as well. The main point of uncertainty for me is that technically the environment is also lighting (in graphics contexts, at least).
src/features/environment.js
Outdated
typeof shadowStrength === 'number' && !Number.isNaN(shadowStrength); | ||
|
||
this[$scene].shadow.material.opacity = BASE_SHADOW_OPACITY * | ||
(shadowStrengthIsNumber ? shadowStrength : DEFAULT_SHADOW_STRENGTH); |
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 I prefer moving some of the internal logic into the component, e.g. this[$scene].shadow.setIntensity(...)
and let the component handle the base shadow opacity, similar to model.applyEnvironmentMap(...)
in this module, and leave the feature focused on plumbing the different configurations/components, and let the components handle the details. This idea sort of fell a part a bit with the removal of the Skysphere, since scene.background
is being manipulated directly here, and we don't have an abstraction/three-component for it otherwise.
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 don't think it'll matter much, but wonder if there's anything we need to do with the default case (e.g. rendering an opacity=0 mesh) -- I think it'll be skipped in the renderer somewhere, and the only potential overhead is the initial creation of the shadow. Even if we're rendering an opacity=0 shadow, it's a fake shadow anyway, so a negligible cost (outside of possibly the LUT on mobile??)
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.
Will fix
15d201c
to
2b3acd6
Compare
This change proposes the addition of configurable shadow intensity, with a new default of
0.0
, effectively causing shadows to be hidden by default.The image above demonstrates the shadow being changed on a timer between values
0.0
and2.0
. The previous default intensity can be achieved by setting the value to1.0
.Fixes #206