On Thu, 4 May 2023 09:46:28 GMT, John Hendrikx <jhendr...@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
