8390433: Add pixel-snapping documentation by mstr2 · Pull Request #2260 · openjdk/jfx

mstr2 · 2026-08-15T18:31:23Z

Pixel snapping is really hard to get right (in fact, it's so hard that even JavaFX itself gets it wrong in so many places).
I've compiled a list of things that I've learned, because there isn't really any good documentation as of yet.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8390433: Add pixel-snapping documentation (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2260/head:pull/2260
$ git checkout pull/2260

Update a local copy of the PR:
$ git checkout pull/2260
$ git pull https://git.openjdk.org/jfx.git pull/2260/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2260

View PR using the GUI difftool:
$ git pr show -t 2260

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2260.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-08-15T18:32:10Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-08-15T18:33:31Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-08-15T18:35:17Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

mlbridge · 2026-08-15T18:39:31Z

Webrevs

01: Full - Incremental (8da4606c)
00: Full (aa010b30)

hjohn

Thanks a lot for documenting this!

I think you highlighted many good points, and I agree with all of them.

I've added a small discussion around the re-snapping (which I think doesn't solve all issue without an adjustment to our snapSize logic) but is about as accurate as it can be with the current state.

That in itself shouldn't stop us from adding these docs, so will approve once the other small points have been discussed/resolved.

hjohn · 2026-08-16T03:13:23Z

+ *     <li>Snapping several children independently can avoid clipping, but at the same time risk exceeding
+ *         the content size defined by the region.


There should be no risk, as the compute results should take snapping into account; this can basically only occur if the calculation used by compute methods is different from the one used during layoutChildren.

hjohn · 2026-08-16T03:19:55Z

+ *         Note that this only applies to <em>independent</em> allocations. If several children must fit within a
+ *         fixed allocated space, the algorithm must consider all children together and coordinate rounding children
+ *         up or down so that their sum does not exceed the allocated space.
+ *     <li><b>Do not repeatedly ceil the same semantic size.</b><br>


I think this applies to all of them (you specifically mentioned ceil), but it is especially dangerous with the ones that use ceil (although I can think we can mitigate this with a small adjustment to the ceil code used if we're willing to be only accurate to say 1 millionth of a pixel).

hjohn · 2026-08-16T03:58:29Z

+ *         (incorrect) form returns {@code 2.0}. This rule does not conflict with the preceding rule: two independent
+ *         pieces of content are two allocations, while two intermediate terms describing one piece of content are
+ *         one allocation.
+ *     <li><b>Re-snap after calculations, using the meaning of the result.</b><br>


I think we may need to check if this actually helps (it sometimes definitely will), even though I've saying this as well there is still a problem:

The snapSize operation first applies its own floating point calculation (multiplying by render scale which can already introduce a tiny error) before calling Math.ceil. So even a correctly snapped end result (say 0.66...667) multiplied by the renderscale 1.5 can become 1.00...001 which is then ceil'd to 2.

This happens because the result of a child's compute, even if snapped, gets used by the parent that treats it as content (using snapSizeX/Y ceiling).

This is why I'm now of the opinion that we should subtract a constant value before ceiling, so the operation becomes:

ceil((v * renderScale) - epsilon) / renderScale

Where the epsilon is set to 1 millionth of a logical pixel (1e-6).

The reasoning to use 1 millionth is:

too large a value may become noticable (ie. 1/10th of a pixel may introduce slight blurriness)

too small a value may not absorb floating point errors that have been multiplied (a spacing * number of children), are using values of fairly high magnitudes (a 100000 pixel screen or group of screens) or are using a fairly high renderscale

1 millionth of a pixel is still unobservable and it is fair to say that a value within 1 millionth of a pixel can be considered to be that pixel

Why not ulp?

It doesn't help correct the result when the calculations have accumulated more than 1 floating point error

It works poorly when the value to snap value is Double.MAX_VALUE (or something equally large) which is used through-out FX (an ulp at that magnitude is like 1e292 pixels) -- ceiling a Double.MAX_VALUE should yield Double.MAX_VALUE not Double.MAX_VALUE - 1e292).

So resnapping may only be needed after significant number of calculations have been done with values that were snapped originally; tiny floating point errors should be absorbed by the parent's snapping (after ceil has been fixed) or by the rendering hardware (usually only accurate up to float precision).

hjohn · 2026-08-16T04:07:23Z

+ *         Apply the snapping policy of the region deliberately:
+ *         {@snippet :
+ *         double rawAvailableWidth = getWidth();
+ *         double rawAvailableHeight = getHeight();
+ *
+ *         // Depending on this region's fitting and overflow policy, snap this region's
+ *         // raw position and raw allocated size to lay out its children.
+ *         }


This is a bit unclear to me, the snippet does not include any snapping?

I've expanded the sample to allocate a single, resizable child.

nlisker

A very nice guide. I've left some comments on readability.

nlisker · 2026-08-16T17:26:03Z

+ *         the final sum with {@code snapSpaceX/Y} again. For more information, refer to <em>Re-snap after
+ *         calculations, using the meaning of the result</em>.


This referred-to section hasn't appeared yet.

Suggested change

* the final sum with {@code snapSpaceX/Y} again. For more information, refer to Re-snap after

* calculations, using the meaning of the result.

* the final sum with {@code snapSpaceX/Y} again. For more information, refer to Re-snap after

* calculations, using the meaning of the result below.

Additionally, I think this "for more information" part appears too late. I'd think it belongs in the section above about classification since it looks like a direct contradiction to what it wrote (sizes are snapped with space suddenly).

The rules about snapping a sum of snapped numbers it split between sections. For sizes, it is explained both here and in Re-snapping (use snapSpace), gaps/spaces are explained here (use snapSpace), and coordinates are explained in Re-snapping (use snapPosition):

* The final {@code snapSpaceX} does not mean that the allocated width is empty space, it merely uses * the snapping method to remove a tiny amount of floating-point drift. * * The same principle applies to coordinates: after calculating a final coordinate from snapped values, use * {@code snapPositionX/Y} to remove potential floating-point drift.

I'd put all 3 of them in one place before they are used and avoid the repetition.

I'll note that it was mentioned previously in Choosing the snapping operation with

<li>{@code snapSpaceX/Y} can also be used to remove floating-point drift from the result of a computation.

so the point about removing floating-point drift is now in 3 places.

I've reordered the bullet points, so that arithmetic drift is explained first. I've also reworded the other bullet points a bit.

nlisker · 2026-08-16T17:45:43Z

+ *         double allocatedWidth = snapSpaceX(firstWidth + gapWidth + secondWidth);
+ *
+ *         // Incorrect: arithmetic can add floating-point noise to snapped values
+ *         double allocatedWidth = firstWidth + gapWidth + secondWidth;
+ *
+ *         // Incorrect: snapSizeX can turn floating-point noise into an extra pixel
+ *         double allocatedWidth = snapSizeX(firstWidth + gapWidth + secondWidth);


The correct example uses snapSpaceX (after snapping individually), but this incorrect example uses snapSizeX without snapping individually. If the idea is to demonstrate re-snapping, then the same method should be used, otherwise it introduces another incorrectness.

I'm not sure what you mean here. The individual components are always snapped in this example, the mistake is using snapSizeX instead of snapSpaceX.

nlisker · 2026-08-16T18:27:21Z

+ *         policy of its parent), it must not reposition or resize itself. If the allocated width or height is not
+ *         aligned, the region cannot both preserve the exact allocation and make its complete bounds pixel-aligned.


"...is not aligned" with what? pixel-aligned?

I've reworded this a bit.

nlisker · 2026-08-16T18:34:31Z

+ *         // Use the snapped width for every dependent height measurement
+ *         double rawChildHeight = boundedSize(
+ *             child.prefHeight(snappedChildWidth),
+ *             child.minHeight(snappedChildWidth),
+ *             child.maxHeight(snappedChildWidth));
+ *
+ *         // Snap the height the child will receive
+ *         double snappedChildHeight = snapSizeY(rawChildHeight);
+ *
+ *         // Incorrect: height is measured for rawChildWidth
+ *         double snappedChildHeight = snapSizeY(boundedSize(
+ *             child.prefHeight(rawChildWidth),
+ *             child.minHeight(rawChildWidth),
+ *             child.maxHeight(rawChildWidth)));


The incorrect example combines the 2 calculations above it, but the mistake is only in the first one. Combining them hides the difference. I would write for the incorrect example:

double rawChildHeight = boundedSize( child.prefHeight(rawChildWidth), child.minHeight(rawChildWidth), child.maxHeight(rawChildWidth)));

for a direct comparison.

snapping doc

aa010b3

openjdk Bot added the rfr Ready for review label Aug 15, 2026

mstr2 mentioned this pull request Aug 16, 2026

8390440: Region.layoutInArea measures content-biased children against an unsnapped value #2261

Open

4 tasks

hjohn reviewed Aug 16, 2026

View reviewed changes

nlisker reviewed Aug 16, 2026

View reviewed changes

review changes

8da4606

		* <li>Snapping several children independently can avoid clipping, but at the same time risk exceeding
		* the content size defined by the region.

		* the final sum with {@code snapSpaceX/Y} again. For more information, refer to <em>Re-snap after
		* calculations, using the meaning of the result</em>.

		* policy of its parent), it must not reposition or resize itself. If the allocated width or height is not
		* aligned, the region cannot both preserve the exact allocation and make its complete bounds pixel-aligned.

Uh oh!

Conversation

mstr2 commented Aug 15, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Aug 15, 2026

Uh oh!

openjdk Bot commented Aug 15, 2026

Uh oh!

openjdk Bot commented Aug 15, 2026

Uh oh!

mlbridge Bot commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

hjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjohn Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlisker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlisker Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mstr2 commented Aug 15, 2026 •

edited by openjdk Bot

Loading

mlbridge Bot commented Aug 15, 2026 •

edited

Loading

hjohn Aug 16, 2026 •

edited

Loading

nlisker Aug 16, 2026 •

edited

Loading