On Tue, 23 Dec 2025 07:11:04 GMT, John Hendrikx <[email protected]> wrote:
>> 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 :)
>
> To add a bit more to this, I think there are a few general rules when it
> comes to snapping:
>
> ## 1. Snap External Values Early
> - Each node has its own snap setting.
> - When using values from other nodes (e.g., `prefHeight`), **convert them as
> early as possible into the current node’s snap space** to ensure consistency
> in layout calculations.
>
> ## 2. Use Ceiling for Size Values from Other Nodes
> - External values are usually **sizes**.
> - It is important to fully respect them—for example, a `Text` node requiring
> 11.5774 pixels should not be rounded down to 11.5, as that could trigger
> ellipsis or clipping.
> - When bringing these values into your snap space, use a **ceiling function**
> (`snapSize`) to guarantee the content fits.
>
> ## 3. Use Rounding for Internal Calculations
> - When combining values that **belong to your current node** (spacing,
> margins, gaps) or have been imported already (by snapping an external
> source), use **rounding functions** (`snapPosition`, `snapSpace`).
> - This prevents accumulation of extra pixels that can cause visual
> discontinuities.
>
> ## 4. Current Node Sizes
> - Any sizes that are important for content layout (like text widths or
> heights) should be snapped with **ceiling** (`snapSize`).
> - Other quantities such as spacing or gaps do not need ceiling; small
> discrepancies here are visually insignificant and won’t cause clipping.
> - Apply ceiling **once** when needed; repeated ceiling calls can lead to
> overestimation of total sizes.
>
> ---
>
> ## Summary
>
> | Value Type | Snap Function |
> |--------------------------------|---------------|
> | External node sizes | `ceil` (`snapSize`) |
> | Internal sums / positions | `round` (`snapPosition`, `snapSpace`) |
> | Current node sizes (important) | `ceil` (`snapSize`), once only |
> | Internal spacings / gaps | `round` or unsnapped |
Thanks @hjohn. That all makes sense. I think I am understanding the purpose of
these now (have been shooting from the hip a bit).
Side not: would be good to get the above message somewhere more visible (not
sure where though...), maybe a JavaDoc in Region?
Anyway, after all that, I've pushed what might hopefully be the final change,
and it's confined to just this code in getOverflowNodeIndex()(:
if (node.isManaged()) {
if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
x = snapPositionY(x + snapSizeY(node.prefHeight(-1)) +
getSpacing());
length = snapPositionY(length);
} else {
x = snapPositionX(x + snapSizeX(node.prefWidth(-1)) +
getSpacing());
length = snapPositionX(length);
}
}
I decided to fix `length` within getOverflowNodeIndex() itself rather than in
the calling method (i.e., don't assume it comes pre-snapped). I think since
getOverflowNodeIndex() is doing the comparison, it's its responsibility.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2643165825
