Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker 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 modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h line 95: > 93: > 94: /* > 95: * Computes the light's contribution by using the Phong shading model. A > contribution consist of a diffuse component and a Tiny nit: consist => consists - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:13:55 GMT, Michael Strauß wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unused comments, clean constructor
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 149:
>
>> 147: float spotLightsFactors[MAX_NUM_LIGHTS * 4]; // 2 angles + 1
>> falloff + 1 padding
>> 148: for (int i = 0, d = 0, p = 0, c = 0, a = 0, r = 0, s = 0; i <
>> MAX_NUM_LIGHTS; i++) {
>> 149: D3DLight light = lights[i];
>
> You're invoking the auto-generated copy constructor of `D3DLight` here, where
> the original code didn't do that. Just making sure that that's what you
> intended.
I will change to `D3DLight& light = lights[i];`.
-
PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:09:13 GMT, Michael Strauß wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 34: > >> 32: public: >> 33: D3DLight() = default; >> 34: virtual ~D3DLight() = default; > > It doesn't seem like this class is supposed to be subclassable. I would > suggest removing the constructor and destructor declarations and marking the > class `final`. It might be subclassed in the future. One of the next changes will be a performance upgrade attempt, and we might need to separate the light types instead of bundling them into one that simulates the others. In theory, this class can be removed even, and instead the Java code could call the rendering pipeline directly with all the needed parameters. It's a more intrusive change though, and might as well wait for Panama with this one. - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Fri, 6 May 2022 14:21:58 GMT, Michael Strauß wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused comments, clean constructor > > modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.h line 61: > >> 59: bool lightsDirty = TRUE; >> 60: int cullMode = D3DCULL_NONE; >> 61: bool wireframe = FALSE; > > It seems like you're using `false` and `FALSE` interchangably (see > `D3DPhongMaterial.h` L58). I would suggest using the `false` keyword with the > builtin type `bool`, and the `FALSE` constant with the Win32 type `BOOL`. The original code used the Win-only type. I thought it should be changed too. I should probably do that, - PR: https://git.openjdk.java.net/jfx/pull/789
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
On Mon, 2 May 2022 19:55:28 GMT, Nir Lisker 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
modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h line 34:
> 32: public:
> 33: D3DLight() = default;
> 34: virtual ~D3DLight() = default;
It doesn't seem like this class is supposed to be subclassable. I would suggest
removing the constructor and destructor declarations and marking the class
`final`.
modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 149:
> 147: float spotLightsFactors[MAX_NUM_LIGHTS * 4]; // 2 angles + 1
> falloff + 1 padding
> 148: for (int i = 0, d = 0, p = 0, c = 0, a = 0, r = 0, s = 0; i <
> MAX_NUM_LIGHTS; i++) {
> 149: D3DLight light = lights[i];
You're invoking the auto-generated copy constructor of `D3DLight` here, where
the original code didn't do that. Just making sure that that's what you
intended.
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/789/files - new: https://git.openjdk.java.net/jfx/pull/789/files/8db9c8ba..05281ba3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=789=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=789=00-01 Stats: 21 lines in 2 files changed: 0 ins; 18 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/789.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/789/head:pull/789 PR: https://git.openjdk.java.net/jfx/pull/789
