-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Remove .shadowDarkness? #7321
Comments
I'm not sure i agree. I use fairly light shadows in my work (0.6 or less) because they are pretty wrong to begin with and when you set darkness to high it stands out too much. |
What @Usnul said. At least until the time at which the shadowing methodology is replaced. For now, I prefer this pattern:
|
How about we move... #ifdef METAL
outgoingLight += diffuseColor.rgb * ( totalDiffuseLight + totalAmbientLight ) * specular + totalSpecularLight + totalEmissiveLight;
#else
outgoingLight += diffuseColor.rgb * ( totalDiffuseLight + totalAmbientLight ) + totalSpecularLight + totalEmissiveLight;
#endif out of Then, we make And we create a new #ifdef METAL
outgoingLight += diffuseColor.rgb * ( totalDiffuseLight * shadowMask + totalAmbientLight ) * specular + totalSpecularLight + totalEmissiveLight;
#else
outgoingLight += diffuseColor.rgb * ( totalDiffuseLight * shadowMask + totalAmbientLight ) + totalSpecularLight + totalEmissiveLight;
#endif And we put this chunk after I know this isn't the final solution but I think it could be a step in the right direction? At least we solve ambient light and emissive... I think specular wouldn't be correct though, that formula doesn't make sense... Maybe it should be... #ifdef METAL
outgoingLight += ( diffuseColor.rgb * ( totalDiffuseLight + totalAmbientLight ) * specular + totalSpecularLight ) * shadowMask + totalEmissiveLight;
#else
outgoingLight += ( diffuseColor.rgb * ( totalDiffuseLight + totalAmbientLight ) + totalSpecularLight ) * shadowMask + totalEmissiveLight;
#endif But then we lose ambient light again... |
Also... maybe we should remove shadowmap out of |
The idea is to replace |
do you think it will work with this example? other than that, i think sometimes you may want to have stark contrast in shading of a material because due to direct light but still want to tone down the shadows, in which case ambient light will be pretty low while desiring shadowDarkness to be low also. |
But this is hacky and not like light works... |
Ok. So, as far as I understand the proper solution would be to merge shadowmap_fragment into lights_phong_fragment. The reason /cc @tschw @mkkellogg |
it is hacky and not like the light works, but it is a good way to cover up hacks like shadow maps :D |
Hmm... If we want to have nice shadows, I think we may need to remove shadow support in |
In the real world the shadows have nothing to do with materials. |
@RemusMar |
Don't mix them up. |
@RemusMar light warps around the edges of geometry in real life depending on material. Beyond this, both umbra and penumbra can have different brightness based on glossiness of back surface. |
Come on ... On the top of that: we don't need a THREE.js bloated with all kind of worthless features. |
|
In fact I was trying to help you in the above posts.
Yes. |
Fixed! Sorry guys, but It's still not as good as #6420 but should be a step in the right direction (and it works in all materials). |
How about showing the three.js revision on the Editor GUI? |
Good idea! |
Before... After... Adding another light source still shows the backface issue though. @WestLangley I wonder if instead of |
I also wonder if something like this would be beneficial?
|
OT: Wow! Editor with shadows 👍 |
Hmm... I guess we can still keep shadowDarkness until the final solution. |
😊 |
|
Hi! sorry for waking up the death. I just wanted to suggest that many lights emiting shadows will render unrealistic results in a scene that has an ambient light: |
@autotel probably better to do a render with Blender or something :) |
Yup. I understand. Is like after that we will start thinking about caustic effects and stuff; a bit out of browser's scope. Often this place is used for feature requests, but in this case I just wanted to point out that possibility. |
@MasterJames points out this issue in #7319:
http://jsfiddle.net/xsgebh0b/5/
Of course, this is a matter of setting
shadowDarkness
to1
(instead of0.5
).Which makes me think that we probably should remove
shadowDarkness
and simplify things. However, I notice that the shadow is 100% black even if the scene has anAmbientLight
of0x404040
and makes me think that the shadow step is in the wrong step.The text was updated successfully, but these errors were encountered: