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

FlorianKirmaier · 2023-12-21T12:33:52Z

In some situations, a part of the SG is no longer rendered.
I created a test program that showcases this problem.

Explanation:

This can happen, when a part of the SG, is covered by another Node.
In this part, one node is totally covered, and the other node is visible.

When the totally covered Node is changed, then it is marked dirty and it's parent, recursively until an already dirty node is found.
Due to the Culling, this totally covered Node is not rendered - with the effect that the tree is never marked as Clean.

In this state, a Node is Dirty but not It's parent. Based on my CodeReview, this is an invalid state which should never happen.

In this invalid state, when the other Node is changed, which is visible, then the dirty state is no longer propagated upwards - because the recursive "NGNode.markTreeDirty" algorithm encounters a dirty node early.

This has the effect, that any SG changes in the visible Node are no longer rendered. Sometimes the situation repairs itself.

Useful parameters for further investigations:
-Djavafx.pulseLogger=true
-Dprism.printrendergraph=true
-Djavafx.pulseLogger.threshold=0

PR:
This PR ensures the dirty flag is set to false of the tree when the culling is used.
It doesn't seem to break any existing tests - but I'm not sure whether this is the right way to fix it.
It would be great to have some feedback on this solution - maybe guiding me to a better solution.

I could write a test, that just does the same thing as the test application, but checks every frame that these nodes are not dirty - but maybe there is a better way to test this.

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)

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/1310/head:pull/1310
$ git checkout pull/1310

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1310

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-21T12:34:46Z

👋 Welcome back fkirmaier! 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 · 2023-12-21T12:38:56Z

Webrevs

…tynodes

FlorianKirmaier · 2023-12-21T12:53:52Z

Fixed wrong ticket-number in title and commit

openjdk · 2023-12-21T12:54:24Z

@FlorianKirmaier Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

kevinrushforth · 2023-12-21T14:46:50Z

/reviewers 2

openjdk · 2023-12-21T14:47:54Z

@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

I'd like to see an automated (probably robot-based) test, if it is feasible.

This will need careful review.

Reviewers: @arapte @lukostyra

kevinrushforth · 2023-12-21T14:47:34Z

.idea/jpa-buddy.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


Please revert the addition of this file.

Removed the accidental change!

kevinrushforth · 2023-12-21T14:49:45Z

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

@@ -2003,6 +2003,7 @@ protected void doRender(Graphics g) {
                if ((bits & DIRTY_REGION_CONTAINS_OR_INTERSECTS_NODE_BOUNDS) == 0) {
                    // If no culling bits are set for this region, this group
                    // does not intersect (nor is covered by) the region
+                    clearDirtyTree();


We'll need to look at this closely to ensure that there are no functional regressions, and that the performance impact is minimal.

FlorianKirmaier · 2023-12-21T15:46:20Z

If someone who understands the rendering could provide some feedback on what is happening - than I can probably make a better fix, and write a reasonable unit-test for this bug.

removed some text with edit, because after thinking about it, it was wrong

hjohn · 2023-12-22T08:34:00Z

The fully covered node which was dirty is now marked clean in the PR when it is culled. Now that it is clean, what happens when it is uncovered (being careful not to change it in any other way)? Does it get rendered correctly, or does it miss the last change made to it while it was fully covered?

If that works correctly still, then I think this is the right fix. Culling the node is (IMHO) the same as rendering it (there is nothing to render) so it should be clean after being culled.

FlorianKirmaier · 2023-12-22T12:54:04Z

It is worth noting that the idea of cleaning the dirty tree, for the culled node, is also done at line NGNode:1414, but for some reason, it doesn't catch the case from this bug.

FlorianKirmaier · 2024-01-16T11:07:03Z

I tried out a bit, by removing the overlapping area in my test program.
I added the changed program to the ticket.
Didn't notice anything wrong.
There are probably many ways to test for regressions.

Can't guarantee that there are no cases that go wrong - but if something doesn't work, then it probably happens more rarely than the original bug.

bridgekeeper · 2024-02-20T12:17:28Z

@FlorianKirmaier 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!

FlorianKirmaier · 2024-03-13T09:13:20Z

push
It's still a important bug.
If someone can guide me on how to write tests for this, i would put in the effort.
Maybe we could make snapshot based tests? That should work on all platforms, right?
(Like testing whether a specific RGB value appears)
Has something in this direction been done somewhere?

lukostyra · 2024-03-13T09:27:40Z

Maybe we could make snapshot based tests? That should work on all platforms, right?

We have a whole suite of tests which use similar methods to perform checks, called robot tests. Since you want to check if a node was properly updated, and the best way to do that seems to be checking RGB values, I think a test for this case belongs right there.

You could use robot utility to check color values of specific pixels on the screen. I think the best way to go about it would be to do something similar to what your test bug apps do (the ones attached to the JBS issue). Ideally I would try and look for first some form of confidence check (if correct controls are "on" and "off"), then progressing the animation and seeing if controls updated as intended.

All robot tests are located in tests/system/src/test/java/test/robot and then they follow package names. I'm not 100% sure where this test should go though (I assume .../javafx/scene?).

openjdk · 2024-03-13T20:00:46Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

bridgekeeper · 2024-04-10T21:19:35Z

@FlorianKirmaier 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 · 2024-04-26T16:03:31Z

I analysed the bug and can confirm the fix.
I summarized it in the following table.
First column is the node structure, the others the steps: update nodes, render, update nodes, render, ...

Nodes (L - left and R - right) / Steps	1st -> update Circles	2nd -> render	3rd -> update Lines	4th -> render
HBox	dirty	clearDirty	dirty	clearDirty
HBox >StackPane-L	dirty	clearDirty	dirty	clearDirty
HBox >StackPane-L > Group-L	dirty	clearDirty	dirty	clearDirty
HBox >StackPane-L > Group-L >Line-L			dirty	clearDirty
HBox >StackPane-L > Group-L >Circle-L	dirty	clearDirty
HBox >StackPane-R	dirty	clearDirty *1		*3
HBox >StackPane-R > Group-R	dirty	dirty	dirty
HBox >StackPane-R > Group-R > Line-R			dirty *2	dirty
HBox >StackPane-R > Group-R > Circle-R	dirty	dirty	dirty	dirty

*1: > no culling bits for first dirty region (Circle-L) -> skip children -> clearDirty() is not called on children
> no root path for the second dirty region (Circle-R) -> no rendering and root.clearDirtyTree() stops on StackPane-R (the children stay dirty: Group-R and Circle-R)

*2: > Line-R.markTreeDirty() skips marking of further parents because Group-R is already dirty -> the StackPane-R stays clean

*3: > the dirty regions are not collected because the StackPane-R is clean -> Line-R is not rendered and the dirty-flag is not reset

The error is that clearDirty() is not called in the second step on the children of StackPane-R (see *1). Subsequent changes in Line-R and Circle-R are not propagated to the root node because Group-R is already dirty (see *2) and therefore StackPane-R remains clean. The last changes are therefore ignored in the next rendering phase (see *3).

We can simply say: the dirty flags must be deleted on all nodes after rendering.

If this is true, the test can simply check this instead of screenshots or similar.

…y are skipped due to visible==false or opacity==0

eduardsdv · 2024-04-29T16:19:12Z

After further investigation and testing I would suggest to remove all clearDirty() and clearDirtyTree() calls and put only one clear-dirty pass after rendering is done (at the end of ViewerPainter.paintImpl(final Graphics backBufferGraphics)).

Since the markDirty() method does both, sets the dirty flag and propagates it to the ancestor nodes, it is better if there is only one method for deleting the dirty flag from the node and all its children. Therefore, I would remove the clearDirtyTree() method and move its content to the clearDirty() method.

This way we can guarantee that all dirty flags are removed after the rendering is completed.
We also guarantee that a clean parent with dirty children will never occur (the bug this PR addresses).

Furthermore, my quick test shows that fewer clearDirty() calls are required in the new version. Bonus: We are even faster.

    public void clearDirty() {
        if (dirty != DirtyFlag.CLEAN || childDirty) {
            dirty = DirtyFlag.CLEAN;
            childDirty = false;
            dirtyBounds.makeEmpty();
            dirtyChildrenAccumulated = 0;

            if (this instanceof NGGroup) {
                List<NGNode> children = ((NGGroup) this).getChildren();
                for (NGNode child : children) {
                    child.clearDirtyTree_();
                }
            }
        }

        if (getClipNode() != null) {
            getClipNode().clearDirty();
        }
    }

…n if they are skipped due to visible==false or opacity==0" This reverts commit 5f9f157.

eduardsdv · 2024-05-03T10:41:32Z

I added a test that shows that the @FlorianKirmaier's fix works. You can start it with: gradlew -PFULL_TEST=true :systemTests:test --tests NGNodeDirtyFlagTest.

To avoid such errors in the future, I would still suggest the refactoring, which I wrote about in my last comment.

@kevinrushforth and @arapte please review.

eduardsdv · 2024-05-06T14:15:07Z

I created an alternative solution for this bug. See PR: #1451

kevinrushforth · 2024-05-11T13:06:39Z

/csr unneeded

openjdk · 2024-05-11T13:06:58Z

@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 · 2024-05-11T19:41:34Z

/csr unneeded

openjdk · 2024-05-11T19:42:17Z

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

FlorianKirmaier · 2024-05-17T12:36:22Z

Sorry for creating the CSR, it was an accident.

Now that Eduard has both created an alternative solution, but also has reviewed that this solution is correct, and provided an unit-test - I think this (or the other) PR is ready to move forwards.

@kevinrushforth
What do you think?

This is still quite a fundamental bug which is probably quite prevalent,
because when it happens it is really hard to notice it.
But it probably happens more often than we think.

kevinrushforth · 2024-05-18T14:13:16Z

Sorry for creating the CSR, it was an accident.

No problem.

Now that Eduard has both created an alternative solution, but also has reviewed that this solution is correct, and provided an unit-test - I think this (or the other) PR is ready to move forwards.

@kevinrushforth What do you think?

Yes, I've asked @arapte to be the primary reviewer on this. I'll leave it to his judgment as to which of the two approaches to take forward.

openjdk bot added the rfr Ready for review label Dec 21, 2023

FlorianKirmaier changed the title ~~8322544 Parts of SG no longer update during rendering - overlapping - culling - dirty~~ 8322619 Parts of SG no longer update during rendering - overlapping - culling - dirty Dec 21, 2023

JDK-8322619 Fix for rendering bug, related to overlap - culling - dir…

43be153

…tynodes

FlorianKirmaier force-pushed the JDK-8322544-render-culling branch from 59ccdbb to 43be153 Compare December 21, 2023 12:53

kevinrushforth requested a review from arapte December 21, 2023 15:10

kevinrushforth reviewed Dec 21, 2023

View reviewed changes

reverted accidental change in the .idea folder

530adc1

openjdk bot changed the title ~~8322619 Parts of SG no longer update during rendering - overlapping - culling - dirty~~ 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty Apr 10, 2024

JDK-8322619: Clear dirty flag on the node and all its children if the…

5f9f157

…y are skipped due to visible==false or opacity==0

eduardsdv added 2 commits May 2, 2024 10:51

Revert "JDK-8322619: Clear dirty flag on the node and all its childre…

2352515

…n if they are skipped due to visible==false or opacity==0" This reverts commit 5f9f157.

JDK-8322619: Add test

e3163f3

eduardsdv mentioned this pull request May 6, 2024

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

Open

3 tasks

openjdk bot added the csr Need approved CSR to integrate pull request label May 10, 2024

openjdk bot removed the csr Need approved CSR to integrate pull request label May 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

FlorianKirmaier commented Dec 21, 2023 •

edited by openjdk bot

bridgekeeper bot commented Dec 21, 2023

mlbridge bot commented Dec 21, 2023 •

edited

FlorianKirmaier commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

kevinrushforth commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

kevinrushforth left a comment

kevinrushforth Dec 21, 2023

FlorianKirmaier Jan 23, 2024

kevinrushforth Dec 21, 2023

FlorianKirmaier commented Dec 21, 2023 •

edited

hjohn commented Dec 22, 2023

FlorianKirmaier commented Dec 22, 2023

FlorianKirmaier commented Jan 16, 2024 •

edited

bridgekeeper bot commented Feb 20, 2024

FlorianKirmaier commented Mar 13, 2024

lukostyra commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

bridgekeeper bot commented Apr 10, 2024

eduardsdv commented Apr 26, 2024

eduardsdv commented Apr 29, 2024

eduardsdv commented May 3, 2024

eduardsdv commented May 6, 2024

kevinrushforth commented May 11, 2024

openjdk bot commented May 11, 2024

kevinrushforth commented May 11, 2024

openjdk bot commented May 11, 2024

FlorianKirmaier commented May 17, 2024

kevinrushforth commented May 18, 2024

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

Are you sure you want to change the base?

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

Conversation

FlorianKirmaier commented Dec 21, 2023 • edited by openjdk bot

Progress

Issues

Reviewing

Webrev

bridgekeeper bot commented Dec 21, 2023

mlbridge bot commented Dec 21, 2023 • edited

Webrevs

FlorianKirmaier commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

kevinrushforth commented Dec 21, 2023

openjdk bot commented Dec 21, 2023

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Dec 21, 2023

Choose a reason for hiding this comment

FlorianKirmaier Jan 23, 2024

Choose a reason for hiding this comment

kevinrushforth Dec 21, 2023

Choose a reason for hiding this comment

FlorianKirmaier commented Dec 21, 2023 • edited

hjohn commented Dec 22, 2023

FlorianKirmaier commented Dec 22, 2023

FlorianKirmaier commented Jan 16, 2024 • edited

bridgekeeper bot commented Feb 20, 2024

FlorianKirmaier commented Mar 13, 2024

lukostyra commented Mar 13, 2024

openjdk bot commented Mar 13, 2024

bridgekeeper bot commented Apr 10, 2024

eduardsdv commented Apr 26, 2024

eduardsdv commented Apr 29, 2024

eduardsdv commented May 3, 2024

eduardsdv commented May 6, 2024

kevinrushforth commented May 11, 2024

openjdk bot commented May 11, 2024

kevinrushforth commented May 11, 2024

openjdk bot commented May 11, 2024

FlorianKirmaier commented May 17, 2024

kevinrushforth commented May 18, 2024

FlorianKirmaier commented Dec 21, 2023 •

edited by openjdk bot

mlbridge bot commented Dec 21, 2023 •

edited

FlorianKirmaier commented Dec 21, 2023 •

edited

FlorianKirmaier commented Jan 16, 2024 •

edited