On Tue, 2 Aug 2022 09:47:21 GMT, Nir Lisker <[email protected]> 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
Found few minor corrections.
I need to do one more pass at the hlsl code.
modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 227:
> 225: status =
> SUCCEEDED(device->SetPixelShaderConstantF(PSR_MAT_DIFFUSE_COLOR,
> material->getDiffuseColor(), 1));
> 226: if (!status) {
> 227: cout << "D3DMeshView.render() - SetPixelShaderConstantF
> (PSR_DIFFUSECOLOR) failed !!!" << endl;
Minor: in cout, `PSR_DIFFUSECOLOR` -> `PSR_MAT_DIFFUSE_COLOR`
modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 233:
> 231: status =
> SUCCEEDED(device->SetPixelShaderConstantF(PSR_MAT_SPECULAR_COLOR,
> material->getSpecularColor(), 1));
> 232: if (!status) {
> 233: cout << "D3DMeshView.render() - SetPixelShaderConstantF
> (PSR_SPECULARCOLOR) failed !!!" << endl;
Minor: in cout, `PSR_SPECULARCOLOR` -> `PSR_MAT_SPECULAR_COLOR`
modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 245:
> 243: status = SUCCEEDED(device->SetPixelShaderConstantF(PSR_LIGHT_COLOR,
> lightsColor, MAX_NUM_LIGHTS));
> 244: if (!status) {
> 245: cout << "D3DMeshView.render() -
> SetPixelShaderConstantF(PSR_LIGHTCOLOR) failed !!!" << endl;
Minor: in cout, `PSR_LIGHTCOLOR` -> `PSR_LIGHT_COLOR`
-------------
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.org/jfx/pull/789
