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

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

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.

Webrevs

• 15: Full - Incremental (475e1bd)
• 14: Full - Incremental (e1fdf8e)
• 13: Full - Incremental (21f91dd)
• 12: Full - Incremental (af0b179)
• 11: Full - Incremental (d3dd506)
• 10: Full - Incremental (2c9af76)
• 09: Full - Incremental (3ab051d)
• 08: Full - Incremental (e20f8a4)
• 07: Full - Incremental (d77204a)
• 06: Full - Incremental (3106023)
• 05: Full - Incremental (c548106)
• 04: Full (2e1223e)
• 03: Full (66cdab3)
• 02: Full (464e8b5)
• 01: Full (f66e8cf)
• 00: Full (a388fb9)

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.

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)

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.

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.

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.

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.

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.

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.

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

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

/integrate

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