8260013: Snapshot does not work for nodes in a subscene #1332

lukostyra · 2024-01-12T14:11:14Z

Originally this issue showed the problem of Node being incorrectly rendered (clipped) when snapshotting, compared to a snapshot of the whole Scene. Later on there was another problem added - lights not being taken into account if they are added to a SubScene.

As it later turned out, the original problem from this bug report is a problem with ParallelCamera incorrectly estimating near/far clipping planes, which just happened to reveal itself while snapshotting a Node. During testing I found out you can make the Node clip regardless of snapshot mechanism. Clipping issue was moved to a separate bug report and this PR only fixes the inconsistency in lights being gathered for a snapshot.

Scene.doSnapshot() was expanded to also check if SubScene provided to it is non-null and to fetch lights assigned to it. Scenario was tested with added SnapshotLightsTest.

Rest of the tests were checked and don't produce any noticeable regressions.

Progress

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

Issue

JDK-8260013: Snapshot does not work for nodes in a subscene (Bug - P3)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Michael Strauß (@mstr2 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1332/head:pull/1332
$ git checkout pull/1332

Update a local copy of the PR:
$ git checkout pull/1332
$ git pull https://git.openjdk.org/jfx.git pull/1332/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1332

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1332.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-12T14:11:48Z

👋 Welcome back lkostyra! 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.

mlbridge · 2024-01-12T14:15:54Z

Webrevs

kevinrushforth · 2024-01-15T16:39:41Z

/reviewers 2

openjdk · 2024-01-15T16:39:53Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mstr2 · 2024-01-15T21:25:57Z

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

-            context.lights = new NGLightBase[scene.lights.size()];
-            for (int i = 0; i < scene.lights.size(); i++) {
-                context.lights[i] = scene.lights.get(i).getPeer();
+        int totalLightCount = 0;


Here's a suggestion: you might be able to replace all of the new code, including the accumulateLightsForSnapshot method, with the following shorter code:

Stream<NGLightBase> lights = Stream.concat( Optional.ofNullable(scene).stream().flatMap(s -> s.lights.stream()).map(LightBase::getPeer), Optional.ofNullable(subScene).stream().flatMap(s -> s.getLights().stream()).map(LightBase::getPeer)); context.lights = lights.toArray(NGLightBase[]::new);

I changed the code as suggested.

After that Snapshot3DTest code started showing some failures, this was due to NGShape3D code not checking if it gets an empty (length == 0) array and throwing IndexOutOfBoundsException. I patched that as well and didn't see any regressions in tests afterwards.

Uses suggested simpler implementation using Java's Streams. Needed an additional check in NGShape3D, otherwise IndexOutOfBounds was thrown

kevinrushforth

This looks mostly correct except for the case where both the Scene and the SubScene have lights. It seems worth adding a test for this case as well.

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

Snapshot now includes lights only from subScene if it is not null, which matches how SubScenes are rendered

lukostyra · 2024-01-26T13:53:22Z

Corrected the behavior and added a test as well

kevinrushforth

Looks good. I left a few minor cleanup comments. I'll reapprove when/if you fix them.

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

tests/system/src/test/java/test/javafx/scene/SnapshotLightsTest.java

lukostyra · 2024-01-29T09:31:40Z

Cleanup done, should be all OK now

mstr2

Looks good!

openjdk · 2024-01-30T05:46:56Z

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

8260013: Snapshot does not work for nodes in a subscene

Reviewed-by: kcr, mstrauss

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

876d535: 8324239: JFXPanelHiDPITest fails on Windows 11
767b027: 8324797: Code example in JavaDoc of ObservableValue#when doesn't compile
359ffb9: 8308955: MediaPlayer/AudioClip skip data on seek/loop
9e7e8e1: 8324270: Update boot JDK to 21.0.2
1170658: 8322784: JFXPanel calls InputMethodRequests on wrong thread
ff4f8e3: 8324237: Typo in comment in GlassApplication.m
c1c52e5: 8322215: [win] OS events that close the stage can cause Glass to reference freed memory
3cf9390: 8323879: constructor Path(Path) which takes another Path object fail to draw on canvas html
872dbc8: 8323543: NPE when table items are set to null
e2bc52d: Merge
... and 13 more: https://git.openjdk.org/jfx/compare/caba6788e2b62b3c1af102d1ff311ac2eaeab8c5...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.

lukostyra · 2024-01-30T06:57:31Z

/integrate

openjdk · 2024-01-30T06:58:18Z

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

876d535: 8324239: JFXPanelHiDPITest fails on Windows 11
767b027: 8324797: Code example in JavaDoc of ObservableValue#when doesn't compile
359ffb9: 8308955: MediaPlayer/AudioClip skip data on seek/loop
9e7e8e1: 8324270: Update boot JDK to 21.0.2
1170658: 8322784: JFXPanel calls InputMethodRequests on wrong thread
ff4f8e3: 8324237: Typo in comment in GlassApplication.m
c1c52e5: 8322215: [win] OS events that close the stage can cause Glass to reference freed memory
3cf9390: 8323879: constructor Path(Path) which takes another Path object fail to draw on canvas html
872dbc8: 8323543: NPE when table items are set to null
e2bc52d: Merge
... and 13 more: https://git.openjdk.org/jfx/compare/caba6788e2b62b3c1af102d1ff311ac2eaeab8c5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-30T06:58:25Z

@lukostyra Pushed as commit d653a31.

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

lukostyra added 3 commits January 12, 2024 14:48

Include subscene lights in snapshot

736d0bf

Add test for snapshot lights inconsistency

d386d19

Move test out of robot tests, update copyrights

2a2da5e

openjdk bot added the rfr Ready for review label Jan 12, 2024

kevinrushforth requested review from arapte and kevinrushforth January 15, 2024 16:39

mstr2 reviewed Jan 15, 2024

View reviewed changes

doSnapshot: Replace light accumulation code

49782ee

Uses suggested simpler implementation using Java's Streams. Needed an additional check in NGShape3D, otherwise IndexOutOfBounds was thrown

kevinrushforth reviewed Jan 24, 2024

View reviewed changes

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java Outdated Show resolved Hide resolved

lukostyra added 2 commits January 26, 2024 13:50

SnapshotLightsTest: Add test for a snapshot with an extra light in scene

b6d2f32

doSnapshot: Adjust light collection

5259be8

Snapshot now includes lights only from subScene if it is not null, which matches how SubScenes are rendered

kevinrushforth approved these changes Jan 26, 2024

View reviewed changes

Minor review adjustments

f05e441

kevinrushforth approved these changes Jan 29, 2024

View reviewed changes

mstr2 approved these changes Jan 30, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 30, 2024

openjdk bot added the integrated Pull request has been integrated label Jan 30, 2024

openjdk bot closed this Jan 30, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jan 30, 2024

lukostyra deleted the snapshot_nodes-8260013 branch February 8, 2024 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8260013: Snapshot does not work for nodes in a subscene #1332

8260013: Snapshot does not work for nodes in a subscene #1332

lukostyra commented Jan 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 12, 2024

mlbridge bot commented Jan 12, 2024 •

edited

Loading

kevinrushforth commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

mstr2 Jan 15, 2024

lukostyra Jan 18, 2024 •

edited

Loading

kevinrushforth left a comment

lukostyra commented Jan 26, 2024

kevinrushforth left a comment

lukostyra commented Jan 29, 2024

mstr2 left a comment

openjdk bot commented Jan 30, 2024

lukostyra commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

8260013: Snapshot does not work for nodes in a subscene #1332

8260013: Snapshot does not work for nodes in a subscene #1332

Conversation

lukostyra commented Jan 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 12, 2024

mlbridge bot commented Jan 12, 2024 • edited Loading

Webrevs

kevinrushforth commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

mstr2 Jan 15, 2024

Choose a reason for hiding this comment

lukostyra Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

lukostyra commented Jan 26, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

lukostyra commented Jan 29, 2024

mstr2 left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 30, 2024

lukostyra commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

openjdk bot commented Jan 30, 2024

lukostyra commented Jan 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 12, 2024 •

edited

Loading

lukostyra Jan 18, 2024 •

edited

Loading