jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8217853: Cleanup in the D3D native pipeline

Open nlisker opened this issue 3 years ago • 4 comments

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

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

Issue

Reviewers

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

nlisker avatar May 02 '22 16:05 nlisker

:wave: 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.

bridgekeeper[bot] avatar May 02 '22 16:05 bridgekeeper[bot]

/reviewers 2

nlisker avatar May 02 '22 16:05 nlisker

@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).

openjdk[bot] avatar May 02 '22 16:05 openjdk[bot]

Webrevs

mlbridge[bot] avatar May 02 '22 16:05 mlbridge[bot]

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

nlisker avatar Oct 18 '22 13:10 nlisker

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!

  1. Right, I reverted this line.
  2. Right, it seems to be a problem in the native level, will have to check carefully.
  3. 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 avatar Dec 13 '22 02:12 nlisker

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.

nlisker avatar Dec 25 '22 03:12 nlisker

@arapte Please re-review.

nlisker avatar Dec 28 '22 16:12 nlisker

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?

mstr2 avatar Jan 04 '23 22:01 mstr2

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.

nlisker avatar Jan 04 '23 23:01 nlisker

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?

mstr2 avatar Jan 05 '23 00:01 mstr2

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.

nlisker avatar Jan 05 '23 00:01 nlisker

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 55fe2dc7371f6dcb12c414c5d672728e47e9c504 has resolved my issue.

I'll re-review it now.

kevinrushforth avatar Jan 09 '23 17:01 kevinrushforth

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

kevinrushforth avatar Jan 09 '23 17:01 kevinrushforth

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

  1. The ambient light doesn't go back to black when an ambient light is added then removed.
  2. Adding a second point light (or spot light) causes no lighting to be done, as if there were no lights
  3. 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).

kevinrushforth avatar Jan 09 '23 18:01 kevinrushforth

@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:

  • 357cd8563bd6ca47afd28dd1481afbe2d2458827: 8282386: JavaFX media stubs rely on libav.org
  • bca1bfc5e3b04293c13417fd923d99864cdd9147: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
  • ca29cc6122010e4b94778cc658efd4fdddc8af67: 8298728: Cells in VirtualFlow jump after resizing
  • e866a6c35c9040115dd62c6c04fd4b3d4d0e1324: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
  • 0dbc4484c96859c6106bb4c62bff261c7e901aed: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
  • 94fb7ede2dcde306a631d1feb0107c96a8221452: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
  • 012fa16faccaf9b6a1f56cd3b5450ab200bdec9c: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
  • a35c3bf78b86c57d6e80d592e99f16ab349b0d8c: 8292922: [Linux] No more drag events when new Stage is created in drag handler
  • 48f6f5ba5b1f38281b38cd1f29a215f795ba0309: 8299272: Update copyright header for files modified in 2022
  • 5b96d348ebcabb4b6d2e1d95937de3c82a1f6876: 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.

openjdk[bot] avatar Jan 10 '23 14:01 openjdk[bot]

/integrate

nlisker avatar Jan 10 '23 14:01 nlisker

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

  • 357cd8563bd6ca47afd28dd1481afbe2d2458827: 8282386: JavaFX media stubs rely on libav.org
  • bca1bfc5e3b04293c13417fd923d99864cdd9147: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
  • ca29cc6122010e4b94778cc658efd4fdddc8af67: 8298728: Cells in VirtualFlow jump after resizing
  • e866a6c35c9040115dd62c6c04fd4b3d4d0e1324: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
  • 0dbc4484c96859c6106bb4c62bff261c7e901aed: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
  • 94fb7ede2dcde306a631d1feb0107c96a8221452: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
  • 012fa16faccaf9b6a1f56cd3b5450ab200bdec9c: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
  • a35c3bf78b86c57d6e80d592e99f16ab349b0d8c: 8292922: [Linux] No more drag events when new Stage is created in drag handler
  • 48f6f5ba5b1f38281b38cd1f29a215f795ba0309: 8299272: Update copyright header for files modified in 2022
  • 5b96d348ebcabb4b6d2e1d95937de3c82a1f6876: 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[bot] avatar Jan 10 '23 14:01 openjdk[bot]

@nlisker Pushed as commit 4a2780134b88e86e437ae2bdffe06b145788e61d.

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

openjdk[bot] avatar Jan 10 '23 14:01 openjdk[bot]

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.

nlisker avatar Jan 10 '23 22:01 nlisker

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.

kevinrushforth avatar Jan 11 '23 13:01 kevinrushforth

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.

nlisker avatar Jan 11 '23 17:01 nlisker