mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.6k stars 35.29k forks source link

Proposed HemisphereLight Modeling Changes #7154

Closed WestLangley closed 8 years ago

WestLangley commented 8 years ago

Hemisphere lights model indirect, ambient, diffuse light.

However, the THREE.HemisphereLight has a specular component, too. This looks nice, but it is not correct. There should not be a specular component. The specular component can be modeled by an environment map or an image-based light, if desired.

In addition, since hemisphere lights model ambient light, lighting from a hemisphere light should always be attenuated by the ambient occlusion map, if it is present. It is not implemented that way in three.js.

I propose that the THREE.HemisphereLight be modified as follows:

  1. Remove the specular term in the hemisphere light model.
  2. Make hemisphere lighting additive to total ambient light (not to total direct light, as it is now), so it will be properly attenuated by an AO map if the map is present.
  3. Optionally, modify the code to support just a single hemisphere light, as we do with AmbientLight. There is no reason for more than one HemisphereLight. (We don't have to do this; it is just an option.)

Also, in the examples, hemisphere lights will have to be either removed and replaced, or supplemented with other, direct, light sources.

Some users have used the THREE.HemisphereLight as a convenient all-in-one light source, and will not like this change. But it is the correct thing to do from a modeling standpoint.

/ping @bhouston

bhouston commented 8 years ago

I of course agree for removing specular from the hemispheric light as I brought that up here: https://github.com/mrdoob/three.js/issues/6271 :)

I support that wholeheartedly.

I am not about continuing to have the idea of ambient light, rather we should probably talk about indirect diffuse light, because physically that what it is -- we both read that term from that amazing Frostbite Engine PBR paper I believe

Often tools allow for more than one hemispheric light, and then restrict them by distances or you can only use the nearest or they blend together like light probes. In a way, hemispheric lights are simplified light probes and likely can be modeled as such. I think trying to restrict it to one hemispheric light isn't necessary -- it can be a user thing.

mrdoob commented 8 years ago

I support this too 👍

WestLangley commented 8 years ago

we should probably talk about indirect diffuse light

Sure. We can use the terms in this post: direct-diffuse, indirect-diffuse, direct-specular, and indirect-specular.

Often tools allow for more than one hemispheric light

OK. I will continue to allow for multiple hemisphere lights.

and then restrict them by distances or you can only use the nearest or they blend together like light probes.

One problem we have is that for DirectionalLight and HemisphereLight, the position is used for direction. I always thought that was odd. We could fix it and have a position and direction for both -- but that would be a separate PR. /ping @mrdoob

bhouston commented 8 years ago

We actually have a work around in Clara to allow you to position hemispheres and directional lights properly while using orientation for direction. So I forgot this was still like this but yeah it is weird and needs to be fixed.

mrdoob commented 8 years ago

Well, the position matters when you are casting a shadow. It can't be normalised in that case.

WestLangley commented 8 years ago

Well, the position matters when you are casting a shadow. It can't be normalised in that case.

Hmmm....

For directional lights, the position is used for the shadows, but a normalized position (in camera space) is passed to the shader, and is used for direction.

For hemisphere lights, shadows are not supported (fortunately), and the position is also normalized when passed to the shader, and is used for direction.

So one proposal would be to add direction as a property to these light types to keep it separate from position.

That would mean for hemisphere lights, position would not be used, since there are no shadows, and direction would be used for direction.

And for directional lights, position would be used for shadows, and direction would be used for direction --- and that may be just a bit too confusing...

bhouston commented 8 years ago

I think for the lighting calculation one only needs direction in the glsl for directional lights but you need position for directional lights for placing the directional light shadow frustums in the scene, but that position isn't used in the lighting calculation.

So we can maybe just rename the position to direction in the shaders for hemisphere and directional and call it done for these. No need to pass in position for either until we need it. Sorry for the distractions.

The thing we actually were working around in Clara.io a long time ago was the need to position sub objects of lights to set the direction of lights (basically always setting a look at target), rather than say using a specific axis of the light as the light direction -- really you want to have both abilities, and the first (look at target) can be implemented in a more general fashion that would set the orientation of a node based on a general look at linage to any other node in the scene.

WestLangley commented 8 years ago

Well, as it stands now, hemisphereLight.postiion.normalize() is used for direction, and hemisphereLight.position has no meaning by itself.

directionalLight.position.normalize() is used for direction, and directionalLight.position is used for shadow mapping only.

Because directional light position and direction are coupled, you can't have a shadow-casting directional light positioned at ( 0, 100, 100 ) that has direction ( 1, 1, 0 ).

The fix is to add directionalLight.direction or directionalLight.target.

In which case, we might as well add hemisphereLight.direction, too.

GGAlanSmithee commented 8 years ago

I am very bad at shading, so excuse me if I'm wrong, but I thought directional light is light that seems to come from so far away (the sun) that, in practice, there is no position to account for and only the direction mathers?

When I started to use three.js I was very suprised that DirectionalLight had a position but no direction, and that shadow casting depended on the position of the light. I worked around this by moving the directional light (sun) with the camera, but is this usually how directional light is implemented? Cannot recall this behaviour in other game engines I've used.

WestLangley commented 8 years ago

I thought directional light is light that seems to come from so far away (the sun) that, in practice, there is no position to account for and only the direction matters?

If you are casting shadows, a position is required to define the shadow camera frustum.

mrdoob commented 8 years ago

I thought directional light is light that seems to come from so far away (the sun) that, in practice, there is no position to account for and only the direction matters?

If you are casting shadows, a position is required to define the shadow camera frustum.

For which, I think we could try replacing with a new distance property.

GGAlanSmithee commented 8 years ago

I see, thanks for the explanation!

bhouston commented 8 years ago

My only request for directional lights is that one can place the shadow volume arbitrarily in space -- thus its center, width, height, start and end can all be set as they are now. The reason is for best results you want the shadow volume to be as small as possible, and positioned just around your objects of interest -- start just before your objects and end just after them, and be centered on them, so you can use all of its precision with the smallest width/height of the shadow map as possible.

Thus the light usually would use position and direction -- it is great to position lights in your scene, sucks when they are all overlapping at the origin. And then specifically for the shadow volume: nearDistance, farDistance, nearWidth, nearHeight or anything equivalent to that is great.

WestLangley commented 8 years ago

@bhouston has reiterated what I said above, and I agree.

Because directional light position and direction are coupled, you can't have a shadow-casting directional light positioned at ( 0, 100, 100 ) that has direction ( 1, 1, 0 ).

The fix is to add directionalLight.direction or directionalLight.target.

In which case, we might as well add hemisphereLight.direction, too.

I think the only change we need is to add directionalLight.direction and hemisphereLight.direction. @bhouston do you agree?

WestLangley commented 8 years ago

@mrdoob wrote

For which, I think we could try replacing with a new distance property.

In light of the previous post, what do you think is best?

mrdoob commented 8 years ago

Hmmm... I don't know... I'm even starting to consider that we may want to decouple the shadow from the light... new THREE.ShadowCaster( light )?

bhouston commented 8 years ago

Unity3D has shadows integrated into their lights, although that doesn't mean we couldn't use ShadowCasters maintained behind the scenes for each light that has shadows enabled?

For directional lights, Unity3D uses light position combined with a shadow distance -- they do not have a start distance for their directional shadows. It appears that the light frustum starts at the light origin/position and goes until shadow distance is reached -- if this was what you were suggesting, it appears that you were suggesting what is "best practice" according to Unity3D:

http://docs.unity3d.com/Manual/DirLightShadows.html (see Shadow Distance section.)

Thus I am okay getting rid of a start distance for the frustum and instead using just a single distance for shadows. We could do this for all shadow volumes if it isn't that way already.

bhouston commented 8 years ago

Unity3D fades out the shadow as you reach the limit of the shadow distance -- this would be nice because the hard cutoffs I've seen in ThreeJS shadows once you reach the far distance cutoff is very noticeable in a lot of cases and looks horrible.

bhouston commented 8 years ago

We could just use the existing distance field on the spot and point lights to control the distance of the shadow map frustum. We can add a distance to the directional light to also control the distance cutoff of its shadow map frustum. And if each has a position, then we are all set, we can simplify the shadow frustum specification significantly and make it less error prone -- as it is complex for users to have to setup the shadowCameraNear/shadowCameraFar variables correctly.

WestLangley commented 8 years ago

I would expect there would be matrix stability or precision issues if shadowCameraNear is zero -- not for a directional light since the shadow camera is orthographic, but likely for spot light, which has a perspective shadow camera.

bhouston commented 8 years ago

We could improve the spot light specification as well to remove the need for the shadowCameraFov because right now we have the cutoff angle represented by angle.

@WestLangley

I would expect there would be matrix stability or precision issues if shadowCameraNear is zero

If that is the case, we could just fix shadowCameraNear to be an epsilon or even 1% or 5% of the maximum range distance value -- that will avoid the precision issues but essentially be the same.

WestLangley commented 8 years ago

@bhouston I think you will get depth precision problems if shadowCameraNear is epsilon.

I have no objection to your idea for simplification, I am just noting the issues we will have to deal with when implementing it.

makc commented 8 years ago

casting @Fabrice3D to maybe comment on lights and shadows ;)

mrdoob commented 8 years ago

For directional lights, Unity3D uses light position combined with a shadow distance -- they do not have a start distance for their directional shadows. It appears that the light frustum starts at the light origin/position and goes until shadow distance is reached -- if this was what you were suggesting, it appears that you were suggesting what is "best practice" according to Unity3D

Yay! Yes, that's exactly what I'm suggesting. I've suggested that for a long time actually, initially for removing the target dependency. Now that I think more about it, I think this is still doesn't replace target...

I do think that we need to create a class for the shadow stuff though... say THREE.Shadow instead of having all these shadow parameters inside lights. Maybe we can add a position to that object that the user could modify... light.shadow.position.set( 100, 0, 100 ). We need to make sure we update those when updating the scenegraph in WebGLShadowMap.

The nice thing about having position and distance in DirectionalLight is that we can create ranges of light. At the moment everything gets lit with a DirectionalLight. But it could be great if whatever was behind the position didn't, and same with whatever is after distance.

Unity3D fades out the shadow as you reach the limit of the shadow distance -- this would be nice because the hard cutoffs I've seen in ThreeJS shadows once you reach the far distance cutoff is very noticeable in a lot of cases and looks horrible.

Agreed! Sounds good to me.

We could just use the existing distance field on the spot and point lights to control the distance of the shadow map frustum. We can add a distance to the directional light to also control the distance cutoff of its shadow map frustum. And if each has a position, then we are all set, we can simplify the shadow frustum specification significantly and make it less error prone -- as it is complex for users to have to setup the shadowCameraNear/shadowCameraFar variables correctly.

Sounds great!

GGAlanSmithee commented 8 years ago

The nice thing about having position and distance in DirectionalLight is that we can create ranges of light. At the moment everything gets lit with a DirectionalLight. But it could be great if whatever was behind the position didn't, and same with whatever is after distance.

I am sure that it is implementation specific, and I understand the argument about having a frustum for shadow casting, but is this really how directional lighting - in general - works? I cannot remember that the position of a directional light has ever made any difference in what gets lit or not in other applications I´ve used. For other kinds of lights, sure, but not directional lights.

There must be some value in staying consistent with how lighting works in other big rendering applications (UE, Unity3D etc). Maybe there is another kind of light that works like a directional light, but where position does make a difference? If so, it could be smart to introduce such a light with that functionality instead of adding it to the DirectionalLight?

mrdoob commented 8 years ago

Yeah, ok... Maybe I got a little bit creative there 😇 Maybe SpotLight in orthographic mode would make more sense.

bhouston commented 8 years ago

@GGAlanSmithee Most other tools have positions for their directional lights. Directional lights in UE4 and Unity3D and 3DS Max and Maya and Softimage all have positions. Directional lights in Arnold and V-Ray all have positions as well. If we were to stay with how it is usually done, our Directional lights in ThreeJS will have positions as well.

Directional Lights in Unity have a range/distance as well as a radius, both of these values are dependent upon the position value. UE4's shadow system uses the position of the directional lights as well quite a bit.

I foresee a bunch of inconvenience and awkwardness if we try to make our Directional lights NOT have a position and I do not really see any real benefit.

GGAlanSmithee commented 8 years ago

@bhouston I know there is a position for directional lights, and as @WestLangley explained to me, there is a good reason for this atleast for shadow casting if nothing else. What I was reffering to was that the position doesn't affect what is lit and not. For example if an object is placed behind a directional light, based on the lights position and direction, that object is still lit.

This is what I observed using other graphical applications, and also what most sources agrees on - a directional light only needs a direction - no position (for lighting, not taking shadows into account). I was just suggesting that changing that behaviour in three.js might be confusing (i.e having a DirectionalLight only lit objects that are in front of it, based on position and direction)

It might very well be the case that I am wrong about this, if so sorry! I should probably remove myself from this conversation since I don't really know what I'm talking about here :)

bhouston commented 8 years ago

I did a bit more research into Unity3D and they do not have a range/distance on their directional lights any more -- I guess the docs are out of date. They now have some smart automatic shadow frustums that auto-change shadow map resolution based on the size on screen of the shadows and if you go far enough away of a shadow it fades out, but the shadows from the same light that are closer do not fade out -- again all automatically. That is something to strive for, but at this moment I do not know how to achieve this effect, but it is very user friendly.

Basically Unity3D has perfect Directional Light shadows everywhere (in front and behind the Directional Light position) and they are fully automatic in terms of their setup.

bhouston commented 8 years ago

@GGAlanSmithee I think you are right though based on my Unity 3D research that DirectionalLights can light behind their point of origin. But for our simple shadowing method, we might as well start the shadows at or near their point of origin because we do not have the Unity3D super shadow frustum method yet.

WestLangley commented 8 years ago

Closing. Original suggestion was implemented in #7170