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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2020-12-16 0:44:57
Message-ID: ACTep1TJLLEkFhwM8QkcMsUK6gnDNKYvGN-bR2Qr28Y=.fee8de79-81ea-4759-b089-886ea29a8ef1 () github ! com
[Download RAW message or body]

On Wed, 16 Dec 2020 00:35:08 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:

> > > 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?
> > 
> > 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. 
> > > 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?
> > 
> > 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.
> > 
> > > What is the legitimate result to expect here; should root.setSnapToPixel(true); \
> > > override setLayoutX(0.5); and align everything for crisp rendering? Or am I \
> > > misunderstanding the scope of setSnapToPixel and it has no effect when layout \
> > > is set explicitly?
> > 
> > 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:
> 
> diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java \
> b/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java index \
>                 565d52b516..00c0f6da61 100644
> --- a/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
> @@ -989,17 +989,11 @@ public class Region extends Parent {
> /** Called to update the cached snapped insets */
> private void updateSnappedInsets() {
> final Insets insets = getInsets();
> -        if (_snapToPixel) {
> -            snappedTopInset = Math.ceil(insets.getTop());
> -            snappedRightInset = Math.ceil(insets.getRight());
> -            snappedBottomInset = Math.ceil(insets.getBottom());
> -            snappedLeftInset = Math.ceil(insets.getLeft());
> -        } else {
> -            snappedTopInset = insets.getTop();
> -            snappedRightInset = insets.getRight();
> -            snappedBottomInset = insets.getBottom();
> -            snappedLeftInset = insets.getLeft();
> -        }
> +        final boolean snap = isSnapToPixel();
> +        snappedTopInset = snapSizeY(insets.getTop(), snap);
> +        snappedRightInset = snapSizeX(insets.getRight(), snap);
> +        snappedBottomInset = snapSizeY(insets.getBottom(), snap);
> +        snappedLeftInset = snapSizeX(insets.getLeft(), snap);
> }
> 
> /**
> 
> We will need a test case for this. You can see \
> [UIRenderSceneTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/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 `snapSpace{X,Y}` rather than `snapSize{X,Y}`

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

PR: https://git.openjdk.java.net/jfx/pull/308


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