On Tue, 23 Dec 2025 06:45:30 GMT, John Hendrikx <[email protected]> wrote:
>> @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 :) 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 | ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2642214031
