[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: RFR: JDK-8306990: The guarantees given by Region's floor and ceiling functions should work for l
From: Michael =?UTF-8?B?U3RyYXXDnw==?= <mstrauss () openjdk ! org>
Date: 2023-05-31 20:06:18
Message-ID: j_I2js3VKaUj4-SFgblWaTbO_3xowcKtYVLKan1N9Sk=.ce9bac68-b69a-4b10-a2e2-69ad04dc0f50 () github ! com
[Download RAW message or body]
On Fri, 5 May 2023 08:08:30 GMT, John Hendrikx <jhendrikx@openjdk.org> wrote:
> > Region has floor and ceiling functions that ensure that calling them twice in a \
> > row will yield the same result:
> > ceil(x) = ceil(ceil(x))
> >
> > However, due to use of a constant `EPSILON` which is added/subtracted before \
> > doing the rounding, this only works for small numbers (in the range of 0-50 \
> > approximately). For larger values and scales, rounding errors can easily occur. \
> > This is visible as artifacts on screen where controls are a pixel wider than they \
> > should be.
> > The use of the `EPSILON` constant is incorrect, as its value depends on the \
> > magnitude of the value in question (as magnitude increases, the fractional \
> > precision decreases).
> > The Math class offers the function `ulp` that should be used here. It represents \
> > the smallest possible change in value for a given double.
> > Extending the existing test case `snappingASnappedValueGivesTheSameValueTest` to \
> > use larger magnitude numbers exposes the problems.
>
> John Hendrikx has updated the pull request incrementally with three additional \
> commits since the last revision:
> - Remove seed
> - Add test cases for negative values
> - Add copyright headers
LGTM
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/ScaledMath.java \
line 31:
> 29: * Math functions which deal with rounding scaled values.
> 30: */
> 31: public class ScaledMath {
If you want, you could make this class final and give it a private constructor. But \
since this is not public API, it probably doesn't matter all that much.
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line \
1323:
> 1321:
> 1322: double[] scales = new double[] {1.0, 1.25, 1.5, 1.75, 2.0, \
> 1.374562997, 20.0};
> 1323: Random random = new Random();
In my opinion, using a fixed seed would have been preferable here.
-------------
Marked as reviewed by mstrauss (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1118#pullrequestreview-1454069449
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1212251622
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1212250917
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic