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

8217472: Add attenuation for PointLight #43

Closed

Conversation

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Nov 17, 2019

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

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 17, 2019

👋 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 nlisker changed the title 8217472 add attenuation for point light 8217472: Add attenuation for PointLight Nov 17, 2019
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Nov 18, 2019

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

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

sample.zip

@nlisker nlisker marked this pull request as ready for review Nov 18, 2019
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Nov 18, 2019

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.

@kevinrushforth kevinrushforth self-requested a review Nov 18, 2019
@openjdk openjdk bot added the rfr label Nov 18, 2019
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Nov 20, 2019

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
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 20, 2019

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

@nlisker nlisker commented Jan 2, 2020

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
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 3, 2020

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.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 3, 2020

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

@nlisker nlisker commented Jan 3, 2020

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

@nlisker nlisker commented Jan 9, 2020

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
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 10, 2020

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 nlisker changed the base branch from master to jfx14 Jan 10, 2020
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Sep 4, 2020

Looks like there's more to it. I've created the test code:

import javafx.application.Application;
import javafx.scene.Group;
import javafx.scene.PerspectiveCamera;
import javafx.scene.PointLight;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Slider;
import javafx.scene.image.ImageView;
import javafx.scene.layout.BorderPane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Box;
import javafx.stage.Stage;

public class PointLightAttenuationTest extends Application {

    public static void main(String[] args) throws Exception {
        launch(args);
    }

    @Override
    public void start(Stage stage) {
        var light = new PointLight(Color.BLUE);
        light.setTranslateZ(-10);
        Box box = new Box(100, 100, 1);
        var group = new Group(light, box);

        var scene = new Scene(group, 500, 500, true);
        var camera = new PerspectiveCamera(true);
        camera.setTranslateZ(-300);
        camera.setFarClip(1000);
        scene.setCamera(camera);
        stage.setScene(scene);
        stage.show();

        var imageView = new ImageView();
        var snapshotButton = new Button("Node Snaphot");
        snapshotButton.setOnAction(e -> imageView.setImage(scene.snapshot(null)));

        var slider = new Slider(0, 0.2, 0);
        slider.setShowTickMarks(true);
        slider.setShowTickLabels(true);
        light.linearAttenuationProperty().bindBidirectional(slider.valueProperty());

        var snapshotStage = new Stage();
        snapshotStage.setScene(new Scene(new BorderPane(imageView, snapshotButton, null, slider, null)));
        snapshotStage.setWidth(600);
        snapshotStage.setHeight(600);
        snapshotStage.show();
    }
}

If the line snapshotButton.setOnAction(e -> imageView.setImage(scene.snapshot(null))); is replaced to use box.snapshot(null, null) then the snapshot is wrong.

@openjdk openjdk bot removed the rfr label Sep 16, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 19, 2020

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

@openjdk openjdk bot added the rfr label Sep 20, 2020
@openjdk openjdk bot removed the rfr label Sep 21, 2020
@openjdk openjdk bot added the rfr label Sep 21, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

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.

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

/*void D3DLight::setRange(float r) {

This comment has been minimized.

@kevinrushforth

kevinrushforth Oct 8, 2020
Member

Remove this unused function?

This comment has been minimized.

@nlisker

nlisker Oct 9, 2020
Author Collaborator

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?

This comment has been minimized.

@kevinrushforth

kevinrushforth Oct 9, 2020
Member

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

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.

var sphere = new Button("Sphere");
sphere.setOnAction(e -> switchTo(environment.createSphere((int) subdivisionSlider.getValue())));

var quadSlider = new Slider(500, 10_000, 1000);

This comment has been minimized.

@kevinrushforth

kevinrushforth Oct 9, 2020
Member

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.

public void start(Stage stage) throws Exception {
environment.setStyle("-fx-background-color: teal");

var subdivisionSlider = new Slider(10, 200, 60);

This comment has been minimized.

@kevinrushforth

kevinrushforth Oct 9, 2020
Member

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.

This comment has been minimized.

@nlisker

nlisker Oct 19, 2020
Author Collaborator

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.

@nlisker nlisker requested a review from arapte Oct 10, 2020
@openjdk openjdk bot removed the csr label Oct 13, 2020
@arapte
arapte approved these changes Oct 19, 2020
Copy link
Member

@arapte arapte left a comment

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

@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

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

@openjdk openjdk bot added the ready label Oct 19, 2020
@nlisker
Copy link
Collaborator Author

@nlisker nlisker commented Oct 19, 2020

/integrate

@openjdk openjdk bot closed this Oct 19, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@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 nlisker deleted the nlisker:8217472_Add_attenuation_for_PointLight branch Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants