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

[Ambarish Rapte]

On Tue, 2 Aug 2022 09:47:21 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:
> 
>   Renamed method

Providing few more comments.

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/Mtl1VS.hlsl line 32:

> 30: //float2 transformTexture(float2 t) { return t; }
> 31: 
> 32: VsOutput main(VsInput vsInput) {

The input struct name was earlier passed from build.gradle, https://github.com/openjdk/jfx/blob/08ec9c8781a57b49a13a2b7febbe33172ebc1a5a/build.gradle#L2344

This change needs to be reflected in build.gradle. so, 
either
1. Remove `"/DVertexType=ObjVertex",` in build.gradle 
OR
2. Change `"/DVertexType=ObjVertex",` in build.gradle  to `"/DVertexType=VsInput",` and revert the type function parameter here.

I would recommend option 2, as it would remind us to use this approach in future if we needed multiple types of vs input structs. But I leave it to you.

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

> 24:  */
> 25: 
> 26: struct PsInput {

The `struct VsOutput` is the actual input to pixel shader. Naming this struct as `PsInput` may not be correct.
I would recommend to have only one struct that is `PsInput` and move member variable texD of `VsOutput` to `PsInput`.
In vertex and pixel shaders use correct names for the variables of `PsInput`.
In vertex shader the variable would be names as output, and in pixel shader as input.
For example: https://github.com/caioteixeira/GameEngines/blob/master/lab5/Assets/Shaders/Phong.hlsl

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

> 36:     float3 worldVecToEye               : texcoord2;
> 37:     float3 worldVecsToLights[nLights]  : texcoord3; // 3, 4, 5
> 38:     float3 worldNormLightDirs[nLights] : texcoord6; // 6, 7, 8

This is an existing behavior, and I just want to open it for discussion and bring it to your consideration for future changes.
These are per vertex attributes and they may cause un-required processing/overhead during rasterization.
In a regular scenario where only one light i.e. default light is set, the other two values in these arrays are reset and are unused in pixel shader. But rasterizer interpolates these values for all pixel pass as input to pixel shader.
May be in future we can find a way to avoid this.

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

> 54: }
> 55: 
> 56: void calcLocalBump(float4 modelVertexPos, float4 modelVertexNormal, out PsInput psInput) {

Looks like the function name `calcLocalBump` was in accordance with the structs  `LocalBump` and `LocalBumpOut`. Now, as these structs are getting removed, we can change this name.
May be something like, `transformVertexAttributes()`?

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

> 54: }
> 55: 
> 56: void calcLocalBump(float4 modelVertexPos, float4 modelVertexNormal, out PsInput psInput) {

In continuation to comment in vs2ps.h :
While in vertex shader: `PsInput` is the output of vertex shader. So It would be more suitable to name the variable as `output` or `vsOutput` instead of `psInput`.

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

Changes requested by arapte (Reviewer).

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