RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v7]
John Hendrikx
jhendrikx at openjdk.org
Wed Mar 29 09:53:09 UTC 2023
On Tue, 3 Jan 2023 06:31:37 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
>> 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.
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
>
> - changes per review
> - Merge branch 'master' into fixes/box-snap-to-pixel
>
> # Conflicts:
> # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/HBoxTest.java
> # modules/javafx.graphics/src/test/java/test/javafx/scene/layout/VBoxTest.java
> - Merge branch 'openjdk:master' into fixes/box-snap-to-pixel
> - revert snappedSum
> - don't call snappedSum in hot loop
> - Improved code documentation
> - Merge branch 'master' into fixes/box-snap-to-pixel
> - changed some method names, make test config a local class
> - added documentation, improved method names
> - Merge branch 'master' into fixes/box-snap-to-pixel
> - ... and 2 more: https://git.openjdk.org/jfx/compare/a35c3bf7...fbbc273a
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.
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 483:
> 481: * of all children as well as the spacing between them
> 482: */
> 483: 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.
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 504:
> 502: /**
> 503: * Shrinks all children widths to fit the target width.
> 504: * 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.
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 527:
> 525: */
> 526: private void growChildrenWidths(List<Node> managed, double[][] childrenWidths, double targetWidth, double height) {
> 527: 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
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 559:
> 557: /**
> 558: * Resizes the children widths to fit the target width, while taking into account the resize limits
> 559: * 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).
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 586:
> 584: // The amount of space that, in the ideal case, we need to add to or subtract from
> 585: // each eligible child in order to fit the children into the target width.
> 586: 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.
modules/javafx.graphics/src/main/java/javafx/scene/layout/HBox.java line 604:
> 602: double oldWidth = currentWidths[i];
> 603: currentWidths[i] = snapSizeX(oldWidth + actualChange);
> 604: 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...
-------------
PR Review: https://git.openjdk.org/jfx/pull/445#pullrequestreview-1362487101
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151589527
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151582955
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151612290
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151549496
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151650961
PR Review Comment: https://git.openjdk.org/jfx/pull/445#discussion_r1151656708
More information about the openjfx-dev
mailing list