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
8211294: ScrollPane content is blurry with 125% scaling #308
Conversation
…screen to prevent it from appearing blurry
|
Webrevs
|
Also, I don't really have an idea on how this could be tested other than visually, so I'm open to suggestions. |
The visual representation corresponds with digits, so there can be tests that check if the numbers are what we expect them to be. |
FYI: there is another bug JDK-8199592 that reads somewhat similar even though the issues are very different. It is also marked as a "Windows" bug. |
The issue will appear consistently as long as the conditions I listed are met, regardless of the actual number of pixels the physical screen can display. |
/reviewers 2 |
@kevinrushforth |
Hello, |
Not yet. I took a quick look, and this is helpful in pointing out where the problem is, but I don't know whether it is the right solution to simply round the transform values when rendering the cache to the screen. |
Hi, Is there anything I can do to help get this moving forward again? As mentioned before, I agree the proposed fix might be simply be addressing the symptoms rather than the root cause and I'm willing to keep working on it but I'd really appreciate a second opinion and some insights as to where to direct my attention if this fix proves to be not enough. For what it's worth, I've been living with this fix on my dev machine for a while now, and it does provide the expected relief to the underlying issue, without me noticing any obvious regressions, neither visually nor performance-wise (I haven't run any targeted benchmarks, though). With JDK-8199592 fixed, I think it'd be great if we could tackle all of the scaling issues in 16 (might be getting a bit late now, though) |
I hope to take a look at this, along with other pending reviews, early next week. There is still some time to get this into 16 if we can find a robust fix. |
That's great news, thanks! |
I spent a bit of time looking at this. I think the root cause of the problem is in ScrollPane itself. It is attempting to layout its children by doing a snap to pixel (meaning that the final scaled translation should be an integer value), but it is failing to do so. This is mostly not a problem when caching is disabled, since our text rendering does sub-pixel antialiasing that looks crisp even at non-integer boundaries. However, translating an already-rendered image by a non-integer boundary will cause the blurriness we are seeing. There is another issue with the Y translation which isn't 0 even when not using a ScrollPane. I'll continue looking at this in the coming week. |
One more comment: given the quality problems that necessarily arise when the translation of a cached image is not on an integer boundary, part of the solution might be to snap the cached image to a pixel boundary as is done in this PR, but we would need to ensure that this doesn't impact smooth scrolling of a TextArea. |
I've done some quick tests with the minimal sample below, and could not notice any performance impact with this patch compared to 15.0.1. public class TextScroll extends Application {
@Override
public void start(Stage stage) throws Exception {
var textArea = new TextArea();
var txt = new File(getClass().getResource("/Scrollpane.java").getFile());
textArea.setText(Files.readString(txt.toPath(), StandardCharsets.UTF_8));
var root = new ScrollPane(textArea);
root.setFitToHeight(true);
root.setFitToWidth(true);
Scene scene;
scene = new Scene(root, 800, 600);
stage.setTitle("Scroll test");
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) {
launch(args);
}
} |
Actually, when I mentioned smooth scrolling, it wasn't performance I was thinking of, it was the ability to scroll by fractional pixel amounts, but with the default snap-to-pixel enabled, that won't happen anyway. I'll look into the other questions you raised tomorrow, specifically what we might want to do about rendering the cache, and also your question about snapToPixel. As for the specific problem with ScrollPane, I found the cause of that one. There is a (long-standing from the look of it) bug in
This fixes the ScrollPane problem without needing to modify the cached rendering code. |
Thanks for that. I am wondering; is it ok to potentially address both sub-issues discussed here (Scrollpane snaping vs cache rendering) under the same JBS bug? Or would it be better to address them under separate issues? Speaking of which, the JBS issue title is in fact misleading, as the issue appears to be not specific to Windows; what's the best practice in such cases; rename the issue? Open a new one? Leave it as is? |
Since both issues -- the snap-to-pixel bug and the rendering of the cached image -- are causing blurriness, it could be OK in principle to address both of them as part of the same bug fix. However, in this instance the snap-to-pixel bug has a simple, well-understood, and safe solution, while the cached rendering bug isn't nearly as simple; there are a couple ways it could be fixed, each with their own implications. Given that, I would prefer to address the snap-to-pixel bug here (which can easily make jfx16), and file a follow-up bug for the cached rendering.
This is easily fixed by renaming the JBS issue, and then updating the PR title to match. I'll update the JBS issue now, after which you can update the PR title. Here are some thoughts about the cached rendering, which should find their way into the new issue: Whatever we do to fix this, the end result should be that rendering a shape or control into a cache and then rendering that cached image should match rendering that shape or control directly. This should be true the first time it is rendered, and should remain true as long as the transform is unchanged (or differs only by a translation delta of whole pixel values) from when the cache was rendered into. This is clearly broken for rendering text if the translation is not on a pixel boundary. Which leads into a question you asked.
A Pane will not force layoutX and layoutY to be on an integer boundary, since it is documented to not alter the position of any of its children. The subclasses of Pane do layout their children, so snap-to-pixel will take effect. So the fact that the controls in your most recent example are being rendered on a non-pixel boundary is not a bug. The fact that the text is so blurry when rendered from a cached image is a bug (as mentioned above). |
For completeness, here is the patch for the snap-to-pixel issue:
We will need a test case for this. You can see UIRenderSceneTest.java for an example of a test that forces Hi-DPI scaling. |
In looking at the other instances where snap-to-pixel is done correctly for Insets, the above should use |
…node to screen to prevent it from appearing blurry" This reverts commit cce931d
I did some testing with your latest patch, and I don't see any problem when dragging a window between screens with different scales. It seems to be recalculating the cached insets and using them in layout as I would expect. Do you have a test case that shows the problem? |
Unfortunately I've only seen it within an application with a fairly complex scene graph, which make isolating the issue tricky. I'm still trying to reproduce it a minimal sample, but in the mean time, please have a look at the a screen capture below, which should at least help clarify the issue I'm observing: This snippet is captured after the window had been moved from a screen scaled to 100% to a second one scaled to 150%; notice how the controls in the "Chart Properties" pane only snap to pixel when the mouse pointer enters the pane. |
Digging deeper into my own code, it appears that there are several areas where I need to account for screen scale within the application's code itself (i.e. I use coordinates from mouseEvent to set the layout of some elements, and these are not snapped), so it probably safe to ignore these issues in the context of the PR. |
So that leaves two follow-on bugs then:
|
I filed the following follow-on bugs:
|
I've added a test that verifies that the inset values for a scrollpane in a scene are snapped to whole pixels, given the actual screen scale. |
The test and the fix look good. I left a couple inline comments.
modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/javafx/scene/UIRenderSnapToPixelTest.java
Outdated
Show resolved
Hide resolved
@arapte I couldn't reproduce the problem you are seeing with the border, but I do see the text spacing issue. Did you merge in the latest changes from master into your local branch when you ran your tests? This is the best way to ensure you are running what will eventually be integrated (especially when the source branch is fairly out of date, which I see that it is in this case). Update: If I run your test program using a build directly from the target branch, I see what you are seeing with the clipped tab pane border. That is likely an unrelated bug that has since been fixed. The text spacing does seem to be changed as a result of this fix. It will need a closer look to know whether this fix has caused the problem, or exposed a bug elsewhere. |
Both border clip and font spacing issues are observed only at 125% scale.
I also see same behavior. Border clip issue does not occur with latest code but the difference in font spacing is observed(only at 125%) |
I'm suspecting that this fix might be revealing an issue related to JDK-8199592. |
@fthevenet The truncated line is very likely related to the fix for JDK-8199592. I always merge in the latest master when doing my testing, which is why I initially didn't see that particular problem. The text spacing is a different problem, since the altered behavior after applying your fix happens with or without the fix for JDK-8199592. It seems likely that it is a separate bug exposed by the fix for this bug rather than caused by it. |
I did some additional testing this morning, and there is definitely an existing problem with positioning labels and text nodes, which is more noticeable when drawing vertical text. The values I see for layoutX and layoutY of the text node (or label) are not correct either before or after this fix. So with this fix, they are just wrong in a slightly different way. So I don't think this should block the integration of this PR. Rather, this is another follow-up bug that we should file. |
@fthevenet This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 64 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.
|
/integrate |
@fthevenet |
/sponsor |
@kevinrushforth @fthevenet Since your change was applied there have been 64 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 9c84c77. |
This PR aims to fix the blurriness to sometimes affects some controls (such as TextArea) in a scene when rendered with a scaling factor that is not an integer (typically when viewed on a HiDPI screen with a 125%, 150% or 175% output scaling).
Please note that regardless of what the JBS issue (and therefore the title of this PR) states, this does not appear to be a Windows only issue as I have observed the same issue on Linux (Ubuntu 20.04).
The following conditions are necessary for the blurriness to appear, but do not guarantee that it will:
The node, or one of its parents, must have set
Node::cacheProperty
totrue
The scaling factor (as detected automatically or explicitly set via glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175)
The effective layout X or Y coordinates for the cached node must be != 0;
Under these conditions, the translate coordinates for the transform used when the cached image for a node is rendered to the screen may be non integer numbers, which is the cause for the blurriness.
Based on these observations, this PR fixes the issue by simply rounding the translate coordinates (using
Math.round
) before the transform is applied inrenderCacheToScreen()
and as far as I can tell, it fixes the blurriness in all the previously affected applications (both trivial test cases or with complex scenegraphs) and does not appear to introduce other noticeable visual artifacts.Still, there might be a better place somewhere else higher up in the call chain where this should be addressed as it could maybe be the root cause for other rendering glitches, though I'm not yet familiar enough with the code to see if it is really the case.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308
$ git checkout pull/308