RFR: 8217853: Cleanup in the D3D native pipeline [v2]

[Ambarish Rapte]

On Sat, 30 Jul 2022 11:01:59 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java line 201:
>> 
>>> 199: 
>>> 200:     /**
>>> 201:      * If no lights are in the scene, add a default white point light at the camera's. The light uses the default
>> 
>> minor: `at the camera's` -> `at the camera's (eye) position`
>> 
>> Additionally would recommend to move the first line `If no lights are in the scene, add a default white point light at the camera's position.` above line number 128 before calling `createDefaultLight`
>
> I thought that the code
> 
>         if (noLights(lights)) {
>             createDefaultLight(g);
> 
> speaks for itself: if there are no lights, add a default light. The details of what that light is are in the method doc. Can still add a comment there.

Sounds good. minor: On another thought, how about naming the method as `setDefaultLight`.

>> modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java line 211:
>> 
>>> 209:                 (float) cameraPos.x, (float) cameraPos.y, (float) cameraPos.z,
>>> 210:                 1.0f, 1.0f, 1.0f, 1.0f,
>>> 211:                 NGPointLight.getDefaultCa(), NGPointLight.getDefaultLa(), NGPointLight.getDefaultQa(), 0,
>> 
>> `isAttenuated` was passed as 1 earlier. Is changing it to `0` works same ?
>
> Yes, in `PsMath.h::computeLight`, there is a check if the light requests attenuation. In general, this is done only for directional lights (that are not attenuated), but in this case we know that this point light is not attenuated so passing 0 skips the redundant calculation. In theory this would improve performance, but because this light can only exist alone I don't expect anything measurable.

I see 4 tests in `PointLightIlluminationTest` fail due to this change, on MacOS and Windows.
Tests: 
sphereLowerRightPixelColorShouldBeDarkRed
sphereUpperRightPixelColorShouldBeDarkRed
sphereUpperLeftPixelColorShouldBeDarkRed
sphereLowerLeftPixelColorShouldBeDarkRed

Error:
expected:rgba(139,0,0,255) but was:rgba(180,0,0,255)

The tests pass if isAttenuated is 1.

>> modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h line 36:
>> 
>>> 34: static const int isSkinned = Skin;
>>> 35: 
>>> 36: static const int maxLights = 3;
>> 
>> May be name as `MAX_LIGHTS` similar to `static const int MAX_BONES = 70;` on line 28 in same file. ?
>> of preferably use `MAX_NUM_LIGHTS` which we already use in .h, .cpp files.
>
> The max lights limitation is going to be removed in the following change set (at least I intend to do it), so I don't think it's worth touching all those constants.

Ok, that change would touch glsl shaders too. Sounds good, we can talk about it with that change.

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

PR: https://git.openjdk.org/jfx/pull/789