On Tue, 3 Jan 2023 06:31:37 GMT, Michael Strauß <mstra...@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 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.java<https://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.java<https://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.java<https://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.java<https://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.java<https://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.java<https://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 GitHub<https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/445*pullrequestreview-1362487101__;Iw!!ACWV5N9M2RV99hQ!KzBy-Nwzdqqe5UkuUWmRz2ReYMEkKWOyZ0g9cDGrsm88iZTdWLOyJ9It_9xcXBR2MwFW5YxmrL31nR_pgSDio965LQaXGA$>, or unsubscribe<https://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: ***@***.***> ------------- PR Comment: https://git.openjdk.org/jfx/pull/445#issuecomment-1488957378
