8264591: HBox/VBox child widths pixel-snap to wrong value

[mstr2]

The children of HBox/VBox don't always pixel-snap to the same value as the container itself when a render scale other than 1 is used. This can lead to a visual glitch where the content bounds don't line up with the container bounds. In this case, the container will extend beyond its content, or vice versa.

The following program shows the problem for HBox:

Region r1 = new Region();
Region r2 = new Region();
Region r3 = new Region();
Region r4 = new Region();
Region r5 = new Region();
Region r6 = new Region();
r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
r1.setPrefWidth(25.3);
r2.setPrefWidth(25.3);
r3.setPrefWidth(25.4);
r4.setPrefWidth(25.3);
r5.setPrefWidth(25.3);
r6.setPrefWidth(25.4);
r1.setMaxHeight(30);
r2.setMaxHeight(30);
r3.setMaxHeight(30);
r4.setMaxHeight(30);
r5.setMaxHeight(30);
r6.setMaxHeight(30);
HBox hbox1 = new HBox(r1, r2, r3, r4, r5, r6);
hbox1.setBackground(new Background(new BackgroundFill(Color.RED, null, null)));
hbox1.setPrefHeight(40);

r1 = new Region();
r2 = new Region();
r3 = new Region();
r4 = new Region();
r5 = new Region();
r6 = new Region();
r1.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
r2.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
r3.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
r4.setBackground(new Background(new BackgroundFill(Color.GREY, null, null)));
r5.setBackground(new Background(new BackgroundFill(Color.DARKGRAY, null, null)));
r6.setBackground(new Background(new BackgroundFill(Color.BLACK, null, null)));
r1.setPrefWidth(25.3);
r2.setPrefWidth(25.3);
r3.setPrefWidth(25.4);
r4.setPrefWidth(25.3);
r5.setPrefWidth(25.3);
r6.setPrefWidth(25.4);
r1.setMaxHeight(30);
r2.setMaxHeight(30);
r3.setMaxHeight(30);
r4.setMaxHeight(30);
r5.setMaxHeight(30);
r6.setMaxHeight(30);
HBox hbox2 = new HBox(r1, r2, r3, r4, r5, r6);
hbox2.setBackground(new Background(new BackgroundFill(Color.RED, null, null)));
hbox2.setPrefHeight(40);
hbox2.setPrefWidth(152);

VBox root = new VBox(new HBox(hbox1), new HBox(hbox2));
root.setSpacing(20);
Scene scene = new Scene(root, 500, 250);

primaryStage.setScene(scene);
primaryStage.show();

Here's a before-and-after comparison of the program above after applying the fix. Note that 'before', the backgrounds of the container (red) and its content (black) don't align perfectly. The render scale is 175% in this example.

I've filed a bug report and will change the title of the GitHub issue as soon as it's posted.

---

Progress

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

Issue

• JDK-8264591: HBox/VBox child widths pixel-snap to wrong value

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 445

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back mstr2! 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.

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

The bug is posted here:

JDK-8264591: HBox/VBox child widths pixel-snap to wrong value

[mlbridge[bot]]

Webrevs

• 06: Full (fbbc273a)
• 05: Full - Incremental (94c1a7c5)
• 04: Full - Incremental (e8ffd3d7)
• 03: Full - Incremental (c1835959)
• 02: Full - Incremental (ecaed633)
• 01: Full - Incremental (744f19f5)
• 00: Full (c218b809)

[Maran23]

I had a look at the PR. But it took quite a while to fully understand the changes (also the current implementation 😄). I think it make sense to improve it a bit e.g. by adding javadoc to the new methods, maybe also the existing? Other ideas are also welcome. With that said maybe more people will review it then. I will have a full look soon as well. :)

[mstr2]

I had a look at the PR. But it took quite a while to fully understand the changes (also the current implementation 😄). I think it make sense to improve it a bit e.g. by adding javadoc to the new methods, maybe also the existing? Other ideas are also welcome. With that said maybe more people will review it then. I will have a full look soon as well. :)

I've added a bit of documentation.

[mstr2]

@Maran23 Would you be interested in reviewing the implementation and the added documentation?

[mstr2]

I've expanded the documentation of the improved algorithm, which hopefully makes it easier to understand the changes.

[openjdk[bot]]

@mstr2 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mstr2]

@mstr2 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Not sure what happened here. I didn't rebase or force-push.

[mstr2]

@Maran23 You previously reviewed and tested the proposed changes. If everything looks good to you, can you approve this PR?

[kevinrushforth]

This one hasn't been on my queue to review. I remember when I first took a look at it that it seems like a pretty large change that will need very careful review and a lot of testing. I seem to recall there are a couple places where the changes didn't seem quite right, but I'd need to go back and take a look.

@andy-goryachev-oracle raises a good point about perhaps wanting to solve this in a similar way for the various places where we need to do some sort of iterative adjustment (such as here and in the eventual solution for JDK-8299753). It seems also worth looking at what @Maran23 proposes for AnchorPane to fix JDK-8295078 in PR #910 (for which the review still needs to be completed).

[Maran23]

@andy-goryachev-oracle raises a good point about perhaps wanting to solve this in a similar way for the various places where we need to do some sort of iterative adjustment (such as here and in the eventual solution for JDK-8299753). It seems also worth looking at what @Maran23 proposes for AnchorPane to fix JDK-8295078 in PR #910 (for which the review still needs to be completed).

Yes, at this point we should probably discuss what would be the best solution to fix the snapping issues once and for all. We might even want to look into other frameworks like Swing. Maybe there are people internally at Oracle who can give valuable input here too?

[hjohn]

@andy-goryachev-oracle raises a good point about perhaps wanting to solve this in a similar way for the various places where we need to do some sort of iterative adjustment (such as here and in the eventual solution for JDK-8299753). It seems also worth looking at what @Maran23 proposes for AnchorPane to fix JDK-8295078 in PR #910 (for which the review still needs to be completed).

Yes, at this point we should probably discuss what would be the best solution to fix the snapping issues once and for all. We might even want to look into other frameworks like Swing. Maybe there are people internally at Oracle who can give valuable input here too?

I think extracting the common code out of HBox and VBox would be good, as they're basically doing the same thing in a different axis. The code could then be unit tested directly (ie. given a set of min/max/pref sizes, a target size and a "snapper", calculate optimal sizes). Something like this:

 double[] distribute(double space, double[] minWidths, double[] maxWidths, Snapper snapper);

The minWidths/maxWidths would be min + pref or pref + max depending on the situation, but this function wouldn't care. The Snapper here would be aware of the scale, and would be a dummy implementation that does nothing when snapping is disabled.

This purposely does not handle spacing between children, as you can calculate that before hand and adjust space to the actual space available for children, simplifying the code.

Disclaimer: I used to create my own layout manager (see: smartlayout) that took into account min/max and weight of each child, and even groups of children (a restriction over two or more children at once) -- weight would affect resizing, allowing you to make layouts where children took space according to some ratio (1:2:3 for example). This functionality I called a SpaceDistributor which was purposely given a neutral name as it could be used for both horizontal or vertical calculations. When including weight, the calculations could get so complex that I had multiple implementations and an exhaustive search implementation (which was used by unit tests to verify the optimized implementations). This was before scaling became an issue though, so this code did not take snapping values to pixels into account.

[andy-goryachev-oracle]

I suspect John is right: the kind of scenarios that would fail with the proposed code is when we have a) fractional scale and b) min/max constraints c) requirement to fill the available space

The reason it would fail is that the component width is not invariant in respect of translation due to snapping and fractional scale. Not only it means that multiple passes are required, but also that each time something gets changed or even moved, the computation needs to be discarded and re-done. It is essentially the same issue as https://bugs.openjdk.org/browse/JDK-8299753

I think John is also right when talking about the common code. I too, have encountered the problem with the Tree/TableView constrained resize policies (bug mentioned above) as well as with my own table layout implementation https://github.com/andy-goryachev/FxDock/blob/master/doc/CPane.md

Perhaps we could come up with something along the lines John described

double[] distribute(double space, double[] prefWidth, double[] minWidths, double[] maxWidths, Snapper snapper);

though I would think the gaps between the components would have to be accounted for - again, due to snapping and fractional scale (or simply passed as components of the minWidths/maxWidths arrays.

I still owe you guys the formal review - mostly my intent is to hand-craft a case that would illustrate the problem. It is still on my list of things to do.

cheers, -andy

From: John Hendrikx @.> Date: Wednesday, March 29, 2023 at 02:50 To: openjdk/jfx @.> Cc: Andy Goryachev @.>, Mention @.> Subject: [External] : Re: [openjdk/jfx] 8264591: HBox/VBox child widths pixel-snap to wrong value (#445)

@hjohn commented on this pull request.

I only looked at HBox changes:

Although this code is probably an improvement, I'm not convinced it always is doing the right thing with regards to the final child sizes -- I think the container will indeed always fill up perfectly, but I think the child sizes can be off by a few pixels from ideal values given some crafted input values, that would require multiple passes in the adjust loop and multiple "snap" calls being applied.

Extending the tests to verify the final layout widths of the children could alleviate these doubts, as well as adding more edge cases. I don't think the current tests exercise all of the newly written code; a coverage check could verify this.

I think the tests could be written to use an unsnapped simplistic sizing algorithm, and verifies resulting child widths are never off by more than 1 pixel. It may also be possible to set the container to unsnapped, then to snapped and verify sizes never change by more than 1 pixel for random input values.

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151549496__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio97IfcEqEA$:

•    double available = extraWidth; // will be negative in shrinking case
  

•    outer:while (Math.abs(available) > 1 && adjustingNumber > 0) {
  

•        final double portion = snapPortionX(available / adjustingNumber); // negative in shrinking case
  

•        for (int i = 0, size = managed.size(); i < size; i++) {
  

•            if (temp[i] == -1) {
  

• /**

• * Resizes the children widths to fit the target width, while taking into account the resize limits
  

• * for each child (their minimum and maximum width). This method will be called once when shrinking,
  

Perhaps clarify why only once is sufficient for shrinking (if I read the code correctly, I think the reason is that adjustments always use the preferred children widths as a "base" value; the naming however is often generic enough that it could be their current layout widths that are being adjusted to a new reality, in which case even shrinking would need to take Priority into acount).

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151582955__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio97De-abfA$:

 }

• private double growOrShrinkAreaWidths(List<Node>managed, double areaWidths[][], Priority priority, double extraWidth, double height) {

•    final boolean shrinking = extraWidth < 0;
  

•    int adjustingNumber = 0;
  

• /**

• * Shrinks all children widths to fit the target width.
  

• * In contrast to growing, shrinking does not require two phases of processing.
  

I find the terminology used confusing in many areas. Children widths seems to be referring to their preferred widths here, not (for example) their current widths. As these are preferred widths, it makes sense that shrinking would not need to look at priorities; if they had been current widths, you'd need to shrink SOMETIMES nodes first.

Documenting the parameter could alleviate this confusion. It may also be good to re-iterate in every parameter documentation whether this is a snapped value or not.

In fact, reading this a bit more, I think the "input" childrenWidths are pref + unused values (splitting the array would make that clearer). The values are snapped already. The empty values are then filled with minimum values, effectively modifying the array (should be documented), while the pref values are modified to become the layout values.

It looks like the double[][] construct is an optimization to prevent allocating two separate arrays... pretty sure that it would not make any difference if it had been split.

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151589527__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio97K4wqIsg$:

@@ -467,77 +472,170 @@ private double[][] getAreaWidths(List<Node>managed, double height, boolean minim

     return temp;

 }

• private double adjustAreaWidths(List<Node>managed, double areaWidths[][], double width, double height) {

• /**

• * Adjusts the children widths (within their min-max limits) to fit the provided space.
  

• * This is necessary when the HBox is constrained to be larger or smaller than the combined preferred
  

• * widths of its children. In this case, we grow or shrink the children until they fit the HBox exactly.
  

• *
  

• * @return the pixel-snapped content width, which is the combined width
  

• *         of all children as well as the spacing between them
  

• */
  

• private double adjustChildrenWidths(List<Node> managed, double[][] childrenWidths, double width, double height) {

Could you document all the parameters, and mention what they represent (min, max, pref) and whether they're snapped or not?

Further, could you split childrenWidths in its constituent parts (I think it is prefWidths and maxWidths ?) -- the double[][] is confusing and after its initial creation, it can be passed as two separate arrays to subsequent functions for clarity.

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151612290__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio94JmcG_nQ$:

•    double[] usedWidths = areaWidths[0];
  

•    double[] temp = areaWidths[1];
  

•    final boolean shouldFillHeight = shouldFillHeight();
  

•    adjustWidthsWithinLimits(managed, usedWidths, minWidths, targetWidth, managed.size());
  

• }

• /**

• * Grows all children widths to fit the target width.
  

• * Growing is a two-phase process: first, only children with ***@***.*** Priority#ALWAYS} are eligible
  

• * for adjustment. If the first adjustment didn't suffice to fit the target width, children with
  

• * ***@***.*** Priority#SOMETIMES} are also eligible for adjustment.
  

• */
  

• private void growChildrenWidths(List<Node> managed, double[][] childrenWidths, double targetWidth, double height) {

•    double[] currentWidths = childrenWidths[0];
  

This is named usedWidths in the shrink version, while I think it actually are the preferred widths, which will be adjusted in this function to become the final layout widths.

As mutating values in Java is rare, a comment would help to clarify this:

double[] currentWidths = childrenWidths[0]; // initially pref widths, adjusted to layout widths

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151650961__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio94S7vhfPQ$:

• private boolean adjustWidthsWithinLimits(

•        List<Node> managed, double[] currentWidths, double[] limitWidths, double targetWidth, int adjustingNumber) {
  

•    double totalSpacing = (managed.size() - 1) * snapSpaceX(getSpacing());
  

•    // Current total width and current delta are two important numbers that we continuously
  

•    // update as this method converges towards a solution.
  

•    double currentTotalWidth = snapSpaceX(sum(currentWidths, managed.size())) + totalSpacing;
  

•    double currentDelta = targetWidth - currentTotalWidth;
  

•    // We repeatedly apply the following algorithm as long as we have space left to
  

•    // distribute (currentDelta), as well as children that are eligible to grow or
  

•    // shrink (adjustingNumber).
  

•    while ((currentDelta > Double.MIN_VALUE || currentDelta < -Double.MIN_VALUE) && adjustingNumber > 0) {
  

•        // The amount of space that, in the ideal case, we need to add to or subtract from
  

•        // each eligible child in order to fit the children into the target width.
  

•        double idealChange = snapPortionX(currentDelta / adjustingNumber);
  

Can this be 0 ?

For example, I have a targetWidth of 100, and two children that have preferred widths of 49 and 50, both with Priority.ALWAYS. I would need to grow to reach the target width of 100. As far as I can see the values in the code would then be:

targetWidth = 100.0;

currentTotalWidth = 99.0;

currentDelta = 1.0;

adjustingNumber = 2;

This would enter the while loop, and ideal change would be:

idealChange = snapPortionX(1.0 / 2) = 0 // snapPortionX does a "floor".

That doesn't look good.

However, I now see that it wouldn't become an infinite loop, and that's because the input parameter adjustingNumber is being modified, and so doesn't always represent what I think it does.

---

In modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.javahttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*discussion_r1151656708__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio97OrRRKqQ$:

•            currentWidths[i] = snapSizeX(oldWidth + actualChange);
  

•            currentTotalWidth = snapSpaceX(currentTotalWidth + (currentWidths[i] - oldWidth));
  

What I don't like about these two lines is that they're already dealing with values that are all snapped (and thus a fractional portion was lost). Then another calculation is done, and they're snapped again, losing even more information. Or is it that the snapping here is unnecessary as the calculation already involves values that are all have been snapped before?

So either, the snapping isn't needed, or things are being snapped multiple times -- the first is confusing, the second is like rounding values multiple times in a calculation instead of only the end result...

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*pullrequestreview-1362487101__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio965LQaXGA$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AZQ34ZECB4CKOP5B246PPW3W6QAVJANCNFSM42AIJ57Q__;!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio97M0GlT9w$. You are receiving this because you were mentioned.Message ID: @.***>

[mstr2]

Thank you very much @hjohn for your extensive review. Before addressing your comments, I'd like to get some clarification (ideally also from @kevinrushforth), because I am a bit confused by the discussion so far.

This PR is a very narrow patch that fixes an obvious bug in HBox and VBox. It's narrow in that it doesn't change the present algorithm, it merely corrects some of its flaws. There is a little bit of refactoring, but that's only done where it helps readers to better understand the algorithm.

However, the discussion seems to center around the idea of large-scale refactoring, even across multiple components, including open tickets like 8299753. That's not something I can do as part of this PR, and if large-scale refactoring is the way we choose to go forward, I'd rather not spend more of my time on this particular PR. There needs to be a realistic chance that this PR will be accepted for integration basically as it is.

If we want to expand the scope of this work, this PR should be closed and the discussion should be continued on the mailing list.

[andy-goryachev-oracle]

(copying here from the mailing list)

I suspect @hjohn is right: the kind of scenarios that would fail with the proposed code is when we have a) fractional scale and b) min/max constraints c) requirement to fill the available space

The reason it would fail is that the component width is not invariant in respect of translation due to snapping and fractional scale. Not only it means that multiple passes are required, but also that each time something gets changed or even moved, the computation needs to be discarded and re-done. It is essentially the same issue as https://bugs.openjdk.org/browse/JDK-8299753

I think John is also right when talking about the common code. I too, have encountered the problem with the Tree/TableView constrained resize policies (bug mentioned above) as well as with my own table layout implementation https://github.com/andy-goryachev/FxDock/blob/master/doc/CPane.md

Perhaps we could come up with something along the lines John described

double[] distribute(double space, **double[] prefWidth**, double[] minWidths, double[] maxWidths, Snapper snapper);

though I would think the gaps between the components would have to be accounted for - again, due to snapping and fractional scale (or simply passed as components of the minWidths/maxWidths arrays.

[hjohn]

Thank you very much @hjohn for your extensive review. Before addressing your comments, I'd like to get some clarification (ideally also from @kevinrushforth), because I am a bit confused by the discussion so far.

This PR is a very narrow patch that fixes an obvious bug in HBox and VBox. It's narrow in that it doesn't change the present algorithm, it merely corrects some of its flaws. There is a little bit of refactoring, but that's only done where it helps readers to better understand the algorithm.

Yes, I think it's an improvement in filling the boxes properly (I asked for some clarification as I found the code hard to read, but that's probably how you found it). My only worry is that it fixes this specific problem, but may be introducing a new issue (I think snapping values multiple times can introduce artifacts in the child sizes, even though they will all align correctly, I think they might be off a bit). If you can confirm this isn't the case, or if it is, fix those last problems then I think it is a positive change.

I have noticed similar problems on my system where I don't use 100 or 200% scaling, and it would be good to see them fixed.

However, the discussion seems to center around the idea of large-scale refactoring, even across multiple components, including open tickets like 8299753. That's not something I can do as part of this PR, and if large-scale refactoring is the way we choose to go forward, I'd rather not spend more of my time on this particular PR. There needs to be a realistic chance that this PR will be accepted for integration basically as it is.

Those were possible future directions I suppose, not necessarily as part of this PR. I dislike the code duplication, but it was that way already.

[kevinrushforth]

Thank you very much @hjohn for your extensive review. Before addressing your comments, I'd like to get some clarification (ideally also from @kevinrushforth), because I am a bit confused by the discussion so far.

If we can resolve all of the concerns with the existing approach, without the need for a more holistic fix, then it seems OK to proceed with this. The question is: can we?

This PR is a very narrow patch that fixes an obvious bug in HBox and VBox. It's narrow in that it doesn't change the present algorithm, it merely corrects some of its flaws. There is a little bit of refactoring, but that's only done where it helps readers to better understand the algorithm.

Yes, I think it's an improvement in filling the boxes properly (I asked for some clarification as I found the code hard to read, but that's probably how you found it). My only worry is that it fixes this specific problem, but may be introducing a new issue (I think snapping values multiple times can introduce artifacts in the child sizes, even though they will all align correctly, I think they might be off a bit). If you can confirm this isn't the case, or if it is, fix those last problems then I think it is a positive change.

This was one my worries as well, but I haven't looked at it closely enough to point out any specific concerns.

[andy-goryachev-oracle]

@mstr2 could you please merge in the latest master - the main line deviated a bit, it is difficult to test. thank you!

[andy-goryachev-oracle]

Any configuration where we have a fractional screen scale and setHGrow(true) is going to be a tricky one. I don't think a single pass algorithm would ever work because the usual invariants (sum of all widths, snapped width) might change if even one component is moved or resized.

Here is an example where the algorithm fails - I am using the MonkeyTester app https://github.com/openjdk/jfx/pull/1097 - notice the last rectangle goes outside of the available width.

BTW, thanks for showing us how to visualize any possible errors in layout - I shamelessly stole your idea for the Monkey Tester. I used grayscale instead of colors to minimize the impact of LCD pixel layouts.

[hjohn]

I think a single pass algorithm should be possible.

The fractional screen scale should make no difference here as you can treat sizes (after scaling when snapping is on) as integer values. It is then a matter of assigning an integer number to each of the children.

What is clear to me however is that the algorithm is sufficiently complex that it warrants direct unit testing. Verifying adjustments with manual testing is inevitably going to miss regressions as there are a lot of cases to take into account.

I've been testing out a separate function to distribute space with unit tests to verify its behavior, and it is quite similar to what already exists requiring only a single pass. The problem with the current algorithm is more that it is tightly integrated making it hard to test, not that it can't be made to function correctly.

[hjohn]

I've put some additional time into this, and I can only conclude that when snapping is on, that every float calculation that is performed (even something as simple as an addition) must be rounded to pixel boundaries or you risk propagating a tiny error that will result in a comparison going the other way in some edge case.

I've now implemented an alternative in #1111 and to get all tests to perform exactly right and exactly when you'd expect to see a pixel added here or there, a lot of pixel boundary rounding is a requirement... so much so that I wonder if it really is wise to do this with floats -- it may be wiser to multiply everything by render scale first, do the calculations with integers, then restore it to floats. I added @mstr2 's test to this one as well, and it passed unchanged.

[andy-goryachev-oracle]

I think a single pass algorithm should be possible.

In some simple cases, yes. But not in all possible cases.

Let's consider one example, a layout similar to TableView.CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS. When we need to distribute extra space and at least one column (or a Node in a layout) hits its maximum size constraint, all the computations must be discarded and re-done using the maximum width for that node. Since now we have more space to re-distribute, another node might hit its maximum constraint, and so the process must be repeated.

The situation is even worse when we have a fractional scale: the computation must be discarded and re-done not only if one node hits a constraint, but even when it gets moved. Moving a node may not only require us to change the (already computed) width of that component, but also change the remaining delta, which basically invalidates the whole thing.

And finally, I doubt it is possible to use the trick mentioned in the preceding comment - by converting to and computing in the scaled coordinates, for a simple reason - the constraints are set in the original, unscaled and unsnapped coordinates. When you try to convert a constraint to scaled and snapped coordinates, the constraint is not an invariant, that is it changes depending on where the node is positioned.

[hjohn]

I think a single pass algorithm should be possible.

In some simple cases, yes. But not in all possible cases.

Let's consider one example, a layout similar to TableView.CONSTRAINED_RESIZE_POLICY_ALL_COLUMNS. When we need to distribute extra space and at least one column (or a Node in a layout) hits its maximum size constraint, all the computations must be discarded and re-done using the maximum width for that node. Since now we have more space to re-distribute, another node might hit its maximum constraint, and so the process must be repeated.

But that's exactly what HBox already does. And you don't need to discard anything, you just continue but skip nodes that have already reached maximum and distribute the remaining space.

The situation is even worse when we have a fractional scale: the computation must be discarded and re-done not only if one node hits a constraint, but even when it gets moved. Moving a node may not only require us to change the (already computed) width of that component, but also change the remaining delta, which basically invalidates the whole thing.

The start position of the Node should not influence the width or space available, see below.

And finally, I doubt it is possible to use the trick mentioned in the preceding comment - by converting to and computing in the scaled coordinates, for a simple reason - the constraints are set in the original, unscaled and unsnapped coordinates. When you try to convert a constraint to scaled and snapped coordinates, the constraint is not an invariant, that is it changes depending on where the node is positioned.

I'm sorry, I don't quite see what you mean here, perhaps this is unique to the TableView layout. The constraints are rounded up to the nearest pixel boundary (even the maximum one). They must be as it would not be possible to render both the left edge and right edge of a control on a pixel boundary if constraints weren't rounded. The position is also always on a pixel boundary. The start position of a control has no bearing on the rounding of the constraint to pixels, as pixels are all equally wide.

[andy-goryachev-oracle]

Perhaps i did not emphasized one thing: I believe single pass algorithms would not work when all of the following are true:

• min/max constraint(s)
• grow property(ies) are set
• fractional scale
• snapping

But then again, perhaps I do miss some fundamental aspect of the process, and it is possible. I welcome a PR than proves me wrong!

[andy-goryachev-oracle]

I'm sorry, I don't quite see what you mean here, perhaps this is unique to the TableView layout.

It is not unique. The scenario I am trying to describe is when set[VH]Grow is true, possibly for multiple nodes. With the fractional scale and snapping, the snapped coordinates are not equidistant. So if one node has to move, all of a sudden its width must change, affecting the others. Which means

1. one cannot scale the constraints as they become position-dependent
2. no single pass algorithm is going to work

(the reason I mentioned TableView is because I've tripped over the very same problem, see JDK-8299753)

For reference:

scale=1.25
x=1 snapped=0.800 step=0.800
x=2 snapped=2.400 step=1.600
x=3 snapped=3.200 step=0.800
x=4 snapped=4.000 step=0.800
x=5 snapped=4.800 step=0.800
x=6 snapped=6.400 step=1.600
x=7 snapped=7.200 step=0.800

scale=1.5
x=1 snapped=1.333 step=1.333
x=2 snapped=2.000 step=0.667
x=3 snapped=3.333 step=1.333
x=4 snapped=4.000 step=0.667
...

[hjohn]

Perhaps i did not emphasized one thing: I believe single pass algorithms would not work when all of the following are true:

• min/max constraint(s)
• grow property(ies) are set
• fractional scale
• snapping

But then again, perhaps I do miss some fundamental aspect of the process, and it is possible. I welcome a PR than proves me wrong!

You can check out #1111 -- I'm not aware of any problems in it.

[hjohn]

I'm sorry, I don't quite see what you mean here, perhaps this is unique to the TableView layout.

It is not unique. The scenario I am trying to describe is when set[VH]Grow is true, possibly for multiple nodes. With the fractional scale and snapping, the snapped coordinates are not equidistant. So if one node has to move, all of a sudden its width must change, affecting the others. Which means

1. one cannot scale the constraints as they become position-dependent
2. no single pass algorithm is going to work

(the reason I mentioned TableView is because I've tripped over the very same problem, see JDK-8299753)

For reference:

scale=1.5 x=1 snapped=1.333 step=1.333 x=2 snapped=2.000 step=0.667 x=3 snapped=3.333 step=1.333 x=4 snapped=4.000 step=0.667

Okay, I'm going to explain my understanding of the system, please correct me where I'm going wrong:

We have logical positions and sizes, which are doubles and can be any value, not just integers. And we have physical positions and sizes which can also be any value, but can be snapped if we want them to correspond to physical pixels. These snapped physical coordinates will be integers, and when converted back to logical ones will become fractional for most render scales.

To go from logical to physical, you multiply the logical value by render scale. To go from physical to logical, you divide the physical value by render scale. To "snap" a logical value, you multiple it by the render scale, round it (depending on type), then divide it by render scale. To "snap" a physical value, you round it to an integer.

Scale	Logical	Physical	Logical (snapped)	Physical (snapped)
1.0	0.0	0.0	0.0	0.0
1.0	0.5	0.5	1.0	1.0
1.0	1.0	1.0	1.0	1.0
1.0	1.5	1.5	2.0	2.0
1.5	0.0	0.0	0.0	0.0
1.5	0.5	0.75	0.66	1.0
1.5	1.0	1.5	1.33	2.0
1.5	1.5	2.25	1.33	2.0

HBox will determine positions itself, and is not limited to integer logical coordinates, see example:

Given a scale of 1.5, and HBox has a logical margin of 1, and snapping is on. After snapping, the margin becomes a logical width of 1.33 (2 pixels wide) [calculation: round(1.0 * renderScale) / renderScale]. The first control therefore gets positioned at a logical position of 1.33. It's logical width can take on any multiple of the physical pixel size divided by the render scale, which is 1 / 1.5 = 0.66. So it can span from 1.33 to 2.0 or 1.33 to 2.66 or 1.33 to 3.33, etc.

What am I missing?

Are the column widths in TableView perhaps restricted to integer values?