On Wed, 16 Dec 2020 00:35:08 GMT, Kevin Rushforth <k...@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
