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 > I wonder if this change requires HBox/VBox changes as well and therefore > cannot be made separately from #1111 as we expected earlier... @andy-goryachev-oracle thanks for testing this (again). I think it was important to track down the root cause here, so I put some time in tracking this down. Turns out it is a real bug introduced with this tiny code change. The problem was that `Math.ulp` will return infinity when given an infinity, and when you then use that in a subtraction, the result is `Double.NaN` -- this caused controls to go a bit crazy :) I've moved the math functions to a new class to make them easier to test, but also added an extra test case that uses `Region` directly, just in case. I've confirmed the Monkey Tester works now, so if you could take a look one more time :-) ------------- PR Comment: https://git.openjdk.org/jfx/pull/1118#issuecomment-1534433754
