8217853: Cleanup in the D3D native pipeline #789

nlisker · 2022-05-02T16:05:08Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8217853: Cleanup in the D3D native pipeline

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/789/head:pull/789
$ git checkout pull/789

Update a local copy of the PR:
$ git checkout pull/789
$ git pull https://git.openjdk.org/jfx pull/789/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 789

View PR using the GUI difftool:
$ git pr show -t 789

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/789.diff

bridgekeeper · 2022-05-02T16:05:12Z

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

nlisker · 2022-05-02T16:05:18Z

/reviewers 2

openjdk · 2022-05-02T16:07:16Z

@nlisker
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with 1 of role reviewers, 1 of role authors).

mlbridge · 2022-05-02T16:11:19Z

Webrevs

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.h

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h

arapte

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

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h

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

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psConstants.h

modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongMaterial.h

modules/javafx.graphics/src/main/native-prism-d3d/D3DPhongShader.h

arapte

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

arapte

Providing few more comments.

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

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

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

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

…the_D3D_native_pipeline

arapte

Looks good to me,
pending one change in vertex shader, ongoing conversation.

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

arapte

LGTM,
suggested a minor correction in comment.
I will re-approve if any more changes.

arapte · 2022-09-01T15:02:17Z

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

@@ -26,7 +26,8 @@
 #include "vsDecl.h"
 #include "vsMath.h"

-VsOutput main(VsInput vsInput) {
+// VertexType = VsInput as defined the gradle build file


Minor:- VertexType = VsInput, as defined in the build.gradle file

nlisker · 2022-10-18T13:47:21Z

@kevinrushforth If this fell through the cracks please add it to the list too :) It's a blocker for future work.

kevinrushforth

I found at least three regressions in behavior:

The ambient light doesn't go back to black when an ambient light is added then removed.
Adding a second point light (or spot light) causes no lighting to be done, as if there were no lights
The magenta point light (and spot light) no longer works at all.

See inline comments for the first problem.

To reproduce the second, run the LightingSample, show a Sphere, select the red point light (correctly shows a red point light on the right), then select the blue point light (no lighting happens).

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java

nlisker · 2022-12-13T02:40:35Z

At first I couldn't reproduce these, then I found out the launch command of the test app points to the wrong build artifacts, so any changes I made were not reflected in my visual checks. It's the same for other test apps, so it's a good thing it was discovered. Thanks!

Right, I reverted this line.
Right, it seems to be a problem in the native level, will have to check carefully.
I think it just looks like that because of the position of the central lights (the magenta ones). The sphere intersects the light upon creation, so there is no lighting of the sphere with this geometry (rotate the camera sideways to see it). Try to animate the sphere, or use the boxes/meshes instead that are created further away from the light.

I'm also adding a default light mode to the test app to make sure it works correctly when there are no other lights in the scene.

nlisker · 2022-12-25T03:28:57Z

For reasons that I don't understand, the problem was the order of the semantics declared in the vs2ps struct. Moving float2 texD : texcoord0; down fixed the issue. I played a bit with the ordering out of curiosity and got different results. I didn't see anything in the documentation that talks about the order of the interpolated variables. This fixed problem 2 for me.

8217853_Cleanup_in_the_D3D_native_pipeline

nlisker · 2022-12-28T16:52:19Z

@arapte Please re-review.

mstr2 · 2023-01-04T22:58:43Z

For reasons that I don't understand, the problem was the order of the semantics declared in the vs2ps struct. Moving float2 texD : texcoord0; down fixed the issue. I played a bit with the ordering out of curiosity and got different results. I didn't see anything in the documentation that talks about the order of the interpolated variables. This fixed problem 2 for me.

I tested this behavior at commit bb9f802, and can't observe any visual difference with regards to the order of texD in the PsInput struct. Adding two points lights works fine in either case.

Are you sure that this isn't an artifact of your local build? I notice that Gradle doesn't seem to pick up changes in HLSL headers, so I have to clean the build artifacts to prevent Gradle from skipping :graphics:generateD3DHeaders. Maybe you changed several things, and one of the other changes happened to fix the original problem?

nlisker · 2023-01-04T23:30:32Z

I tested this behavior at commit bb9f802, and can't observe any visual difference with regards to the order of texD in the PsInput struct. Adding two points lights works fine in either case.

Can you reproduce Kevin's observation at 1f66f61? If yes, which commit fixed it for you? For me it's the one that changed the order of the fields.

Are you sure that this isn't an artifact of your local build? I notice that Gradle doesn't seem to pick up changes in HLSL headers, so I have to clean the build artifacts to prevent Gradle from skipping :graphics:generateD3DHeaders. Maybe you changed several things, and one of the other changes happened to fix the original problem?

I delete the hlsl shaders every build so that they will be recreated. Still, there could be some caching involved that I couldn't spot. I changed the order of the variables in the struct back and forth and tried several orders to make sure it's not a coincidence and the results were consistant.

mstr2 · 2023-01-05T00:41:48Z

Can you reproduce Kevin's observation at 1f66f61? If yes, which commit fixed it for you? For me it's the one that changed the order of the fields.

With 1f66f61, I can reproduce Kevin's first observation (ambient light doesn't go back to black), but not the other two.
Adding a second point light works just as I'd expect it, and the magenta point light also works as expected.

With the next commit 3142908, I can confirm that the ambient light issue is fixed (it goes back to black when I uncheck the ambient light toggle). I've also triple-checked that I'm pointing the LightingSample application at the correct JavaFX build artifacts.

I'm running the application on a PC with a dedicated NVIDIA GeForce GTX 970 with driver version 511.23.
What are you and @kevinrushforth running this on? Maybe there's some undefined behavior going on, and changing the order of fields just happened to hide the issue on your machine?

nlisker · 2023-01-05T00:54:24Z

The ambient light issue was on the Java side. The purple light is probably just a false observation because it starts in a position where it doesn't apply its light. The real mystery is the problem with the two point lights.

I have an AMD 470.

kevinrushforth · 2023-01-09T17:42:45Z

Can you reproduce Kevin's observation at 1f66f61? If yes, which commit fixed it for you? For me it's the one that changed the order of the fields.

Btw, I can confirm that yes, this fixed it for me. Specifically, commit 55fe2dc has resolved my issue.

I'll re-review it now.

kevinrushforth · 2023-01-09T17:44:31Z

Btw, I am running this on an Intel UHD Graphics 630.

kevinrushforth · 2023-01-09T18:46:20Z

To close the loop on the three problems I reported earlier:

The ambient light doesn't go back to black when an ambient light is added then removed.

Adding a second point light (or spot light) causes no lighting to be done, as if there were no lights

The magenta point light (and spot light) no longer works at all.

I can confirm that 1 and 2 are fixed, and 3 isn't an issue, but is just how the test app works (it behaves the same with or without this patch, and shows up fine when I animate the sphere).

arapte

LGTM

openjdk · 2023-01-10T14:42:39Z

@nlisker This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8217853: Cleanup in the D3D native pipeline

Reviewed-by: arapte, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 27 new commits pushed to the master branch:

357cd85: 8282386: JavaFX media stubs rely on libav.org
bca1bfc: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
48f6f5b: 8299272: Update copyright header for files modified in 2022
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
... and 17 more: https://git.openjdk.org/jfx/compare/6f36e7043299bfb9edf8befbca1e45a938077e4b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

nlisker · 2023-01-10T14:43:27Z

/integrate

openjdk · 2023-01-10T14:46:57Z

Going to push as commit 4a27801.
Since your change was applied there have been 27 commits pushed to the master branch:

357cd85: 8282386: JavaFX media stubs rely on libav.org
bca1bfc: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
ca29cc6: 8298728: Cells in VirtualFlow jump after resizing
e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
48f6f5b: 8299272: Update copyright header for files modified in 2022
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
... and 17 more: https://git.openjdk.org/jfx/compare/6f36e7043299bfb9edf8befbca1e45a938077e4b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-10T14:47:23Z

@nlisker Pushed as commit 4a27801.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

nlisker · 2023-01-10T22:43:30Z

Btw, I can confirm that yes, this fixed it for me. Specifically, commit 55fe2dc has resolved my issue.

If you have time, it would be interesting check if it breaks for you by changing the order of the semantics. It might be worth adding a comment there because it's a rather surprising side effect.

kevinrushforth · 2023-01-11T13:52:13Z

Btw, I can confirm that yes, this fixed it for me. Specifically, commit 55fe2dc has resolved my issue.

If you have time, it would be interesting check if it breaks for you by changing the order of the semantics. It might be worth adding a comment there because it's a rather surprising side effect.

Which part of the change, exactly, do you mean? The part that moved texcoord0 to the beginning of the struct? I could take a quick look after JavaFX 20 RDP1.

nlisker · 2023-01-11T17:16:28Z

The change that moved float2 texD : texcoord0; from the beginning to the end is the one that fixed it for me, and apparently for you. If you can move it back to the beginning of the struct and check if it fails, it would be helpful. I will add a comment there in a later PR.

Initial commit

8db9c8b

openjdk bot added the rfr label May 2, 2022

Remove unused comments, clean constructor

05281ba

kevinrushforth requested review from kevinrushforth and arapte May 3, 2022 22:45

mstr2 reviewed May 6, 2022

View reviewed changes

modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.h Show resolved Hide resolved

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc Outdated Show resolved Hide resolved

modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.h Outdated Show resolved Hide resolved

brcolow reviewed Jun 1, 2022

View reviewed changes

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psMath.h Outdated Show resolved Hide resolved

arapte suggested changes Jul 29, 2022

View reviewed changes

nlisker added 3 commits July 30, 2022 14:03

Addressed review comments

25f5ff7

Missed review comment

025feea

Renamed method

689497a

arapte suggested changes Aug 2, 2022

View reviewed changes

nlisker added 5 commits August 13, 2022 02:44

Address review comments and fix attempt for attenuation

df6f780

debugging

7916eb6

Merge remote-tracking branch 'origin/master' into 8217853_Cleanup_in_…

195c30f

…the_D3D_native_pipeline

undo debugging

6d86905

Addressed review comments

72305b3

arapte reviewed Aug 26, 2022

View reviewed changes

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/Mtl1VS.hlsl Outdated Show resolved Hide resolved

Addressed review comment

1f66f61

arapte approved these changes Sep 1, 2022

View reviewed changes

kevinrushforth requested changes Dec 10, 2022

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java Outdated Show resolved Hide resolved

nlisker added 2 commits December 13, 2022 04:43

Reverted a change that breaks ambient lighting

3142908

Added a default light mode

42994e9

nlisker added 2 commits December 24, 2022 20:58

Add reserved registers comment

05b589d

Changed the order of semantics and moved constant out of struct

55fe2dc

nlisker added 3 commits December 25, 2022 05:44

Merge remote-tracking branch 'origin/master' into

856a9db

8217853_Cleanup_in_the_D3D_native_pipeline

Removed unused fields

ee27969

Changed comment as suggested

bb9f802

kevinrushforth approved these changes Jan 9, 2023

View reviewed changes

kevinrushforth requested a review from arapte January 9, 2023 20:34

arapte approved these changes Jan 10, 2023

View reviewed changes

openjdk bot added the ready label Jan 10, 2023

openjdk bot added the integrated label Jan 10, 2023

openjdk bot closed this Jan 10, 2023

openjdk bot removed ready rfr labels Jan 10, 2023

nlisker deleted the 8217853_Cleanup_in_the_D3D_native_pipeline branch January 10, 2023 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8217853: Cleanup in the D3D native pipeline #789

8217853: Cleanup in the D3D native pipeline #789

nlisker commented May 2, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 2, 2022

nlisker commented May 2, 2022

openjdk bot commented May 2, 2022

mlbridge bot commented May 2, 2022 •

edited

Loading

arapte left a comment

arapte left a comment

arapte left a comment

arapte left a comment

arapte left a comment

arapte Sep 1, 2022 •

edited

Loading

nlisker commented Oct 18, 2022

kevinrushforth left a comment •

edited

Loading

nlisker commented Dec 13, 2022 •

edited

Loading

nlisker commented Dec 25, 2022

nlisker commented Dec 28, 2022

mstr2 commented Jan 4, 2023

nlisker commented Jan 4, 2023

mstr2 commented Jan 5, 2023

nlisker commented Jan 5, 2023

kevinrushforth commented Jan 9, 2023

kevinrushforth commented Jan 9, 2023

kevinrushforth commented Jan 9, 2023

arapte left a comment

openjdk bot commented Jan 10, 2023

nlisker commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

nlisker commented Jan 10, 2023

kevinrushforth commented Jan 11, 2023

nlisker commented Jan 11, 2023

8217853: Cleanup in the D3D native pipeline #789

8217853: Cleanup in the D3D native pipeline #789

Conversation

nlisker commented May 2, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 2, 2022

nlisker commented May 2, 2022

openjdk bot commented May 2, 2022

mlbridge bot commented May 2, 2022 • edited Loading

Webrevs

arapte left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

arapte Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

nlisker commented Oct 18, 2022

kevinrushforth left a comment • edited Loading

Choose a reason for hiding this comment

nlisker commented Dec 13, 2022 • edited Loading

nlisker commented Dec 25, 2022

nlisker commented Dec 28, 2022

mstr2 commented Jan 4, 2023

nlisker commented Jan 4, 2023

mstr2 commented Jan 5, 2023

nlisker commented Jan 5, 2023

kevinrushforth commented Jan 9, 2023

kevinrushforth commented Jan 9, 2023

kevinrushforth commented Jan 9, 2023

arapte left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 10, 2023

nlisker commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

nlisker commented Jan 10, 2023

kevinrushforth commented Jan 11, 2023

nlisker commented Jan 11, 2023

nlisker commented May 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 2, 2022 •

edited

Loading

arapte Sep 1, 2022 •

edited

Loading

kevinrushforth left a comment •

edited

Loading

nlisker commented Dec 13, 2022 •

edited

Loading