On Mon, 22 Dec 2025 20:16:37 GMT, Cormac Redmond <[email protected]> wrote:
>> Yeah, that's going to cause problems at some point. It must be resnapped >> when doing comparisons with it. > > @hjohn: I appreciate the above is a lot of text (this close to Xmas!)... > > I went ahead and made the changes so it's easier to see just the diff: > https://github.com/openjdk/jfx/pull/2016/files ... I can revert if I am > wrong. I think it's safe, and the simplest change to fix both `length` and > `x` issues. It can get complicated, I had to think a bit on the response. The main danger is switching to `snapSize` for your final step, as it is always rounds up, which can easily result in a huge jump (ie. 6.0000001 -> 7) that clearly isn't intended at all. Also, each Node has its own snapping setting. You are doing calculations for the current node. The node you are accessing with `prefHeight` may have a different setting. It is therefore probably wise to snap any external values first so they adhere to the same snapping setting as the current node you are working on before continuing calculations. This is especially important when there are multiple nodes involved as you may get multiple unsnapped values that you really shouldn't add together before snapping them to your current node's setting. But you are also now using the `snapSize` function (in the outer part) which IMHO is the "dangerous" variant as it does a ceiling operation. So if either `spacing` or `x` are even slightly off, it rounds up. For example on a 150% screen pixels are 2/3, so let's say spacing is `x` is 2/3, `spacing` is 4/3, adding those two together can yield subtle errors, and could result in for example `1.9999` or `2.0001`. Now because the outer function is rounding up (to the next pixel), that can get amplified badly. You would expect the result to be `2` (3 pixels of 2/3 size) but it may easily become `2.66666` (4 pixels of 2/3 size)... By applying the ceiling function only to `prefHeight` then rounding, you are much more likely to get a stable result when adding many values together. Perhaps a test case is missing that can show this subtle problem :) ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2642165685
