On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker <nlis...@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

Reply via email to