RFR: 8234921: Add DirectionalLight to the selection of 3D light types [v2]

[Kevin Rushforth]

On Sat, 14 Aug 2021 13:00:52 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Adds a directional light as a subclass of `LightBase`. I think that this is the correct hierarchy for it.
>>
>> I tried to simulate a directional light by putting a point light far away, but I got artifacts when the distance was large. Instead, I added an on/off attenuation flag as the 4th component of the attenuation 4-vector. When it is 0, a simpler computation is used in the pixel/fragment shader that calculates the illumination based on the light direction only (making the position variables meaningless). When it is 1, the point/spot light computation is used. It's possible that the vertex shader can also be simplified in this case since it does not need to transform the position vectors, but I left this optimization avenue for another time.
>>
>> I noticed a drop of ~1 fps in the stress test of 5000 meshes.
>>
>> I added a system test that verifies the correct color result from a few directions. I also updated the lighting sample application to include 3 directional lights and tested them on all the models visually. The lights seem to behave the way I would expect.
>
> Nir Lisker has updated the pull request incrementally with one additional commit since the last revision:
>
>   Update copyright year

I tested this on all three platforms and it looks good. There are a couple of copy/paste typos where `SpotLight` should be `DirectionalLight`. I also left a question / suggestion for dealing with the boolean. You can either change it, or leave it and add a comment.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/DirectionalLightHelper.java line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

2021

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/DirectionalLightHelper.java line 35:

> 33:
> 34: /**
> 35:  * Used to access internal methods of SpotLight.

Typo: `SpotLight` --> `DirectionalLight`.

modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2Light.java line 68:

> 66:
> 67:     boolean isDirectionalLight() {
> 68:         return isAttenuated < 0.5;

Minor: you can just test for 0 or not 0 since you only ever set them as constants. It's up to you...I don't object to rounding, which is effectively what you've done.

modules/javafx.graphics/src/main/java/javafx/scene/DirectionalLight.java line 43:

> 41:  * A light that illuminates an object from a specific direction.
> 42:  * The direction is defined by the {@link #directionProperty() direction} vector property of the light. The direction
> 43:  * can be rotated by setting a rotation transform on the {@code SpotLight}. For example, if the direction vector is

Typo: `SpotLight` --> `DirectionalLight`

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 54:

> 52:
> 53: bool D3DLight::isDirectionalLight() {
> 54:     return attenuation[3] < 0.5;

Same comment as for `ES2Light`.

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 106:

> 104:
> 105: void computeLight(float i, float3 n, float3 refl, float specPower, float3 L, float3 lightDir, in out float3 d, in out float3 s) {
> 106:     if (gLightAttenuation[i].w < 0.5) {

Is there any performance difference between `< 0.5` and `!= 0` ? I suspect not, but in any case, you might consider the latter (as I also mentioned in the java files). The latter is more clear, so if you choose to stick with what you have, I'd like to see a comment added.

modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main1Light.frag line 122:

> 120:     Light light = lights[i];
> 121:     vec3 lightDir = lightTangentSpaceDirections[i].xyz;
> 122:     if (light.attn.w < 0.5) {

Same comment as D3D shaders.

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

> 83:         pointLight2.setTranslateX(-LIGHT_X_DIST);
> 84:         spotLight2.setTranslateX(-LIGHT_X_DIST);
> 85:

I recommend setting the direction of `directionalLight1` and `directionalLight2` as follows:


directionalLight1.setDirection(new Point3D(-LIGHT_X_DIST, 0, LIGHT_Z_DIST));
directionalLight2.setDirection(new Point3D(LIGHT_X_DIST, 0, LIGHT_Z_DIST));

tests/system/src/test/java/test/javafx/scene/lighting3D/DirectionalLightTest.java line 71:

> 69:
> 70:     @Test
> 71:     public void testSpotlightAttenuation() {

Should be `testDirectionalLight`.

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

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