-
-
Notifications
You must be signed in to change notification settings - Fork 35.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
Added SphericalHarmonicsLight. #13115
Added SphericalHarmonicsLight. #13115
Conversation
…order spherical harmonics.
src/renderers/WebGLRenderer.js
Outdated
@@ -1582,6 +1582,7 @@ function WebGLRenderer( parameters ) { | |||
// wire up the material to this renderer's lighting state | |||
|
|||
uniforms.ambientLightColor.value = lights.state.ambient; | |||
uniforms.sphericalHarmonicsValues.value = lights.state.sphericalHarmonicsValues; |
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 you can drop the Values
-suffix to keep the code consistent with the others.
lights.state.directionalValues
is called lights.state.directional
, same as the rest.
@bhouston Do you still feel that small cube maps are a better approach for irradiance than SH? Consequently, do you feel we should support @atteneder We currently use (see 2 below) the lowest level That being said...
|
Spherical harmonics are neat. I believe that for GI many localized spherical harmonics are used and then interpolated between as you move through the world. I favor spherical harmonics. I didn't didn't have time to complete my previous implementation here: #6405 Maybe we can merge this in with this new implementation. I think that SH is a decent addition to ThreeJS and will fine usage. |
Just to be more clear would it be possible @atteneder to merge in my previous SH implementation into yours? Maybe as an additional PR? This class in particular is incredibly useful for dealing with SHs: https://github.com/bhouston/three.js/blob/07ee8b1734c2babdc55fb96176f8cb10507b03ad/src/math/SphericalHarmonics3.js |
BTW if follow my implementation of SH here: https://github.com/bhouston/three.js/blob/07ee8b1734c2babdc55fb96176f8cb10507b03ad/src/renderers/shaders/ShaderChunk/spherical_harmonics_inc.glsl You'll notice that it is possible to get both irradiance as well as the direct luminance from the SH. Thus this could have a more complete implementation into the lighting step, rather than just irradiance. Also if you merge this into your PR, you will have a full spherical harmonics class for integration and modification of SHs: https://github.com/bhouston/three.js/blob/07ee8b1734c2babdc55fb96176f8cb10507b03ad/src/math/SphericalHarmonics3.js |
@bhouston I tried to merge your work into my branch, but I have a hard time doing so with github's tools. I can only compare your last commits ( this one bhouston@07ee8b1 ) and my branch, but cannot create a PR or export a diff. Probably because there's no branch referencing it anymore (was removed). |
…uston found here: bhouston@07ee8b1 Changes: Utilize SphericalHarmonics3 class to create a SphericalHarmonicsLight (instead of plain coefficient values). Using Ben's shader functions from spherical_harmonics_in.glsl and removed my previous implementation. Note: Moved demo values from original SphericalHarmonics3 class into example webgl_lights_spherical_harmonics.html. I thought they don't need to be in the build.
@bhouston nevermind, I manually merged your work. |
new THREE.Color(-0.1150112, -0.0936603, -0.0839287), | ||
new THREE.Color(-0.3742487, -0.2755962, -0.2875017), | ||
new THREE.Color(-0.1694954, -0.1343096, -0.1335315), | ||
new THREE.Color( 0.5515260, 0.4222179, 0.4162488) |
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 not the right design. These are coefficients, not colors. Colors can't be negative.
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, I guess you are right. I guess we should replace them with vectors.
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.
You have to admit that in the SphericalHarmonic3 class the Color class actually works quite well and is a convenient way to represent the coefficients, both for retrieval and accumulation: https://github.com/mrdoob/three.js/pull/13115/files#diff-b4c03c6f0f7c31a8942572bd76e5068d
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.
Thinking about it, I completely disagreed. They are coefficients but they are also colors. They are the colors of the various SH bulbs. Yes, sometimes they are negative but that is allowed when they are color coefficients.
Look on page 13 - basically the color coefficiences are the intensities of these individual bulbs added up -- in our case just the first 3 layers of 9 bulbs: http://silviojemma.com/public/papers/lighting/spherical-harmonic-lighting.pdf
So I think these should stay colors even though they are color efficients.
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.
They are coefficients but they are also colors.
@bhouston What color is ( - 1, 0, 0 )?
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 code is really nice too: https://github.com/mrdoob/three.js/pull/13115/files#diff-b4c03c6f0f7c31a8942572bd76e5068dR69
To convert it to a Vector3 seems like it just introduces more conversions and inconvenience. So yet, I am arguing from style back to this being a "color" but I like the way the code is. Also it sort of has to be a color because I never do anything but scale and sum them, there is really no true conversion taking place.
Also more proof that technically these are colors, band 0 of the standard SH is just a single color and it it just scaled by the coefficient 1. That seems like a color to me.
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.
According to wikipedia: https://en.wikipedia.org/wiki/Spherical_harmonics (see the main picture), the coefficiencies can be negative based on convention as they are defining the magnitude of periodic like functions. Just like the frequency amplitudes resulting from an Fourier transform of a real signal can be negative. It doesn't mean there are negative frequencies (again, you could ask, does negative frequencies exist) but it is related to the phase of the signal that allows it to be negative, because it is a periodic process.
Same thing.
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.
Sorry, I have hard time stomaching negative colors. We have the methodVector3.addScaledVector( v, s )
if you insist on that parameterization.
Spherical Harmonics are univariate, and unit-less. An SHLight
can be parameterized by 9 coefficients for each of 3 channels. Given the coefficients, the radiance and irradiance is then computed.
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.
"Spherical Harmonics are univariate, and unit-less."
They are not unit less, otherwise how would we produce a color as a result? Working backwards from the typed result, one of the two parts of the products must be unitless, either the variable coefficient or the fixed weights for the various harmonics. I am pretty sure the fixed weights are unitless, and because the result is a typed result (in our case color), then the coefficiencies must be typed.
This can seen because SH can represent many different types of qualities and in each case, the coefficiencies takes on the same properties of those types (intensity, color, etc.)
But it isn't important to me, so someone can swithc it to vector, it is just a type conversion.
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.
Color is unit-less. It is just a mask, and hence must be non-negative. It is the intensity parameter that has the units.
That is how I think about it anyway.
And, yes, the implementation can be changed later if we want.
There are problems here... By using On the left is the irradiance estimated from the highest mipmap; on the right, is the irradiance from your inputs. They should be similar. Also, |
Nice. :)
Maybe just leave the code in so we can use it in the future? I know there are uses but I do not have time today to look them up. |
Could be an intensity issue? Many of these environment maps do not have a calibrated intensity and neither does the SHes. They just seem to be random overall exposures. In my code I do show how one can convert a environment map to an SH approximation, it is the accumulator function here: https://github.com/mrdoob/three.js/pull/13115/files#diff-b4c03c6f0f7c31a8942572bd76e5068dR69 Basically one just has to repeatedly sample a cube map and accumulate these samples. It should convert to a proper SH representation once you normalize the results. |
* @author Andreas Atteneder | ||
*/ | ||
|
||
function SphericalHarmonicsLight( sphericalHarmonics3, intensity = 1.0 ) { |
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'd just use the term "sh" instead of "sphericalHarmonics3"
|
||
isSphericalHarmonicsLight: true, | ||
|
||
getCoefficients: function() { |
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'd promote this to SphericalHarmonics3 class as the function "toArray" or similar.
@@ -98,6 +98,11 @@ IncidentLight directLight; | |||
|
|||
vec3 irradiance = getAmbientLightIrradiance( ambientLightColor ); | |||
|
|||
#ifdef USE_SPHERICAL_HARMONICS | |||
vec3 shIrradiance = shGetIrradianceAt( sphericalHarmonics, worldNormal ); | |||
irradiance += LinearToGamma(vec4(shIrradiance,1), float(GAMMA_FACTOR) ).xyz; |
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.
Are you sure this is correct? Our renderer is in linear space.
How do you know the spherical harmonics coefficient are in linear space? I would think it is much more likely you have to go in the opposite direction: GammaToLinear. But it depends on how the coefficients were created.
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.
@WestLangley this could be part of the issue with intensities not aligning.
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.
There also appears to be a left handed vs right-handed coordinate system issue. The irradiance computed in the PR appears to be mirrored relative to the env map.
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.
If there is a different in handedness I would suggest strandardizing on unity's approach? I am not sure what is the dominant, canonical SH implementation, but we should try to identify that one and copy its standards rather than picking a random LH/RH for ours. Basically I am taking this to mean which coefficient corresponds to which direction.
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.
@bhouston There is no handedness problem with my implementation.
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'm not entirely sure wheter the coefficients I put in are gamma/linear, same about the handed-ness. I just gathered them from the Unity Editor and presumed (looking at Unity's shader + predefines). I'll look into it by creating a more meaningfull test cube as soon as I find time.
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.
Generally nice changes. Although I am sure that linearToGamma is incorrect. If the original source material was linear, then no conversion is needed, if the source material is gamma, you need GammerToLinear. Our renderer is linear in this part, thus you need to either keep it in linear or convert gamma to linear. There is no way that LinearToGamma is correct here.
@@ -19,6 +19,10 @@ varying vec3 vViewPosition; | |||
|
|||
#endif | |||
|
|||
#ifdef USE_SPHERICAL_HARMONICS | |||
varying vec3 worldNormal; |
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.
Unnecessary, look at how we are already query the hemiphere light and the environment map:
Notice it passes in geometry structure?
@@ -8,6 +8,10 @@ varying vec3 vViewPosition; | |||
|
|||
#endif | |||
|
|||
#ifdef USE_SPHERICAL_HARMONICS | |||
varying vec3 worldNormal; |
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 unnecesary. We already have this information.
@@ -52,4 +56,7 @@ void main() { | |||
#include <shadowmap_vertex> | |||
#include <fog_vertex> | |||
|
|||
#ifdef USE_SPHERICAL_HARMONICS | |||
worldNormal = normalize( mat3( modelMatrix[0].xyz, modelMatrix[1].xyz, modelMatrix[2].xyz ) * normal ); |
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 unnecesary. We already have this information.
Using some old code from three years ago, I get the following. The left sphere is the irradiance estimated from the highest mipmap, and the right sphere is the irradiance from the SH coefficients computed from the cube map. The computed irradiance appears to be a better representation than the mipmapped one. |
@bhouston I agree with your comments above that this PR is going to have to be refactored. @atteneder If you don't mind, it is easier for me to take your new contributions here and merge them with code @bhouston and I did three years ago, than it is for me to step you through all the changes necessary to fix this PR. Is that OK with you? |
@WestLangley the reason the SPH looks like it may be a better approximation may be that if you read from the very lowest resolution mipmap, it may be only a single pixel per cube size. It may be more accurate to read from the second lowest resolution mipmap or similar and it will have a resolution more similar to the SH. |
@bhouston We could compute the sh coefficients from the env map and use those for the irradiance component, rather than the low res mipmap, but it may not matter that much, visually. |
I would prefer to not do another step as we are already calculating the PMREM. The different isn't significant enough and really we could just read from the second to last mip map and I think it would be the same as the SH. I think SH is more useful when you calculate a lot of them as a GI solution and then interpolate between them as an object moves around an environment. SH in that context is very useful because it is very data efficient to store 100 of them on the GPU, where as storing 100 Cubemaps is just impossible. |
BTW my proposed use of SH for representing pre-computed global illumination via a network of light probes is not unique. It has been tried in Unity a long time ago: https://blogs.unity3d.com/2011/03/09/light-probes/ Alternatively we could implement PRT - it was sort of in fashion in the mid 2000s, but it seems to have gone away. http://www0.cs.ucl.ac.uk/staff/j.kautz/PRTCourse/ https://en.wikipedia.org/wiki/Precomputed_Radiance_Transfer |
This would be for non-PMREM use cases, though. I was also under the impression that PMREM was not going to be needed once LOD selection and mipmapping across seams is universally supported. Do you disagree? The current PMREM implementation also adds 100+ geometries to the scene. I never cared, because I did not view it as a permanent feature. |
The same statement applies to raw seamless CubeMaps. I would prefer no second step if it is unnecessary. IF you are concerned just use the second to last LOD and it should be similar to the SH result. Of course if you have a lot, like 100 light probes, the most efficient solution is SH, but you only get irradiance, then next most efficient is small CubeUV textures (like those produced by the current PMREM packer) and the last efficient is small CubeMaps. |
- Renamed variable - Implemented SphericalHarmonics3.toArray() mrdoob#13115
Regarding the double-counting: |
… should already be in linear space.
That is a great solution. |
…ronment map at mip map level 8 being added ). New behaviour: if a SphericalHarmonicsLight is present, it is used. Otherwise we fall back to then environment map.
…tion getSHLightProbeIndirectIrradiance. As suggested by bhouston mrdoob#13115 (comment)
I guess our color is a mask because we often limit it to 0 to 1 mostly for
UI and interchange purposes with traditional ldr representations. In view
of our truncated ldr color representation your argument makes sense.
hDR color in general is three floating point values of rgb. In this view I
feel my argument makes sense.
So because we do not have a hDR color class we can switch this to a vector3.
Best regards,
Ben Houston
http://Clara.io Online 3d modeling and rendering
…On Jan 29, 2018 6:12 PM, "WestLangley" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/webgl_lights_spherical_harmonics.html
<#13115 (comment)>:
> + new THREE.Color(-0.0837808, -0.0940454, -0.1027518),
+ new THREE.Color(-0.0319670, -0.0214051, -0.0147691),
+ new THREE.Color( 0.1641816, 0.1377558, 0.1010403),
+ new THREE.Color( 0.3697189, 0.3097930, 0.2029923)
+ );
+
+ THREE.SphericalHarmonics3.GalileoTomb = new THREE.SphericalHarmonics3().set(
+ new THREE.Color( 1.0351604, 0.7603549, 0.7074635),
+ new THREE.Color( 0.4442150, 0.3430402, 0.3403777),
+ new THREE.Color(-0.2247797, -0.1828517, -0.1705181),
+ new THREE.Color( 0.7110400, 0.5423169, 0.5587956),
+ new THREE.Color( 0.6430452, 0.4971454, 0.5156357),
+ new THREE.Color(-0.1150112, -0.0936603, -0.0839287),
+ new THREE.Color(-0.3742487, -0.2755962, -0.2875017),
+ new THREE.Color(-0.1694954, -0.1343096, -0.1335315),
+ new THREE.Color( 0.5515260, 0.4222179, 0.4162488)
Color is unit-less. It is just a mask, and hence must be non-negative. It
is the intensity parameter that has the units.
That is how I think about it anyway.
And, yes, the implementation can be changed later if we want.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13115 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAj6_VR5SaIbosXMI809A1E6lb5QXROVks5tPlBngaJpZM4ReiDb>
.
|
…lors), following this discussion: mrdoob#13115 (comment)
# Conflicts: # src/renderers/shaders/ShaderChunk/lights_fragment_begin.glsl # src/renderers/shaders/ShaderChunk/lights_pars_maps.glsl
SphericalHarmonicsLight is an ambient lighting term based on 3rd order spherical harmonics.
See the example I included to see its effect.
Motivation:
At the moment you can only add ambient lighting by one flat color value (THREE.AmbientLight). I was investigating how to get a more sophisticated visual result. Spherical Harmonics seem to be state of the art in other engines, so I tried to introduce it to three.js
Thoughts:
Only tested with one spherical harmonics light per scene. Having more than one doesn't really make sense I guess.
A follow up feature could be to retrieve/calculate the SH values from a cubemap directly. For now this can be done by tools like Lys ( https://www.knaldtech.com/lys/ ) or using this little Unity3d helper script i created:
https://gist.github.com/atteneder/d4fe33368afaf176413ec09d14ef41db