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

[Andy Goryachev]

On Thu, 4 May 2023 09:46:28 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 one additional commit since the last revision:
> 
>   Fix NaN returns when using Math.ulp

This version does not seem to crash the Monkey Tester, both VBox and HBox continue to work.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/ScaledMath.java line 1:

> 1: package com.sun.javafx.scene.layout;

please add a standard header

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/ScaledMath.java line 18:

> 16:      * larger integers with exact precision beyond this limit.
> 17:      *
> 18:      * @param value The value that needs to be floored

is this method only for positive values?

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/ScaledMath.java line 46:

> 44:      * @return value ceiled with scale
> 45:      */
> 46:     public static double ceil(double value, double scale) {

same comment about negative values

modules/javafx.graphics/src/test/java/test/javafx/scene/layout/RegionTest.java line 1356:

> 1354:         // test snapPortionX/snapPortionY methods
> 1355: 
> 1356:         random.setSeed(seed);

I still think we need to remove setSeed() here and below.

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

PR Review: https://git.openjdk.org/jfx/pull/1118#pullrequestreview-1414035195
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1185593563
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1185594018
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1185594187
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1185595181