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

[Ambarish Rapte]

On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Refactoring and renaming changes to some of the D3D pipeline files and a few changes on the Java side. These are various "leftovers" from previous issues that we didn't want to touch at the time in order to confine the scope of the changes. They will make future work easier.
>> 
>> Since there are many small changes, I'm giving a full list here:
>> 
>> **Java**
>> 
>> * `NGShape3D.java`
>>   * Extracted methods to help with the cumbersome lighting loop: one method per light type + empty light (reset light) + default point light. This section of the code would benefit from the upcoming pattern matching on `switch`.
>>   * Normalized the direction here instead of in the native code.
>>   * Ambient light is now only set when it exists (and is not black).
>> * `NGPointLight,java` - removed unneeded methods that were used by `NGShape3D` before the per-lighting methods were extracted (point light doesn't need spotlight-specific methods since they each have their own "add" method).
>> * `NGSpotLight.java` - removed `@Override` annotations as a result of the above change.
>> 
>> **Native C++**
>> 
>> * Initialized the class members of `D3DLight`, `D3DMeshView`  and `D3DPhongMaterial` in the header file instead of a more cumbersome initialization in the constructor (this is allowed since C++ 11). 
>> * `D3DLight`
>>   * Commented out unused methods. Were they supposed to be used at some point?
>>   * Renamed the `w` component to `lightOn` since it controls the number of lights for the special pixel shader variant and having it in the 4th position of the color array was confusing.
>> * `D3DPhongShader.h`
>>   * Renamed some of the register constants for more clarity.
>>   * Moved the ambient light color constant from the vertex shader to the pixel shader (see the shader section below). I don't understand the calculation of the number of registers in the comment there: "8 ambient points + 2 coords = 10". There is 1 ambient light, what are the ambient points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it is unused.
>>   * Reduced the number of assigned vertex register for the `VSR_LIGHTS` constant since it included both position and color, but color was unused there (it was used directly in the pixel shader), so now it's only the position.
>> * `D3DMeshView.cc`
>>   * Unified the lighting loop that prepares the lights' 4-vetors that are passed to the shaders.
>>   * Removed the direction normalization as stated in the change for `NGShape3D.java`.
>>   * Reordered the shader constant assignment to be the same order as in `D3DPhongShader.h`.
>> 
>> **Native shaders**
>> * Renamed many of the variables to what I think are more descriptive names. This includes noting in which space they exist as some calculations are done in model space, some in world space, and we might need to do some in view space. For vectors, also noted if the vector is to or from (`eye` doesn't tell me if it's from or to the camera).
>> * Commented out many unused functions. I don't know what they are for, so I didn't remove them.
>> * `vsConstants`
>>   * Removed the light color from here since it's unused, only the position is.
>>   * Removed the ambient light color constant from here since it's unused, and added it to `psConstants` instead.
>> * `vs2ps`
>>   * Removed the ambient color interpolation, which frees a register (no change in performance).
>>   * Simplified the structure (what is `LocalBumpOut` and why is it called `light` and contains `LocalBump?).
>> * `Mtl1PS` and `psMath`
>>   * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they are used for better clarity.
>>   * Moved the lights loop to `Mtl1PS`. The calculation itself remains in `psMath`.
>> 
>> No changes in performance were measured and the behavior stayed the same.
>
> Nir Lisker has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unused comments, clean constructor

Providing few minor comments.
I still need to do another pass. All looks good till now.

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`

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 ?

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 42:

> 40:     float position[3] = {0};
> 41:     float color[3] = {0};
> 42:     float lightOn = 0;

how would `isOn` sound ?

modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h line 56:

> 54:     D3DContext *context = NULL;
> 55:     float diffuseColor[4] = {0};
> 56:     float specularColor[4] = {1 , 1, 1, 32};

minor: remove space before comma.

modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongShader.h line 43:

> 41: #define VSR_LIGHT_POS 10  // 1 position = 5 * 1 = 5: c10-14
> 42: // Registers 15-19 free
> 43: #define VSR_LIGHT_DIRS 20    // 1 direction = 5 * 1 = 5: c20-24

Comments need to be changed as maxLights is now 3.
I would not recommend to change the macro values or registers in this PR. Instead mention in the comment that 2 are reserved for future if we want to increase the number of lights.

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psConstants.h line 28:

> 26: // see D3DPhongShader.h for register assignments
> 27: 
> 28: static const int numMaxLights = 3;

May be use same as used in vsConstants.h : `MAX_NUM_LIGHTS` or `MAX_LIGHTS` as recommended in review comment.

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

> 96:  * specular component. The computation is done in world space.
> 97:  */
> 98: void computeLight(float i, float3 n, float3 refl, float specPower, float3 toLight, float3 lightDir, in out float3 d, in out float3 s) {

`toLight` -> `vertexToLightVec` ?

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vs2ps.h line 28:

> 26: struct PsInput {
> 27: 
> 28:     static const float nLights = 3;

May be use same as used in `vsConstants.h` : `MAX_NUM_LIGHTS` or `MAX_LIGHTS` as recommended in review comment.

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.

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

Changes requested by arapte (Reviewer).

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