[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v4]
From:       eduardsdv <duke () openjdk ! org>
Date:       2024-04-29 16:22:15
Message-ID: 3g99pouwEwYgt8XrKs9A-5mhfw60WUQ1u-dMmpJP3m8=.2e61147f-68d1-4aa1-b203-6cbee15e407a () github ! com
[Download RAW message or body]

On Mon, 29 Apr 2024 09:30:18 GMT, Florian Kirmaier <fkirmaier@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


[prev in list] [next in list] [prev in thread] [next in thread]