RFR: 8234920: Add SpotLight to the selection of 3D light types [v9]

[Kevin Rushforth]

On Mon, 8 Feb 2021 07:04:05 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could also be a subclass of `PointLight` as it's a point light with direction and extra factors. I saw that `scenario.effect.light.SpotLight` extends its respective `PointLight`, but it's not a perfect analogy. In the end, I think it's a questions of whether `PointLight` will be expanded in a way which doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).  I, personally, find it more intuitive that these are the "radius angles", so half these angles, as used in the spotlight factor formula. Do you think I can change this or do you prefer the current definition of the angles?
>> 
>> - [ ] The current implementation uses an ad-hoc direction property (using a `Point3D`). It crossed my mind that we could use the rotation transforms of the node to control the direction instead, just like we use the translation/layout of the node to get the position (there is an internal Affine3D transform for lights, not sure why `AmbientLight` needs it). Wouldn't that make more sense? When I rotate the light I would expect to see a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as spot lights with a 360 degrees coverage, which simplifies a few places. We can still try to optimize for a point light by looking at the light parameters: `falloff = 0` and `outerAngle = 180`. These possible optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the pixel/fragment shaders in the form of 3 different ways to compute the spotlight factor (the `computeLightN` methods). We need to check which of these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light type and using precalculated values (in `D3DMeshView.cc` for d3d and `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing the spotlight factor contributions (`computeSpotlightFactor`, `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable considering it will not be the bottleneck. We can remove these if we want to simplify, though maybe if we allow a large number of lights it could make a difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several occasions and got different results in terms of absolute numbers, but the relative performance difference remained more or less the same. Out of the 3 `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with `computeSpotlightFactor` > `computeSpotlightFactor2`  > `computeSpotlightFactor3` at in terms of performance (~2 fps difference each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave the best performances.
>
> Nir Lisker has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Merge remote-tracking branch
>  - Speculative fix for Mac
>  - Fixed shader field name

Finally getting back to this.

There is an outstanding API question regarding the direction vector: Should the transforms of the SpotLight node to scene coordinates affect the direction vector (e.g., such that a rotation would rotate the direction the spotlight is pointing)? If so, do we even need the direction vector or could we define the direction as `(0,0,1)` in the local coordinates of the SpotLight, and tell applications to apply the appropriate rotation to define the direction.

I think for consistency, the answer is "Yes" to the first question (that's how the position works and it would be odd for the transform to affect the position and not the direction). I'm less certain about the answer to the second question. Without utility functions like `lookAt` to help compute the appropriate transform, it seems important to be able to specify the direction as an explicit parameter. And even if we had utility functions, an app might want the control (although you might argue it would be unneeded). My instinct is to keep it as defined. If we go with this, the only change that is needed is a note that the transforms applied to the SpotLight node affect it's direction as well as its position.

Implementation:

Can you add a system test similar to what you did for attenuation to verify the basic functionality?

I tested this a bit on Windows using D3D and it behaves as I would expect, although I didn't do exhaustive testing.

On Mac I no longer get a GLSL shader error at runtime, but spotlights aren't working correctly, either. The falloff happens too fast (at 1 you can't see the light at all). For the inner/outer angles, something looks backwards. See the attached image
<img width="1212" alt="SpotLight-macOS" src="https://user-images.githubusercontent.com/34689748/111841271-f293d000-88ba-11eb-915c-38e2c69f5e70.png">

I left a few inline comments, although I haven't reviewed the shader files in detail yet.

modules/javafx.graphics/src/main/native-prism-d3d/D3DContext.cc line 514:

> 512: /*
> 513:  * Class:     com_sun_prism_d3d_D3DContext
> 514:  * Method:    nSetSpotLight

Typo: `nSetSpotLight` --> `nSetLight`

modules/javafx.graphics/src/main/native-prism-d3d/D3DContext.cc line 523:

> 521:         jfloat dirX, jfloat dirY, jfloat dirZ, jfloat innerAngle, jfloat outerAngle, jfloat falloff)
> 522: {
> 523:     TraceLn(NWT_TRACE_INFO, "D3DContext_nSetSpotLight");

Typo: `D3DContext_nSetSpotLight` `D3DContext_nSetLight`

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.h line 47:

> 45:         float r, float g, float b, float w,
> 46:         float ca, float la, float qa, float maxRange,
> 47:         float dirX, float dirY, float firZ,

Typo: `firZ` --> `dirZ`

tests/performance/3DLighting/attenuation/Environment.java line 26:

> 24:  */
> 25: 
> 26: package attenTest;

Revert this (else it won't compile)

tests/performance/3DLighting/attenuation/FPSCounter.java line 26:

> 24:  */
> 25: 
> 26: package attenTest;

Revert this (else it won't compile)

tests/performance/3DLighting/attenuation/LightingSample.java line 26:

> 24:  */
> 25: 
> 26: package attenTest;

Revert this (else it won't compile)

-------------

PR: https://git.openjdk.java.net/jfx/pull/334