-
Notifications
You must be signed in to change notification settings - Fork 443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty #1451
base: master
Are you sure you want to change the base?
8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty #1451
Conversation
👋 Welcome back eduardsdv! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
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 |
@kevinrushforth |
/csr unneeded |
@kevinrushforth The CSR requirement cannot be removed as CSR issues already exist. Please withdraw JDK-8332040 and then use the command |
/csr unneeded |
@kevinrushforth determined that a CSR request is not needed for this pull request. |
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 |
The test has already been added to both PRs.
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? |
@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! |
keep alive |
@arapte Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing a few comments on test.
I might need a day or two more for testing/reviewing the fix.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
@kevinrushforth The build fails on Linux. Can you please take a look? |
…/JDK-8322619-render-dirty-flag
Ok. I solved the build by merging from the master branch. |
I have tested the patch with several samples. Fix looks good.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor comments on the tests, apart from the failure that I see on Mac.
Otherwise I could not find any issues.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more test comments.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
Outdated
Show resolved
Hide resolved
…ERANCE when comparing colors
@arapte: The failed test on the Mac is probably caused by the lack of synchronization between the JavaFX and QuantumRenderer threads. I added the |
The newer version still fails on my Mac machine. Attaching modified test that executes as expected on my Windows and Mac machines. |
Yes, the test with your suggestion also works under Windows. Thank you, @arapte. |
Thanks for verifying. In that case, let's remove the |
I removed the method. |
@lukostyra Can you be the second reviewer on this? |
This is an alternative solution to the PR: #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 theclearDirty()
method.Furthermore, since dirty flag is only used when rendering by
ViewPainter
, it should also be deleted byViewPainter
only.This guarantees:
Therefore I removed all calls of the methods
clearDirty()
andclearDirtyTree()
from all other classes except theViewerPainter
.The new version of the
clearDirty()
method together with calling it from theViewerPainter
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 -PUSE_ROBOT=true :systemTests:test --tests NGNodeDirtyFlagTest
Progress
Issues
Reviewers
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