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

[Kevin Rushforth]

On Thu, 22 Oct 2020 18:29:19 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> Added a SpotLight only to the D3D pipeline currently.
> 
> ### API
> 
> 1. 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.
> 
> 2. 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?
> 
> CSR will be created when the API review is finished.
> 
> ### Implementation
> 
> I've gotten advice from a graphics engineer to treat point lights as spot lights with a 360 degrees coverage, which simplifies the shader a lot and removes branching over the light type. This is covered anyway by a possible optimization in the pixel shader, which I've noted in an inline comment, that skips the spotlight computation if `falloff` is 0 (this is the case for non-spotlights).
> 
> ### Performance
> 
> Testing 3 point lights in order to compare with the [pre-patch performance](https://github.com/openjdk/jfx/pull/43#issuecomment-600632114):
> 
> Without the falloff == 0 branching:
> sphere 1000 subdivisions: 120
> mesh 5000: 9.5
> mesh 200: 111.5
> 
> With the falloff == 0 branching:
> sphere 1000 subdivisions: 120
> mesh 5000: 9.3
> mesh 200: 112.5
> 
> Ubuntu 20:
> With the patch:
> Mesh 200: 60 fps
> Mesh 5000: 15 fps
> Sphere 1000: 60 fps
> 
> Without the patch (master branch)
> Mesh 200: 60 fps
> Mesh 5000: 16.3 fps
> Sphere 1000: 60 fps
> 
> So no major changes. I will repeat these tests to make sure there was no mistake.

I think the API looks good with a few comments about valid ranges.

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 48:

> 46:  * outer angle}, and a {@link #falloffProperty() falloff} factor. For a point whose angle to the light is {@code a}, if
> 47:  * {@code a < innerAngle} then that point receives maximum illumination, if {@code a > outerAngle} then that point
> 48:  * receives no illumination, and if {@code innerAngle < a < outerAngle} then the illumination is determined by the

that should be `innerAngle <= a <= outerAngle`

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 59:

> 57:  * effect by setting {@code innerAngle} to be larger than {@code outerAngle} and lowering {@code maxRange}.
> 58:  * <p>
> 59:  * <img src="doc-files/spotlightRing.png" alt="Image of the ring light effect">

I don't see the value in supporting this type of effect. It would potentially constrain the implementation and would complicate the description of the lighting equation.

Without this complication, the equation as described in the earlier paragraph is effectively:

if (a < innerAngle) {
    I = 1
} else if (a > outerAngle) {
    I = 0
} else {
    I = pow((cos(a) - cos(outer)) / (cos(inner) - cos(outer)), falloff)
}
In order to support the ring effect, you would need define the equation differently (i.e., it doesn't just naturally derive from the above equation). In any case, it seems best to just remove this, and document that the valid range of `outerAngle` is `[innerAngle, 180]`. We could clamp on usage as we do in a few other areas. Similarly, we probably want to document that `falloff` should be non-negative.

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 163:

> 161:      * receive no light. At smaller angles, the light intensity starts to increase.
> 162:      * <p>
> 163:      * The angle is clamped between 0 and 180.

between innerAngle and 180?

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 165:

> 163:      * The angle is clamped between 0 and 180.
> 164:      *
> 165:      * @defaultValue 90

Should this be 180 be default? I don't have a strong opinion on this.

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 194:

> 192:      * values are allowed, but give unrealistic lighting.
> 193:      *
> 194:      * @defaultValue 1

Maybe it should default to 0? I don't have a strong opinion on this either.

I do think that it should say that values < 0 are clamped to 0.

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

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