RFR: JDK-8306990: The guarantees given by Region's floor and ceiling functions should work for larger values [v6]

[Michael Strauß]

On Fri, 5 May 2023 08:08:30 GMT, John Hendrikx <jhendrikx at 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