Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8234920: Add SpotLight to the selection of 3D light types #334

Conversation

nlisker
Copy link
Collaborator

@nlisker nlisker commented Oct 22, 2020

Added a SpotLight only to the D3D pipeline currently.

API discussion points

  • Added SpotLight as a subclass of LightBase. However, it could also be a subclass of PointLight as it's a point light with direction and extra factors. I saw that scenario.effect.light.SpotLight extends its respective PointLight, but it's not a perfect analogy. In the end, I think it's a questions of whether PointLight will be expanded in a way which doesn't not suit SpotLight, and I tend to think that the answer is no.

  • The inner and outer angles are the "diameter angles" as shown here. I, personally, find it more intuitive that these are the "radius angles", so half these angles, as used in the spotlight factor formula. Do you think I can change this or do you prefer the current definition of the angles?

  • The current implementation uses an ad-hoc direction property (using a Point3D). It crossed my mind that we could use the rotation transforms of the node to control the direction instead, just like we use the translation/layout of the node to get the position (there is an internal Affine3D transform for lights, not sure why AmbientLight needs it). Wouldn't that make more sense? When I rotate the light I would expect to see a change in direction.

Implementation discussion points

  • I've gotten advice from a graphics engineer to treat point lights as spot lights with a 360 degrees coverage, which simplifies a few places. We can still try to optimize for a point light by looking at the light parameters: falloff = 0 and outerAngle = 180. These possible optimization exist in ES2PhongShader.java and D3DMeshView.cc, and in the pixel/fragment shaders in the form of 3 different ways to compute the spotlight factor (the computeLightN methods). We need to check which of these give the best results.

Performance

Testing 3 point lights and comparing this branch with master using a 1000 division sphere, 200 meshes, and 5000 meshes.
Using an AMD RX 470 4GB GPU.

In this branch, there is a possible CPU optimization for checking the light type and using precalculated values (in D3DMeshView.cc for d3d and ES2PhongShader.java for opengl). On the GPU, I tried 3 ways of computing the spotlight factor contributions (computeSpotlightFactor, computeSpotlightFactor2 and computeSpotlightFactor3) trying out different branching and shortcuts.

Results

The CPU "optimizations" made no difference, which is understandable considering it will not be the bottleneck. We can remove these if we want to simplify, though maybe if we allow a large number of lights it could make a difference (I doubt it). I don't have a strong preference either way.

The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).

Win 10
Compared with the master branch, this patch shows 5-10 fps drop in the mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several occasions and got different results in terms of absolute numbers, but the relative performance difference remained more or less the same. Out of the 3 computeSpotlightFactor methods, computeSpotlightFactor3, which has no "optimizations", gives slightly better performance.

Ubuntu 18
The mesh 200 test always gave 60 fps because it is locked to this fps, so we can't measure the real GPU performance change.
The mesh 5000 test shows 2-6 fps drop from master, with computeSpotlightFactor > computeSpotlightFactor2 > computeSpotlightFactor3 at in terms of performance (~2 fps difference each).

Conclusion: we can expect a 5 fps drop more or less with 3 point lights. computeSpotlightFactor3 on d3d and computeSpotlightFactor on opengl gave the best performances.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8234920: Add SpotLight to the selection of 3D light types

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/334/head:pull/334
$ git checkout pull/334

Update a local copy of the PR:
$ git checkout pull/334
$ git pull https://git.openjdk.java.net/jfx pull/334/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 334

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/334.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 22, 2020

👋 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
Copy link
Collaborator Author

@nlisker nlisker commented Oct 22, 2020

/csr
/reviewers 2

@openjdk openjdk bot added the csr label Oct 22, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@nlisker has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@nlisker please create a CSR request and add link to it in JDK-8234920. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@nlisker
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Oct 26, 2020

I suggest we start with looking at the API and the subclass question. This will unblock the CSR process.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 27, 2020

My preference would be for SpotLight to subclass PointLight. I also somewhat prefer the 1/2 angle (radius) rather than diameter to specify the spread angles (inner and outer).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 27, 2020

Not that it's all that relevant, but I will note that the (very old) Java 3D API also had SpotLight as a subclass of PointLight.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Oct 30, 2020

Is the old implementation worth looking at, or is is completely different?

I updated the API after subclassing PointLight. After we settle on it I will submit a CSR and we can move on to the implementation.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Nov 2, 2020

Another API point is how to implement the direction - a Point3D or 3 doubles. We've had this discussion before when talking about transforms, and didn't reach a conclusion.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 6, 2020

Is the old implementation worth looking at, or is is completely different?

No, the Java 3D implementation was done using fixed-function pipeline (not shaders), so not really a good starting point.

Another API point is how to implement the direction - a Point3D or 3 doubles.

Yes, we need to sort this one out for SpotLight. This is a direction vector similar to the rotation axis in Rotate, so using Point3D for this property seems most consistent.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Nov 10, 2020

This is a direction vector similar to the rotation axis in Rotate, so using Point3D for this property seems most consistent.

This is the current implementation. The only downside is that it's more difficult to use bindings with.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think the API looks good with a few comments about valid ranges.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 22, 2020

@nlisker this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8234920_Add_SpotLight_to_the_selection_of_3D_light_types
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict label Dec 22, 2020
@arapte
Copy link
Member

@arapte arapte commented Jun 16, 2021

When I apply that fix it works fine for me on my older MacBook.

With the change, It works fine on my mac machine too.

@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Jun 16, 2021

Thanks for the debugging, Kevin! In the HLSL shader these are already in out variables, so no changes there.

I commented out the less performant methods.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jun 16, 2021

The glsl computeLight methods are still calling the now-commented-out alternative shaders, so it fails at load time. See, for example, main1Light.frag#L129. The rest of the changes look fine.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I did a full test run on 5 different system, including manual tests on 4 of them:

Windows 10 w/ Intel graphics
Linux w/ NVIDIA graphics (no manual testing)
Linux VM guest running on Windows 10 host
Mac w/ discrete graphics
Mac w/ integrated graphics

No problems detected. All looks good.

I think there could be some additional tuning done for point lights, but that could be looked at in a follow-on fix.

I reviewed the CSR and it is ready to be Finalized.

I finished reviewing the shader code, and left a couple comments on the HLSL shaders.

float4x3 mBones[MAX_BONES] : register(c35);

float4 gReserved240[16] : register(c240);
Copy link
Member

@kevinrushforth kevinrushforth Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gReserved240 is now at the wrong location (it should be 245), so if it were ever used it would be a problem. It should be updated to avoid confusion at least.

Copy link
Member

@kevinrushforth kevinrushforth Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the size should be updated to 11 as well (since we probably don't want to go past 256).

Copy link
Collaborator Author

@nlisker nlisker Jun 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need gReserved240 and gSomethingElse (in the pixel shader) at all. If they are at the end, they don't need to be reserved I think.
Also, the vertex shader has gReserved5[5] that reserves c5 to c9, but the pixel shader does not have anything on c2 and c3, which are also reserved for something.

Another thing I don't understand is why the vertex shader overlaps register definitions for gAmbinet and gAmbinetData[10], and mWorld and mBones[MAX_BONES] (and why 70 of these?).

@kevinrushforth kevinrushforth self-requested a review Jun 18, 2021
@openjdk openjdk bot removed the csr label Jun 18, 2021
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Jun 19, 2021

In addition to addressing the review comments, I added some comments on register assignments to help with the math and explanations. No functional changes were made there.

Copy link
Member

@arapte arapte left a comment

Provided two suggestions. You can address if you think they are worth to address now.
Over all the PR looks good to me, I observed an existing issue while testing the attenuation.AttenLightingSample.

Both Point and Spot light are not correctly applied to Mesh when number of faces is increased to more than 60.
Following are steps to reproduce with Point light with existing source without this PR.
As this is an existing issue we can address it in a follow on.

  1. Apply this patch current repo, without this PR
--- a/tests/performance/3DLighting/attenuation/LightingSample.java
+++ b/tests/performance/3DLighting/attenuation/LightingSample.java
@@ -69,8 +69,8 @@ public class LightingSample extends Application {
         var sphere = new Button("Sphere");
         sphere.setOnAction(e -> switchTo(environment.createSphere((int) subdivisionSlider.getValue())));

-        var quadSlider = new Slider(500, 10_000, 1000);
-        quadSlider.setMajorTickUnit(500);
+        var quadSlider = new Slider(10, 200, 60);
+        quadSlider.setMajorTickUnit(10);
         setupSlier(quadSlider);

         var quadLabel = new Label();
  1. Compile and run attenuation.AttenLightingSample
  2. Turn on Magenta light and click Mesh button. Observe that light is proper

image

  1. Increase the number of quads to 70 and observe that light is not proper

image

};

//3 lights used
uniform Light lights[3];

varying vec4 lightTangentSpacePositions[3];
varying vec4 lightTangentSpaceDirections[3];
Copy link
Member

@arapte arapte Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lightTangentSpacePositions is pre existing variable. I think the name lightTangentSpacePositions is misguiding here. We use this variable to hold a point to light vector in tangent space. I think the name should have been something like, tangentSpacePointToLightVec or pointToLightVecInTangentSpace. Again this is not mandatory for this PR, But it did confuse me a bit while reading.

The other variable name lightTangentSpaceDirections sounds correct, as it holds the lights direction in tangent space, but if we change the existing variable then similarly this variable name should be also be changed.

Copy link
Member

@kevinrushforth kevinrushforth Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is preexisting, I probably wouldn't change it as part of this PR, but instead it could be done as part of the future work to support more than 3 lights.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jun 22, 2021

Both Point and Spot light are not correctly applied to Mesh when number of faces is increased to more than 60.
Following are steps to reproduce with Point light with existing source without this PR.
As this is an existing issue we can address it in a follow on.

@arapte can you file a JBS bug for this?

@arapte
Copy link
Member

@arapte arapte commented Jun 22, 2021

@arapte can you file a JBS bug for this?

Reported the issue here: JDK-8269133

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I don't have any more comments or concerns. If you choose to reorder the variables or make other simple changes, I'll re-approve.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

You need a corresponding change in the vertex shader.

float range;
vec3 dir;
Copy link
Member

@kevinrushforth kevinrushforth Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be reordered to match the changes in the fragment shaders. As it is, it causes a shader link error.

Copy link
Collaborator Author

@nlisker nlisker Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I can't test the fix on Linux for now. I made the change, but didn't verify that it works.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Tested on Mac and Linux. Works fine again.

arapte
arapte approved these changes Jun 24, 2021
Copy link
Member

@arapte arapte left a comment

Excellent work.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

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

8234920: Add SpotLight to the selection of 3D light types

Reviewed-by: kcr, arapte

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 19 new commits pushed to the master branch:

  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • 04493e5: 8165214: ListView.EditEvent.getIndex() does not return the correct index
  • 13cffba: 8269026: PasswordField doesn't render bullet character on Android
  • 171e484: 8267551: Support loading images from inline data-URIs
  • 98138c8: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
  • c81a722: 8264139: Suppress removal warnings for Security Manager methods
  • 0ffa8e2: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene
  • e6cf1df: 8267094: TreeCell: cancelEvent must return correct editing location
  • ... and 9 more: https://git.openjdk.java.net/jfx/compare/329013b321d0f95a26aa70c36b0702dc90e2f56f...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 openjdk bot added the ready label Jun 24, 2021
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Jun 24, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

Going to push as commit 3fd4c97.
Since your change was applied there have been 19 commits pushed to the master branch:

  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • 04493e5: 8165214: ListView.EditEvent.getIndex() does not return the correct index
  • 13cffba: 8269026: PasswordField doesn't render bullet character on Android
  • 171e484: 8267551: Support loading images from inline data-URIs
  • 98138c8: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
  • c81a722: 8264139: Suppress removal warnings for Security Manager methods
  • 0ffa8e2: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene
  • e6cf1df: 8267094: TreeCell: cancelEvent must return correct editing location
  • ... and 9 more: https://git.openjdk.java.net/jfx/compare/329013b321d0f95a26aa70c36b0702dc90e2f56f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jun 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2021

@nlisker Pushed as commit 3fd4c97.

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

@nlisker nlisker deleted the 8234920_Add_SpotLight_to_the_selection_of_3D_light_types branch Jun 24, 2021
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Jun 24, 2021

Thanks for the all the work Kevin and Ambarish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
3 participants