Renderer ignores more than >2nd band of spherical harmonics illumination

[BernhardEgger]

Our renderer ignores everything beyond the second band of spherical harmonics illumination.
Other parts of the framework support more bands.
Adding more bands to the renderer would need some new code.
This line of code however let's the user in the dark about the parameters being cut and ignored...
Wasted quite a bit of time on this and would honestly prefer if the code would throw an error if the renderer is called with more parameters than supported.

scalismo-faces/src/main/scala/scalismo/faces/render/PixelShaders.scala

Line 154 in 25ae368

val numCoeffs = math.min(environmentMap.size, 9)

I'm happy to implement a change but would prefer to hear first what people think?

[Andreas-Forster]

The idea is that inexperienced users use use SphericalHarmonicsLight class where we have a guard for the number of coefficients in place:

scalismo-faces/src/main/scala/scalismo/faces/parameters/Illumination.scala

Line 55 in 25ae368

require(coefficients.isEmpty || SphericalHarmonics.totalCoefficients(bands) == coefficients.length, "invalid length of coefficients to build SphericalHarmonicsLight")

For me, there are several options to prevent your accidental miss usage:

• We could protect the access to the shader classes and make them private to scalismo.faces to indicate that they are not meant to be used if you do not really understand what's going on.
• We could allow more than 9 coefficients but truncate to any useful number such that only full bands are used, and print a warning or throw an error if we drop some coefficients.
• We could enforce the types of the coefficient to an explicit class, not allowing even to construct coefficients with an unreasonable number of parameters.
• Leave it as is, assuming not many will fall into the pit and change it when there are more complaints.

My preference would be 1, 3, or 4, but I am open to change my mind if you have a strong preference.

[BernhardEgger]

I would never dare to access any shader class, since I don't understand what's going on (expect when there is a need for it, e.g. to write a CVPR2020 paper with specular albedo maps). As an unexperienced user, I of course only access the beautiful SphericalHarmonicsLight class. I however did not expect that this class would lead me behind the light (not sure if that one works in english).

I however ran into this issue:
I naively thought I could use the StandardFitScript with a higher resolution environment map. To e.g. approximate pointlights better, it makes sense to add more SH coefficients. I added the bands using the following command:
environmentMap = SphericalHarmonicsLight.frontal.withNumberOfBands(sh)) where sh is the number of bands I want to use. This leads to a proper set of SH coefficients that also pass the requirement in Illumination.scala you pointed out.
So adding more bands nicely throws an error if I try to call SHLightSolverProposal with more than 2nd band.
However if I remove the optimizer proposal more than 2nd band can be fitted via sampling.
The sampling then samples as many bands as I wish, but I quickly realized that adding more than 2nd band somehow doesn't improve the results. Even when fixing pose, shape and color. So I got suspicious and tried to approximate a pointlightsource.

So I wrote the following little script that renders a face using more and more bands (attached)
InterpolatePointToAmbient.zip
This made me realize, that whatever the parameters are, it seems like everything after the 2nd band is just ignored and not rendered.
I found the culprit to be in the pixel shader. Where everything after the second band is just ignored. That means the renderer is not capable of rendering beyond 2nd band.
I was suprised that no error is thrown. Especially since some parts of the framework support more than 2 bands.
SphericalHarmonics.scala for example is written in a way to support up to the 4th band.
I was even more surprised to see the reason why the SphericalHarmonicsLambertShader is only supporting environMap.size <=9 which means 2nd bands): the SphericalHarmonicsLight.lambertKernel is written only till the 2nd band. The other necessary component in that shader, SphericalHarmonics.shBasisFunctionDirect could handle up to the 4th band. I implemented the lambertKernel for 3rd and 4th band (unsure if the numbers were correct) and I indeed can render them with the shader when allowing more coeffs.

I would suggest to add a guard for SphericalHarmonicsLight with higher than the 4th band, add the 3rd and 4th band to the lambertKernel and remove the limitation in the SphericalHarmonicsLambertShader.

[Andreas-Forster]

Oh I see. Now it is clear where you run into the problem. Thank you for clarifying the situation that you discovered. It is indeed unnescessary to limit the SH coefficients the way the shader does it if we provide the additional nescessary coeffients in SphericalHarmonicsLight.

I agree that guarding the SphericalHarmonicsLight is a good idea as long as we have this limitation to an upper limit of bands.