RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v4]

[eduardsdv]

On Mon, 29 Apr 2024 09:30:18 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> 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.
>
> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8322619: Clear dirty flag on the node and all its children if they are skipped due to visible==false or opacity==0

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();
        }
    }

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1310#issuecomment-2083153697