jfx
jfx copied to clipboard
openjdk
8218745: TableView: visual glitch at borders on horizontal scrolling
Alternative PR to https://github.com/openjdk/jfx/pull/1330 which does not modify the layout of VirtualFlow.
This PR fixes the glitching by removing the code in NGNode.renderRectClip, which made many calculations leading to floating point errors.
Interestingly I found out, that getClippedBounds(..) is already returning the correct bounds that just need to be intersected with the clip of the Graphics object.
So the following code is effectively doing the same:
Old:
BaseBounds newClip = clipNode.getShape().getBounds();
if (!clipNode.getTransform().isIdentity()) {
newClip = clipNode.getTransform().transform(newClip, newClip);
}
final BaseTransform curXform = g.getTransformNoClone();
final Rectangle curClip = g.getClipRectNoClone();
newClip = curXform.transform(newClip, newClip); // <- The value of newClip after the transform is what getClippedBounds(..) is returning
newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
Rectangle clipRect = new Rectangle(newClip)
New:
BaseTransform curXform = g.getTransformNoClone();
BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
Rectangle clipRect = new Rectangle(clipBounds);
clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
As you can see, there are very similar, but getClippedBounds does a much better job in calculating the bounds.
I also wrote a tests proving the bug. I took 100% of the setup and values from a debugging session I did when reproducing this bug.
I checked several scenarios and code and could not find any regressions. Still, since this is change affects all nodes with rectangular clips, we should be careful. Performance wise I could not spot any difference, I do not expect any difference. So I would like to have at least 2 reviewers. Note that I will do more testing as well soon on all JavaFX applications I have access to.
As written in the other PR, I have some interesting findings on this particular problem.
Copy&Paste from the other PR:
Ok, so I found out the following:
When a Rectangle is used as clip without any effect or opacity modification, the rendering goes another (probably faster) route with rendering the clip. That's why setting the opacity to 0.99 fixes the issue - another route will be used for the rendering.
This happens at the low level (NGNode) side of JavaFX.
...
I could track it down to be a typical floating point problem
...
The bug always appears when I scroll and the clip RectBounds are something like:
RectBounds { minX:6.999996, minY:37.0, maxX:289.00003, maxY:194.0} (w:282.00003, h:157.0)
...
while this does not happen when the value is something like:
RectBounds { minX:7.000004, minY:37.0, maxX:289.0, maxY:194.0} (w:282.0, h:157.0
Even more details:
- As briefly explained above, due to floating point arithmetic, we may do not get the correct value, leading to an incorrect calculation where 1 pixel is missing. That is why this issue happens only on display scales other than integer values.
- And since only the
ClippedContainerchanges itslayoutXAND thelayoutXof its clip, the bug only appears there - JavaFX Nodes uses
double, while the low level side (NG) usesfloatmostly, which seems to make things less accurate, although not 100% sure if doubles will avoid this particular problem completely, probably not. I'm not sure why this decision was made.
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 2 Reviewers)
Issue
- JDK-8218745: TableView: visual glitch at borders on horizontal scrolling (Bug - P4)
Reviewers
- Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1462/head:pull/1462
$ git checkout pull/1462
Update a local copy of the PR:
$ git checkout pull/1462
$ git pull https://git.openjdk.org/jfx.git pull/1462/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1462
View PR using the GUI difftool:
$ git pr show -t 1462
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1462.diff
Webrev
:wave: Welcome back mhanl! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@Maran23 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:
8218745: TableView: visual glitch at borders on horizontal scrolling
Reviewed-by: angorya, arapte
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 5 new commits pushed to the master branch:
- 56175f40037a6a03f65a99d26b74c7b110d29b70: 8346222: SwingNodePlatformExitCrashTest fails with JUnit 5.11.3
- c5a983952ee62ffa56a55a840b57a357fe204bcc: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D
- b76c05b9356656c1bd6a4a7026b858d3a935a51f: 8335470: [XWayland] JavaFX tests that use AWT Robot fail on Wayland
- 8ae268fbe7b08c20a9c68d5f74389698e5e6ecf0: 8344111: Remove obsolete permission check methods from javafx.graphics
- ebeee75191b14f0a9945f15e5ccc5fb5e9744817: 8344114: Remove obsolete permission check methods from Font classes
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.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
Webrevs
- 06: Full (078a667f)
- 05: Full (cd4a53b2)
- 04: Full (f03da8d2)
- 03: Full (9c1d8fa6)
- 02: Full (67629e86)
- 01: Full - Incremental (bb27b3fe)
- 00: Full (9d20198f)
I'm reasonably sure there was a good reason for the code in NGNode doing what it did. This will need very careful review and testing before we would accept it.
/reviewers 2 reviewers
@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 2 Reviewers).
Reviewers: @arapte @andy-goryachev-oracle
@Maran23 wait for either @arapte or me to review the proposed Prism changes in this PR.
I'm reasonably sure there was a good reason for the code in NGNode doing what it did. This will need very careful review and testing before we would accept it.
100% agree. Note that the code there is a shortcut for performance reasons. Removing it will also fix the bug since the code below is doing the right thing, but will probably result in a performance impact.
Thats why I checked the other path and had a closer look what it does, since it still needs to the right for whatever clip is used. And there I saw that is does nearly the same thing, but with the getClippedBounds instead.
With that in mind, we need to especially check if the fast path did something completely unexpected what the other 'slow' path did not and we may miss now.
I see a problem on Windows 11 at 125% scale: using the tester app in the ticket, the table focus rectangle's right edge is not visible at some width (i.e. appears and disappears when resizing the window width):
When it is visible, there is no jitter described in the ticket. Also, it can be reproduced at 100% scale.
It may or may not be a separate issue.
I see a problem on Windows 11 at 125% scale: using the tester app in the ticket, the table focus rectangle's right edge is not visible at some width (i.e. appears and disappears when resizing the window width):
It may or may not be a separate issue.
Yeah this is a 'known bug' for me, and has nothing to do with this fix. Note that I only can reproduce this with 125% scale, not 100%. Windows 10 here.
Yeah this is a 'known bug' for me, and has nothing to do with this fix.
I do not see the issue without the fix though (at the same resolution).
I do not see the issue without the fix though (at the same resolution).
I can, the TableView is the root of the Scene in this case. Happens inside a StackPane as well.
you are right: I see the focus rectangle jitter at 175% scale on win 11 (w/o the fix), so it must be a different issue. At this scale, it merely shows a thinner line, perhaps that's why I did not notice it earlier.
you are right: I see the focus rectangle jitter at 175% scale on win 11 (w/o the fix), so it must be a different issue. At this scale, it merely shows a thinner line, perhaps that's why I did not notice it earlier.
Yeah exactly, it looks like when the focus rect is on the border of the window, it will sometimes disappear (probably jitter 'out' of the window). When there is some padding inbetween, the focus rect looks like it is jittering instead. Probably the same root cause.
@Maran23 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!
@Maran23 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout 8218745-tableview-glitch-clipping-fix
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@Maran23 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!
@Maran23 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@Maran23 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!
@Maran23 It shows a merge conflict when I tried to merge in the master. Could you please check.
@Maran23 It shows a merge conflict when I tried to merge in the master. Could you please check.
Thanks for the ping, resolved now. Hopefully I did not messed up any JUnit Import. 😄
@Maran23 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!
sorry, please disregard. had the window open where the merge conflict label was still there.