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

Nir Lisker nlisker at openjdk.org
Sun Dec 25 03:49:20 UTC 2022


> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 17 additional commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into
   8217853_Cleanup_in_the_D3D_native_pipeline
 - Changed the order of semantics and moved constant out of struct
 - Add reserved registers comment
 - Change xyz to rgb
 - Added a default light mode
 - Reverted a change that breaks ambient lighting
 - Addressed review comment
 - Addressed review comments
 - undo debugging
 - Merge remote-tracking branch 'origin/master' into 8217853_Cleanup_in_the_D3D_native_pipeline
 - ... and 7 more: https://git.openjdk.org/jfx/compare/ab956ab5...856a9db4

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

Changes:
  - all: https://git.openjdk.org/jfx/pull/789/files
  - new: https://git.openjdk.org/jfx/pull/789/files/55fe2dc7..856a9db4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=789&range=09
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=789&range=08-09

  Stats: 172576 lines in 2618 files changed: 24191 ins; 130956 del; 17429 mod
  Patch: https://git.openjdk.org/jfx/pull/789.diff
  Fetch: git fetch https://git.openjdk.org/jfx pull/789/head:pull/789

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


More information about the openjfx-dev mailing list