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

[Andy Goryachev]

On Thu, 27 Apr 2023 10:11:42 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.

looks good with minor change requests.  if you agree to make them, i'll re-approve.

Note: this PR came out of review https://github.com/openjdk/jfx/pull/1111

I tried to test *every* double value in the range from 100.0 to 200.0.  In several hours the test did not even reach 101.0, so testing it exhaustively is probably not feasible.  Using 32 bit floats, it ran through ~8M values in the same range almost instantly.  I don't think we need to add these exhaustive tests, at the same time using random values is also of little utility give the large space of all possible doubles (still good to have though).

For completeness sake:


    @Test
    public void testFloat() {
        int ct = 0;
        for(float x = 0.0f; x<Integer.MAX_VALUE; ) {
            t(x, 1.75);
            x = Math.nextUp(x);
            ct++;
        }
        System.out.println("count=" + ct);
    }
    
    private static void t(double x, double scale)
    {
        double x1 = scaledCeil2(x, scale);
        double x2 = scaledCeil2(x1, scale);
        if(x1 != x2) {
            throw new Error("x=" + x + " x1=" + x1 + " x2=" + x2 + " ==" + (x1 == x2));
        }
    }

    private static double scaledCeil2(double value, double scale) {
        double d = value * scale;
        return Math.ceil(d - Math.ulp(d)) / scale;
    }

modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 310:

> 308:     private static double scaledFloor(double value, double scale) {
> 309:         double d = value * scale;
> 310: 

could we get rid of this newline - here and on lines 330, 413 please?

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

> 1288:             stage.setRenderScaleX(scale);
> 1289:             for (int j = 0; j < 1000; j++) {
> 1290:                 double value = new Random().nextDouble() * Integer.MAX_VALUE;

I'd suggest to set and **print** random seed here and on line 1300.  If the test ever fails with some random value, we should be able to reproduce the issue using that particular seed.

(+ use the same Random instance in the second part of the test)

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

Marked as reviewed by angorya (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1118#pullrequestreview-1404338490
PR Comment: https://git.openjdk.org/jfx/pull/1118#issuecomment-1525923629
PR Comment: https://git.openjdk.org/jfx/pull/1118#issuecomment-1525929191
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1179334170
PR Review Comment: https://git.openjdk.org/jfx/pull/1118#discussion_r1179340194