8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty

[eduardsdv]

This is an alternative solution to the PR: https://github.com/openjdk/jfx/pull/1310.

This solution is based on the invariant that if a node is marked as dirty, all ancestors must also be marked as dirty and that if an ancestor is marked as clean, all descendants must also be marked as clean. Therefore I removed the clearDirtyTree() method and put its content to the clearDirty() method.

Furthermore, since dirty flag is only used when rendering by ViewPainter, it should also be deleted by ViewPainter only. This guarantees:

1. that all dirty flags are removed after rendering, and
2. that no dirty flags are removed when a node is rendered, e.g. by creating a snapshot or printing. Therefore I removed all calls of the methods clearDirty() and clearDirtyTree() from all other classes except the ViewerPainter.

The new version of the clearDirty() method together with calling it from the ViewerPainter needs to visit far fewer nodes compared to the version prior this PR.

The supplied test checks that the nodes are updated even if they are partially covered, which led to the error in the version before the PR. The test can be started with gradlew -PFULL_TEST=true :systemTests:test --tests NGNodeDirtyFlagTest

---

Progress

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

Issues

• JDK-8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty (Bug - P4)
• JDK-8332040: Parts of SG no longer update during rendering - overlapping - culling - dirty (CSR) (Withdrawn)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1451

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back eduardsdv! 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.

[openjdk[bot]]

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

8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty

Reviewed-by: arapte, lkostyra

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

• 56e07dbc9b04af06fc8cd1970595d19cb77fea4a: 8335216: [windows] Missing error check for GetSystemDirectory in glass
• 72701e6cb4095b8f5042f54ae6bb2c0cec446bcf: 8335633: Missing @Override in Dimension
• 459b15fc61e7a78f29c16fe665370657c1236b3f: 8326712: Robot tests fail on XWayland
• 5656b80f8a584a2d4d791f9fab83f1719b29f986: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
• 6c3fb5f557597a5a226d4a7bd41d84b7feb4a162: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
• 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte, @kevinrushforth, @lukostyra) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge[bot]]

Webrevs

• 05: Full - Incremental (a7646e8e)
• 04: Full - Incremental (229927cc)
• 03: Full - Incremental (ac971e14)
• 02: Full - Incremental (e3bd58f3)
• 01: Full - Incremental (a2383fd9)
• 00: Full (8aed95aa)

[kevinrushforth]

Reviewers: @lukostyra @arapte

This is a more intrusive fix than #1310 so we would only want to go this route if we can show that it is a correct fix, introduces no regressions (in either correctness or performance), and makes it easier to maintain in the future.

/reviewers 2

[openjdk[bot]]

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

[kevinrushforth]

/csr unneeded

[openjdk[bot]]

@kevinrushforth The CSR requirement cannot be removed as CSR issues already exist. Please withdraw JDK-8332040 and then use the command /csr unneeded again.

[kevinrushforth]

/csr unneeded

[openjdk[bot]]

@kevinrushforth determined that a CSR request is not needed for this pull request.

[hjohn]

Even though this fix is more intrusive, it does seem to address the root cause. Instead of cleaning dirty bits in many places where it is easy to miss one, it always occurs only exactly where needed (after painting completes) using a method that can't accidentally miss parts.

At a minimum I think the tests that are part of this PR should be included in FX whichever fix ends up being integrated.

A quick glance doesn't turn up anything unexpected. I'm only wondering if the code in paintImpl should always clear the dirty bits even if an exception occurs during painting, to harden it against potential bugs and not end up trying to repaint again and again likely getting the same exception again and again.

[eduardsdv]

At a minimum I think the tests that are part of this PR should be included in FX whichever fix ends up being integrated.

The test has already been added to both PRs.

I'm only wondering if the code in paintImpl should always clear the dirty bits even if an exception occurs during painting, to harden it against potential bugs and not end up trying to repaint again and again likely getting the same exception again and again.

Hm, that can indeed happen. On the other hand, if the dirty flag is reset even in the case of an exception, parts of the UI may not be updated for a long time until a node in that area receives a change. The question is which of these two options is the least harmful.

I'm fine with each of them. What do the others think?

[kevinrushforth]

I'm only wondering if the code in paintImpl should always clear the dirty bits even if an exception occurs during painting, to harden it against potential bugs and not end up trying to repaint again and again likely getting the same exception again and again.

Hm, that can indeed happen. On the other hand, if the dirty flag is reset even in the case of an exception, parts of the UI may not be updated for a long time until a node in that area receives a change. The question is which of these two options is the least harmful.

I'm fine with each of them. What do the others think?

I'd probably lean towards addressing this in a follow-up issue, if there turns out to be a need.

@arapte can you look at this when you review it?

[bridgekeeper[bot]]

@eduardsdv This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[eduardsdv]

keep alive

[eduardsdv]

@arapte Please review.

[eduardsdv]

@kevinrushforth The build fails on Linux. Can you please take a look?

[eduardsdv]

Ok. I solved the build by merging from the master branch.

[arapte]

I have tested the patch with several samples. Fix looks good. Though the test fails on my Mac machine with below error. Please verify..

NGNodeDirtyFlagTest > testNGNodesNotDirty FAILED
    org.junit.ComparisonFailure: A node was not rendered properly. Wrong color found expected:<[LIGHTGREEN]> but was:<[0x81ee7eff]>
        at org.junit.Assert.assertEquals(Assert.java:117)
        at test.com.sun.prism.impl.NGNodeDirtyFlagTest.checkColor(NGNodeDirtyFlagTest.java:135)
        at test.com.sun.prism.impl.NGNodeDirtyFlagTest.lambda$checkLineColor$5(NGNodeDirtyFlagTest.java:126)

1 test completed, 1 failed

> Task :systemTests:test FAILED

[eduardsdv]

@arapte: The failed test on the Mac is probably caused by the lack of synchronization between the JavaFX and QuantumRenderer threads. I added the waitForRenderer() method to wait for the stage to render before checking the colors..

[arapte]

The newer version still fails on my Mac machine.

Attaching modified test that executes as expected on my Windows and Mac machines. The main change that makes it work on Mac is the change in checkColor() method. There are some additional minor changes, and I recommend to keep those. Please do test on your machines.

NGNodeDirtyFlagTest.java.zip

[eduardsdv]

Yes, the test with your suggestion also works under Windows. Thank you, @arapte.

[arapte]

Yes, the test with your suggestion also works under Windows. Thank you, @arapte.

Thanks for verifying. In that case, let's remove the waitForRenderer() method that was added to solve the test failure on Mac but is not required anymore.

[eduardsdv]

Yes, the test with your suggestion also works under Windows. Thank you, @arapte.

Thanks for verifying. In that case, let's remove the waitForRenderer() method that was added to solve the test failure on Mac but is not required anymore.

I removed the method.

[kevinrushforth]

@lukostyra Can you be the second reviewer on this?

[eduardsdv]

/integrate

[openjdk[bot]]

@eduardsdv Your change (at version a7646e8ed072e51e1fd912ffa1f7087024f3557a) is now ready to be sponsored by a Committer.

[lukostyra]

/sponsor

[openjdk[bot]]

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

• 56e07dbc9b04af06fc8cd1970595d19cb77fea4a: 8335216: [windows] Missing error check for GetSystemDirectory in glass
• 72701e6cb4095b8f5042f54ae6bb2c0cec446bcf: 8335633: Missing @Override in Dimension
• 459b15fc61e7a78f29c16fe665370657c1236b3f: 8326712: Robot tests fail on XWayland
• 5656b80f8a584a2d4d791f9fab83f1719b29f986: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
• 6c3fb5f557597a5a226d4a7bd41d84b7feb4a162: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
• 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@lukostyra @eduardsdv Pushed as commit 9723a9c4ce3a4490df962b83da0df71b1e5145f1.

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