8217472: Add attenuation for PointLight #43

nlisker · 2019-11-17T04:15:34Z

CSR: https://bugs.openjdk.java.net/browse/JDK-8218264

Progress

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

Issue

JDK-8217472: Add attenuation for PointLight

Reviewers

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

Download

$ git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43
$ git checkout pull/43

…ntLight' into 8217472_Add_attenuation_for_PointLight

bridgekeeper · 2019-11-17T04:16:05Z

👋 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 (refresh this page to view it).

nlisker · 2019-11-18T13:08:12Z

Tested D3D on Win 10 and GL on Ubuntu 18.04.

There is still a bug (at least on Win) for some certain combinations of values as shown in this picture:

This sample program can be used to test the changes.
Controls: mouse wheel zooms, RMB drag rotates, LMB pans.

sample.zip

nlisker · 2019-11-18T13:12:42Z

Kevin, Ambarish,

You can start the review, especially the API. I will hunt that specific values bug this week.

I'll need to know what kind of tests are needed in terms of functionality and performance.

mlbridge · 2019-11-18T18:00:45Z

Webrevs

nlisker · 2019-11-20T00:32:36Z

The bug I mentioned above is not a bug actually. There are large changes over a small distance that make it looks like a jump in the lighting values, but when working with a finer scale the lighting dynamics seem correct.

kevinrushforth · 2019-12-20T21:56:32Z

I think this is on the right track. The API looks like it is in good shape.

This will need a fair bit of testing to ensure that there are no regressions either in functionality or (especially) performance, in addition to tests for the new functionality. On the performance aspect, the inner loop of the lighting calculation now has an additional if test for the max range and additional arithmetic calculations for the attenuation. What we will need is a test program that we can run on Mac and Windows to measure the performance of rendering in a fill-rate-limited case. Ideally, we would not pay much of a performance hit in the default case where ca == 1, la == 0, qa == 0, but we first need to be able to measure the drop in performance before we can say whether it is acceptable.

Speaking of testing, I took the current patch for a test drive on Mac and Windows. I get the following system test failures on Mac, and also the same failure using fx83dfeatures/LightMotion in toys.

Shader compile log: ERROR: 0:308: Use of undeclared identifier 'range'
ERROR: 0:316: Regular non-array variable 'dist' may not be redeclared

test.robot.test3d.MeshCompareTest > testSnapshot3D[3] STANDARD_ERROR
    java.lang.RuntimeException: Error creating fragment shader
    	at javafx.graphics/com.sun.prism.es2.ES2Shader.createFromSource(ES2Shader.java:141)
    	at javafx.graphics/com.sun.prism.es2.ES2PhongShader.getShader(ES2PhongShader.java:177)
        ...
test.robot.test3d.MeshCompareTest > testSnapshot3D[3] FAILED
    java.lang.IllegalArgumentException: Unrecognized image loader: null
        at javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278)
        at javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53)
        at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1340)
        at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1372)
        at javafx.graphics/javafx.scene.Scene.snapshot(Scene.java:1462)
        at test.robot.test3d.MeshCompareTest.lambda$testSnapshot3D$0(MeshCompareTest.java:315)


test.robot.test3d.Snapshot3DTest > testSnapshot3D[3] FAILED
(same failure as above)


test.robot.test3d.Snapshot3DTest > testSnapshot3D[7] FAILED
(same failure as above)

nlisker · 2020-01-02T18:18:21Z

I get the following system test failures on Mac

Shader compile log: ERROR: 0:308: Use of undeclared identifier 'range'
ERROR: 0:316: Regular non-array variable 'dist' may not be redeclared

I don't have a Mac to test on, but on Ubuntu system tests pass (I ran the test command for systemTests). Does the sample app I attached also fail on Mac? They both use the same shaders, so I wonder where the issue could be.
Moreover, the error messages are strange. dist is not redeclared and range is not undeclared in the shader. The error message seems to originate from the native function Java_com_sun_prism_es2_GLContext_nCompileShader in GLContext.c, not managing to compile the shader, but I can't tell why.

kevinrushforth · 2020-01-03T00:48:57Z

I get the same error on Ubuntu 16.04 as on Mac. Did you run the system tests with -PFULL_TEST=true -PUSE_ROBOT=true? Also, you can try running the fx83dfeatures/LightMotion toy and you should see the same error.

I still need to test your sample app on Mac.

arapte

I have added few comments, but have not run tests and sample yet.

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

modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DContext.java

modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java

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

modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java

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

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

kevinrushforth · 2020-01-03T22:37:06Z

I still need to test your sample app on Mac.

I get the error with your sample app. It fails on Mac or Linux (Ubuntu 16.04) with the same error as I reported above.

nlisker · 2020-01-03T23:02:56Z

The error was for the cases of 2 and 3 lights (I was testing 1) and should be fixed now. My fault with copy-paste... that's why we use loops, but I guess this is some optimization for the es2 pipeline. I wonder if it's really worth it over a single shader looping over the number of lights like d3d does.

nlisker · 2020-01-09T18:00:19Z

This will need a fair bit of testing to ensure that there are no regressions either in functionality or (especially) performance, in addition to tests for the new functionality.

Which tests for the new functionality should I write? Up to the shader itself it's mostly just passing on variables, and the API is standard DoublePropertys. I can test the dirty bits / redraw logic.

On the performance aspect, the inner loop of the lighting calculation now has an additional if test for the max range and additional arithmetic calculations for the attenuation. What we will need is a test program that we can run on Mac and Windows to measure the performance of rendering in a fill-rate-limited case. Ideally, we would not pay much of a performance hit in the default case where ca == 1, la == 0, qa == 0, but we first need to be able to measure the drop in performance before we can say whether it is acceptable.

Can you point me to a similar performance test? I didn't see a way to easily measure FPS.
I don't know how to avoid the if test for the maxRange, I'm open to suggestions. The only thing I can think of is if maxRange is infinite (the default), we will swap the shader for one that doesn't make that check. However, swapping shaders also costs performance.

kevinrushforth · 2020-01-10T00:34:18Z

We have a few performance tests in apps/performance, but I don't know how up to date they are. They might give you a starting point on how to measure FPS, but really the harder part is going to be coming up with a workload -- a scene with a number of Shape3Ds with large triangles (so that they are fill-rate limited) and 1-3 lights, etc -- that you can use to measure rendering performance without measuring overhead. Typically you want a scene that is rendering continuously in the < 30 fps range, and more like 10 fps to minimize the overhead even more.

Before we figure out whether we need to double the number of shaders (which was one of the ideas that I had as well), we need to know how much of a problem it is. Is it < 2% performance drop on typical cases on a variety of machines or it is a 25% drop (or more likely somewhere in between). If the perf drop is negligible, then it isn't worth doubling the shaders. If it is significant, then we probably need to.

If we do need to double the shaders, I wouldn't do it based on the maxRange being infinite, rather I would do it based on whether attenuation is being used. That way the existing shaders would be unchanged, while the new shader would deal with both attenuation and the maxRange test.

Hopefully, there won't be enough of a perf hit to require doubling the shaders, but we need to be able to measure it.

For functional testing, in addition to the simple API tests, we want to make sure that the basic rendering is working with and without attenuation. Some sort of visual test where you verify that attenuation is / isn't happening as well as testing the cutoff. I wouldn't get too fancy with these...just a sanity test.

kevinrushforth · 2020-09-19T15:32:58Z

@nlisker Since there is a possible bug in snapshot, you might consider using Robot screen capture instead?

Also, I don't know if you noticed, but there are now whitespace errors after the fix for JDK-8240499.

kevinrushforth

The public API and implementation looks good. I left a few inline comments.

I'll review the CSR in parallel with your fixing the few things I noted.

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

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

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

kevinrushforth · 2020-10-08T22:21:06Z

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

@@ -56,3 +60,7 @@ void D3DLight::setPosition(float x, float y, float z) {
    position[1] = y;
    position[2] = z;
 }
+
+/*void D3DLight::setRange(float r) {


Remove this unused function?

I forgot to mention this point. These setter functions for color and position (and range) are never used since whenever there is a change in the java side the whole array and lights are recreated instead of being adjusted for the change. I'm not familiar enough with the memory model of JNI, but it seems expensive to re-render everything on every change (I think that the mesh is also recreated every time in the native code). Is this the way it's supposed to work?

If the lights array is being recreated on a change, there might be a small savings to be had to reuse the existing array (it would need to be updated only during the sync while the renderer is idle).

If the mesh is always being recreated, there may be an opportunity for an even bigger savings, presuming that only the mesh data changed (and not the index values in the faces array).

This could be a future optimization, but we would need something to quantify the possible savings.

modules/javafx.graphics/src/main/native-prism-es2/GLContext.c

tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java

kevinrushforth

I think this is ready to go in once it gets a second reviewer and once the CSR is approved.

I left two very minor comments on the test. I don't mind whether you fix them now or as a follow-up.

kevinrushforth · 2020-10-09T18:09:40Z

tests/performance/3DLighting/attenuation/LightingSample.java

+        var sphere = new Button("Sphere");
+        sphere.setOnAction(e -> switchTo(environment.createSphere((int) subdivisionSlider.getValue())));
+
+        var quadSlider = new Slider(500, 10_000, 1000);


I was doing some final testing, and ran this on a few different systems. I think the min and max values are too large (especially the min value). I recommend something more like min=100 and max=5000.

kevinrushforth · 2020-10-09T18:11:14Z

tests/performance/3DLighting/attenuation/LightingSample.java

+    public void start(Stage stage) throws Exception {
+        environment.setStyle("-fx-background-color: teal");
+
+        var subdivisionSlider = new Slider(10, 200, 60);


I might recommend setting the max value of the slider to something like 1000, since even on a slow system, it runs quite nicely at that tessellation.

I forgot to reply to this and the other comment. Since we're going to implement more light types, this small test app is going to be updated anyway, so I will change the values then.

arapte

Looks good to me. There seems to be an existing issue with PointLight. It is reported here JDK-8255015

openjdk · 2020-10-19T17:55:49Z

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

8217472: Add attenuation for PointLight

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

31fce21: 8223375: Remove Netbeans specific files from the source code repository
e856a58: 8246202: ChoiceBoxSkin: misbehavior on switching skin, part 2
eb626aa: 8254040: [testbug] Need additional regressions tests for ObservableList removeAll / retainAll
eaabfc5: 8253935: [testbug] ComboBoxTest.testEditorKeyInputsWhenPopupIsShowing fails on Mac, Linux
00f5b7c: 8253597: TreeTableView: must select leaf row on click into indentation region
205e4b9: 8178297: TableView scrolls slightly when adding new elements
a56ba63: 8254255: Remove obsolete .hgignore file
184f12c: 8252192: Update to Visual Studio 2019 version 16.7.2
7857022: 8251946: ObservableList.setAll does not conform to specification
1c485a3: 8252191: Update to gcc 10.2 on Linux
... and 115 more: https://git.openjdk.java.net/jfx/compare/4ec163df90c88cc01b70d58831ad2abdb86c196d...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 · 2020-10-19T17:58:09Z

/integrate

openjdk · 2020-10-19T18:03:43Z

@nlisker Since your change was applied there have been 125 commits pushed to the master branch:

31fce21: 8223375: Remove Netbeans specific files from the source code repository
e856a58: 8246202: ChoiceBoxSkin: misbehavior on switching skin, part 2
eb626aa: 8254040: [testbug] Need additional regressions tests for ObservableList removeAll / retainAll
eaabfc5: 8253935: [testbug] ComboBoxTest.testEditorKeyInputsWhenPopupIsShowing fails on Mac, Linux
00f5b7c: 8253597: TreeTableView: must select leaf row on click into indentation region
205e4b9: 8178297: TableView scrolls slightly when adding new elements
a56ba63: 8254255: Remove obsolete .hgignore file
184f12c: 8252192: Update to Visual Studio 2019 version 16.7.2
7857022: 8251946: ObservableList.setAll does not conform to specification
1c485a3: 8252191: Update to gcc 10.2 on Linux
... and 115 more: https://git.openjdk.java.net/jfx/compare/4ec163df90c88cc01b70d58831ad2abdb86c196d...master

Your commit was automatically rebased without conflicts.

Pushed as commit aab26d9.

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

nlisker and others added 6 commits November 8, 2019 23:55

Initial push of 8217472

a590f09

Separate range from attenuation

52e1ae1

GL pipeline

d39bcee

Merge remote-tracking branch 'nlisker/8217472_Add_attenuation_for_Poi…

4b63a01

…ntLight' into 8217472_Add_attenuation_for_PointLight

Docs and year update

3450ffe

Correction for indexes

415927e

nlisker changed the title ~~8217472 add attenuation for point light~~ 8217472: Add attenuation for PointLight Nov 17, 2019

Fixed whitespaces

a388fb9

nlisker marked this pull request as ready for review November 18, 2019 13:08

kevinrushforth assigned nlisker Nov 18, 2019

kevinrushforth self-requested a review November 18, 2019 18:00

openjdk bot added the rfr Ready for review label Nov 18, 2019

nlisker mentioned this pull request Dec 6, 2019

WIP: JDK-8217477: Implement attenuation-coefficients for PointLight javafxports/openjdk-jfx#384

Draft

arapte suggested changes Jan 3, 2020

View reviewed changes

Addressing review comments

f66e8cf

Fixed shader compilation errors for 2 and 3 lights in es2

464e8b5

Attenuation and range changed internally to floats from doubles

66cdab3

nlisker changed the base branch from master to jfx14 January 10, 2020 16:57

openjdk bot removed the rfr Ready for review label Sep 16, 2020

Fixed whitespace

d3dd506

openjdk bot added the rfr Ready for review label Sep 20, 2020

Added an automated test

89539e7

openjdk bot removed the rfr Ready for review label Sep 21, 2020

Fixed whitespaces

af0b179

openjdk bot added the rfr Ready for review label Sep 21, 2020

Change range after clamping

21f91dd

kevinrushforth reviewed Oct 8, 2020

View reviewed changes

tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java Show resolved Hide resolved

Addressed review comments

e1fdf8e

kevinrushforth reviewed Oct 9, 2020

View reviewed changes

tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java Show resolved Hide resolved

Fixed test

475e1bd

kevinrushforth approved these changes Oct 9, 2020

View reviewed changes

nlisker requested a review from arapte October 10, 2020 02:18

openjdk bot removed the csr Need approved CSR to integrate pull request label Oct 13, 2020

arapte approved these changes Oct 19, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 19, 2020

openjdk bot closed this Oct 19, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Oct 19, 2020

nlisker deleted the 8217472_Add_attenuation_for_PointLight branch October 20, 2020 13:14

nlisker mentioned this pull request Nov 2, 2020

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

Closed

7 tasks

nlisker mentioned this pull request Sep 8, 2023

8315842: 3D tests fail because of edge pixel differences openjdk/jfx-tests#5

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8217472: Add attenuation for PointLight #43

8217472: Add attenuation for PointLight #43

nlisker commented Nov 17, 2019 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 17, 2019

nlisker commented Nov 18, 2019

nlisker commented Nov 18, 2019

mlbridge bot commented Nov 18, 2019 •

edited

Loading

nlisker commented Nov 20, 2019 •

edited

Loading

kevinrushforth commented Dec 20, 2019

nlisker commented Jan 2, 2020

kevinrushforth commented Jan 3, 2020

arapte left a comment

kevinrushforth commented Jan 3, 2020

nlisker commented Jan 3, 2020 •

edited

Loading

nlisker commented Jan 9, 2020

kevinrushforth commented Jan 10, 2020

kevinrushforth commented Sep 19, 2020 •

edited

Loading

kevinrushforth left a comment

kevinrushforth Oct 8, 2020

nlisker Oct 9, 2020

kevinrushforth Oct 9, 2020

kevinrushforth left a comment

kevinrushforth Oct 9, 2020

kevinrushforth Oct 9, 2020

nlisker Oct 19, 2020

arapte left a comment

openjdk bot commented Oct 19, 2020 •

edited

Loading

nlisker commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

8217472: Add attenuation for PointLight #43

8217472: Add attenuation for PointLight #43

Conversation

nlisker commented Nov 17, 2019 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Nov 17, 2019

nlisker commented Nov 18, 2019

nlisker commented Nov 18, 2019

mlbridge bot commented Nov 18, 2019 • edited Loading

Webrevs

nlisker commented Nov 20, 2019 • edited Loading

kevinrushforth commented Dec 20, 2019

nlisker commented Jan 2, 2020

kevinrushforth commented Jan 3, 2020

arapte left a comment

Choose a reason for hiding this comment

kevinrushforth commented Jan 3, 2020

nlisker commented Jan 3, 2020 • edited Loading

nlisker commented Jan 9, 2020

kevinrushforth commented Jan 10, 2020

kevinrushforth commented Sep 19, 2020 • edited Loading

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Oct 8, 2020

Choose a reason for hiding this comment

nlisker Oct 9, 2020

Choose a reason for hiding this comment

kevinrushforth Oct 9, 2020

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Oct 9, 2020

Choose a reason for hiding this comment

kevinrushforth Oct 9, 2020

Choose a reason for hiding this comment

nlisker Oct 19, 2020

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 19, 2020 • edited Loading

nlisker commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

nlisker commented Nov 17, 2019 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 18, 2019 •

edited

Loading

nlisker commented Nov 20, 2019 •

edited

Loading

nlisker commented Jan 3, 2020 •

edited

Loading

kevinrushforth commented Sep 19, 2020 •

edited

Loading

openjdk bot commented Oct 19, 2020 •

edited

Loading