Re: [External] : Re: Consistent baseline layout algorithm
I've created a small sample app (a screenshot is available in the GitHub PR discussion at https://github.com/openjdk/jfx/pull/433), and it seems to me that, while understanding the intricacies of isTextBaseline and isPrefBaseline might be a bit tricky, from an application developer's perspective it seems to "just work" in most cases that I've tried so far. However, I think the documentation still needs to be improved when the concepts settle. As to your questions: it is true that isTextBaseline generally propagates up the scene graph. Parent::isTextBaseline() reports true if, when computing its own baseline offset, it selects a child that reports isTextBaseline()==true as the baseline source. However, even when text baseline children are available, the selected child might not be a text baseline node if prefBaseline=true was specified on a non-text child node to override the default selection. I think isTextBaseline could reasonably be a read-only property, the performance impact is probably negligible if the property is only initialized when it is actually accessed. Am Fr., 16. Apr. 2021 um 19:08 Uhr schrieb Kevin Rushforth : > > Good, since these are the sort of behavioral changes we almost always > avoid. That change in the default behavior likely would not have been > approved. > > As for the rest, I'll need some time to fully understand the impact of > the proposed behavior for propagating isTextBaseline up the tree, and > how that interacts with prefBaseline. It seem like it might be tricky to > document and for app developers to understand all the ramifications. > > A couple questions: First, I presume you are only proposing to propagate > isTextBaseline up the scene graph, meaning that all parents would report > textBaseline == true if that parent is a text baseline node (e.g., > Labeled, TextFlow) or if any descendant is? Second, given that > isTextBaseline is not immutable, have you considered whether it should > be a read-only property, so an application could listen for changes or > bind to it? There would likely be a performance trade-off in making it a > property. > > -- Kevin
Re: [External] : Re: Consistent baseline layout algorithm
Good, since these are the sort of behavioral changes we almost always avoid. That change in the default behavior likely would not have been approved. As for the rest, I'll need some time to fully understand the impact of the proposed behavior for propagating isTextBaseline up the tree, and how that interacts with prefBaseline. It seem like it might be tricky to document and for app developers to understand all the ramifications. A couple questions: First, I presume you are only proposing to propagate isTextBaseline up the scene graph, meaning that all parents would report textBaseline == true if that parent is a text baseline node (e.g., Labeled, TextFlow) or if any descendant is? Second, given that isTextBaseline is not immutable, have you considered whether it should be a read-only property, so an application could listen for changes or bind to it? There would likely be a performance trade-off in making it a property. -- Kevin On 4/16/2021 8:58 AM, Michael Strauß wrote: That's true, and it also seems like outside of synthetic tests, the benefit from this optimization diminishes in larger scene graphs. So I will revert this change. Am Fr., 16. Apr. 2021 um 16:28 Uhr schrieb Scott Palmer : “ I've changed the default alignment for Label to TOP_LEFT (the default alignment of the base class Labeled is CENTER_LEFT).” How can you do that without breaking things? Even though it may be uncommon to set minHeight or prefHeight, that isn’t the point. It still breaks existing code. Scott
Re: [External] : Re: Consistent baseline layout algorithm
That's true, and it also seems like outside of synthetic tests, the benefit from this optimization diminishes in larger scene graphs. So I will revert this change. Am Fr., 16. Apr. 2021 um 16:28 Uhr schrieb Scott Palmer : > > “ I've changed the default alignment for > Label to TOP_LEFT (the default alignment of the base class Labeled is > CENTER_LEFT).” > > How can you do that without breaking things? Even though it may be uncommon > to set minHeight or prefHeight, that isn’t the point. It still breaks > existing code. > > Scott
Re: [External] : Re: Consistent baseline layout algorithm
“ I've changed the default alignment for
Label to TOP_LEFT (the default alignment of the base class Labeled is
CENTER_LEFT).”
How can you do that without breaking things? Even though it may be uncommon to
set minHeight or prefHeight, that isn’t the point. It still breaks existing
code.
Scott
> On Apr 16, 2021, at 12:48 AM, Michael Strauß wrote:
>
> I've learned a few new things while working on the proposed new layout
> algorithm, and added a few new APIs:
>
>
> 1. A central concept of the new algorithm was the notion of a
> text-baseline node, indicated by Node::isTextBaseline(). I've come to
> realize that this property should percolate upwards in the scene
> graph: if a node has a text-baseline, the node's parent should also be
> considered to have a text-baseline. Adding this new behavior works
> surprisingly well and produces very intuitive layout results.
>
>
> 2. The default behavior of all layout containers is to pick the first
> text-baseline child to derive their own baseline offset. I've added
> Node::prefBaselineProperty(), which makes it possible to override this
> default selection: layout containers now pick the first child that
> reports Node::isPrefBaseline(), and only if there is no such child,
> they fall back to Node::isTextBaseline(). Developers can use this
> property to fine-tune their baseline layouts.
>
>
> 3. Optimization: Controls that contain text will often consist of a
> container of some sort and a javafx.scene.text.Text node within it.
> Computing the baseline offset of such a control is very easy with the
> new layout algorithm:
>
> public double getBaselineOffset() {
>return text.getLayoutBounds().getMinY() + text.getLayoutY() +
> text.getBaselineOffset();
> }
>
> This works because changing text.layoutY will automatically schedule
> another layout pass for all of its parents. Re-layouting all parents
> is necessary because changing layoutY can change the effective
> baseline offset, and changing the baseline offset of any node can have
> layout implications on any of its parents.
>
> However, when we consider the Label control (which is probably among
> the most commonly used controls), this can be a bit excessive. Label
> controls are often used to display pure text, and as such, we can
> often "know" the baseline offset without actually needing to schedule
> a second layout pass. This assumption is only correct if the text
> within the Label is top-aligned (because if it isn't, the Label
> baseline offset can not be known in advance of an actual layout pass).
>
> To leverage this assumption, I've changed the default alignment for
> Label to TOP_LEFT (the default alignment of the base class Labeled is
> CENTER_LEFT). In most cases, there will be no visual difference
> anyway, because I imagine Label controls will seldomly be set to a
> minHeight or prefHeight.
>
> This specific scenario will enable an optimization where the first
> layout pass of Label will not schedule a second layout pass. It might
> be possible to find more such scenarios that can benefit from
> fast-path optimizations.
>
>
> 4. In order to get a better understanding of the layout process, I
> added additional logging to track layout passes. Then I compared the
> current algorithm with the new algorithm by tracking the initial
> layout after starting a sample program (i.e. all layout activity until
> the first frame is rendered). In the following log, "cumulative layout
> passes" means how often layoutChildren() has been invoked on any of
> the scene graph nodes. The actual log output includes a tree
> visualization of the entire scene graph that is being layouted, which
> I've removed for the sake or brevity.
>
> Current algorithm log output:
> INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
> layout passes: 49
> INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 43
> INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 0
>
> New algorithm log output:
> INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
> layout passes: 86
> INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 19
>
> A major difference is that the current algorithm will often leave the
> scene graph in a 'dirty' layout state after running a complete layout
> cycle, which necessitates another layout cycle as part of the next
> pulse. The new algorithm, however, leaves the scene graph in a clean
> layout state after a complete cycle, which takes more work at first,
> but saves work that would otherwise be done in the next pulse.
>
>
> 5. Since the new layout algorithm will leave the scene graph in a
> clean state, it is not necessary to repeatedly layout the same thing
> (like in Node::doLayoutPass()). Cases like these should be identified
> and may be changed to single layout invocations.
>
>
> Overall, I think there's good reason to assume that the proposed
> algorithm works and that it produces
Re: [External] : Re: Consistent baseline layout algorithm
I've learned a few new things while working on the proposed new layout
algorithm, and added a few new APIs:
1. A central concept of the new algorithm was the notion of a
text-baseline node, indicated by Node::isTextBaseline(). I've come to
realize that this property should percolate upwards in the scene
graph: if a node has a text-baseline, the node's parent should also be
considered to have a text-baseline. Adding this new behavior works
surprisingly well and produces very intuitive layout results.
2. The default behavior of all layout containers is to pick the first
text-baseline child to derive their own baseline offset. I've added
Node::prefBaselineProperty(), which makes it possible to override this
default selection: layout containers now pick the first child that
reports Node::isPrefBaseline(), and only if there is no such child,
they fall back to Node::isTextBaseline(). Developers can use this
property to fine-tune their baseline layouts.
3. Optimization: Controls that contain text will often consist of a
container of some sort and a javafx.scene.text.Text node within it.
Computing the baseline offset of such a control is very easy with the
new layout algorithm:
public double getBaselineOffset() {
return text.getLayoutBounds().getMinY() + text.getLayoutY() +
text.getBaselineOffset();
}
This works because changing text.layoutY will automatically schedule
another layout pass for all of its parents. Re-layouting all parents
is necessary because changing layoutY can change the effective
baseline offset, and changing the baseline offset of any node can have
layout implications on any of its parents.
However, when we consider the Label control (which is probably among
the most commonly used controls), this can be a bit excessive. Label
controls are often used to display pure text, and as such, we can
often "know" the baseline offset without actually needing to schedule
a second layout pass. This assumption is only correct if the text
within the Label is top-aligned (because if it isn't, the Label
baseline offset can not be known in advance of an actual layout pass).
To leverage this assumption, I've changed the default alignment for
Label to TOP_LEFT (the default alignment of the base class Labeled is
CENTER_LEFT). In most cases, there will be no visual difference
anyway, because I imagine Label controls will seldomly be set to a
minHeight or prefHeight.
This specific scenario will enable an optimization where the first
layout pass of Label will not schedule a second layout pass. It might
be possible to find more such scenarios that can benefit from
fast-path optimizations.
4. In order to get a better understanding of the layout process, I
added additional logging to track layout passes. Then I compared the
current algorithm with the new algorithm by tracking the initial
layout after starting a sample program (i.e. all layout activity until
the first frame is rendered). In the following log, "cumulative layout
passes" means how often layoutChildren() has been invoked on any of
the scene graph nodes. The actual log output includes a tree
visualization of the entire scene graph that is being layouted, which
I've removed for the sake or brevity.
Current algorithm log output:
INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
layout passes: 49
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 43
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 0
New algorithm log output:
INFO: Layouting VBox (triggered by scene out-of-pulse), cumulative
layout passes: 86
INFO: Layouting VBox (triggered by scene pulse), cumulative layout passes: 19
A major difference is that the current algorithm will often leave the
scene graph in a 'dirty' layout state after running a complete layout
cycle, which necessitates another layout cycle as part of the next
pulse. The new algorithm, however, leaves the scene graph in a clean
layout state after a complete cycle, which takes more work at first,
but saves work that would otherwise be done in the next pulse.
5. Since the new layout algorithm will leave the scene graph in a
clean state, it is not necessary to repeatedly layout the same thing
(like in Node::doLayoutPass()). Cases like these should be identified
and may be changed to single layout invocations.
Overall, I think there's good reason to assume that the proposed
algorithm works and that it produces consistent results for
application developers. At this point it would be useful to know
whether or not to continue with the effort.
Re: [External] : Re: Consistent baseline layout algorithm
I haven’t done much in terms of custom controls, but I do see this issue a lot in my primary application. It would be nice to have it fixed as the layout looks rather sloppy without it. Any time I see the layout “correct” itself when I interact with a control or tweak the size of a window it is also distracting. When I saw this fix proposed here my first thought was, “finally!” So I for one am looking forward to this. It seems that the number of iterations will be very low for most cases, so I’m not terribly concerned about any sort of performance impact. But that is my only worry, as I have had issues with layout/CSS taking a lot of time in the past, so in some cases even a single extra iteration may lead to a perceptible delay. (I was mostly working with FX 8 when I had those issues, so hopefully things are better in recent versions, I haven’t measured.) I agree that the default limit could probably be reduced, 10 iterations would likely be plenty more than needed. As you say, something is probably wrong with your control if it goes that far. Regards, Scott > On Apr 2, 2021, at 10:51 AM, Kevin Rushforth > wrote: > > Circling back to this: Do any JavaFX application developers on this list > have any thoughts? I think this is a good idea, and it seems worth moving > this forward, but it will require a fair bit of effort. I'd like to hear > buy-in from other developers -- particularly those who develop or use custom > controls. > > A few additional comments: > > * Regarding the threshold, I like the idea of logging at a fine (or finer) > level if it goes beyond a smaller number, say 3 or 4, passes, but continuing > further to iterate. I wonder if 100 might be too large for the default limit, > though. It seems likely that the control is unstable and will never converge > if it gets to that many layout passes. If there were a way to analyze what a > reasonable limit is that would be better (if not, we can probably live with a > fairly high value, if it really is an unlikely pathological case). Assuming a > reasonably high threshold, logging at a warning level would be good if it > doesn't converge. > > * Documentation: I do think we will want to document the overall concept of > layout needing multiple passes to converge in some cases, along with > documenting the threshold. I also think that Node::getBaselineOffset should > mention the preference of text nodes. The concept of "text" nodes needs to be > on Node, if for no other reason than that Text doesn't inherit from Parent. > > * This will need to be very well tested, both to ensure no regressions in > behavior, and with many new tests added to validate the new functionality, > including testing the threshold logic. > > -- Kevin > > >> On 3/20/2021 6:42 AM, Michael Strauß wrote: >> 1) Pathological cases: So far, I haven't been able to produce >> pathological cases that exceed 3 layout passes by using standard >> layout containers and controls. If we need to have a threshold >> substantially higher than that depends on whether or not there are >> indeed legitimate cases where some kind of layout requires more passes >> to converge. Maybe we could log at a very fine level when a control >> takes more than 3 passes to converge, but still continue processing up >> to a substantially higher number of passes. This would allow >> developers to debug and optimize their custom controls, but still >> produce a valid solution if a particular control is generally stable, >> but just very poorly optimized. >> >> 2) I believe that the multi-pass layout algorithm shouldn't need >> additional documentation since it could be considered an >> implementation detail. There is no 1:1 correspondence between pulses >> and layout passes currently, and there won't be with the new >> algorithm. Also, any layout pass with the current algorithm can >> potentially trigger another layout pass, so that's not fundamentally >> different either. For control authors, we might need to document that >> it is generally okay to rely on circular dependencies between size >> hints, baseline offset and child nodes, as long as those dependencies >> allow the layout to settle. The fact that text nodes are preferred >> with respect to baseline computation is documented in >> Parent.getBaselineOffset(), maybe Node.getBaselineOffset() should also >> mention this. We might also add a section on baseline computation to >> the javafx.scene.layout package documentation, where I think it is >> most relevant (and currently entirely undocumented). >> >> 3) New API on Node: in order for this idea to work, there needs to be >> an abstract concept of "text node". Since the "baseline" concept is >> introduced on Node, I thought this would also be the appropriate place >> to introduce the "text node" concept; however, it could also be >> introduced on Parent instead. Note that the "text node" concept is >> introduced at a lower level than actual text
Re: [External] : Re: Consistent baseline layout algorithm
Circling back to this: Do any JavaFX application developers on this list have any thoughts? I think this is a good idea, and it seems worth moving this forward, but it will require a fair bit of effort. I'd like to hear buy-in from other developers -- particularly those who develop or use custom controls. A few additional comments: * Regarding the threshold, I like the idea of logging at a fine (or finer) level if it goes beyond a smaller number, say 3 or 4, passes, but continuing further to iterate. I wonder if 100 might be too large for the default limit, though. It seems likely that the control is unstable and will never converge if it gets to that many layout passes. If there were a way to analyze what a reasonable limit is that would be better (if not, we can probably live with a fairly high value, if it really is an unlikely pathological case). Assuming a reasonably high threshold, logging at a warning level would be good if it doesn't converge. * Documentation: I do think we will want to document the overall concept of layout needing multiple passes to converge in some cases, along with documenting the threshold. I also think that Node::getBaselineOffset should mention the preference of text nodes. The concept of "text" nodes needs to be on Node, if for no other reason than that Text doesn't inherit from Parent. * This will need to be very well tested, both to ensure no regressions in behavior, and with many new tests added to validate the new functionality, including testing the threshold logic. -- Kevin On 3/20/2021 6:42 AM, Michael Strauß wrote: 1) Pathological cases: So far, I haven't been able to produce pathological cases that exceed 3 layout passes by using standard layout containers and controls. If we need to have a threshold substantially higher than that depends on whether or not there are indeed legitimate cases where some kind of layout requires more passes to converge. Maybe we could log at a very fine level when a control takes more than 3 passes to converge, but still continue processing up to a substantially higher number of passes. This would allow developers to debug and optimize their custom controls, but still produce a valid solution if a particular control is generally stable, but just very poorly optimized. 2) I believe that the multi-pass layout algorithm shouldn't need additional documentation since it could be considered an implementation detail. There is no 1:1 correspondence between pulses and layout passes currently, and there won't be with the new algorithm. Also, any layout pass with the current algorithm can potentially trigger another layout pass, so that's not fundamentally different either. For control authors, we might need to document that it is generally okay to rely on circular dependencies between size hints, baseline offset and child nodes, as long as those dependencies allow the layout to settle. The fact that text nodes are preferred with respect to baseline computation is documented in Parent.getBaselineOffset(), maybe Node.getBaselineOffset() should also mention this. We might also add a section on baseline computation to the javafx.scene.layout package documentation, where I think it is most relevant (and currently entirely undocumented). 3) New API on Node: in order for this idea to work, there needs to be an abstract concept of "text node". Since the "baseline" concept is introduced on Node, I thought this would also be the appropriate place to introduce the "text node" concept; however, it could also be introduced on Parent instead. Note that the "text node" concept is introduced at a lower level than actual text controls like Labeled, because layout containers (which are not controls) need this information. Also, the definition of what constitutes "text" is left to control authors. In a very contrived example, a control might display an image, but declare itself to be "text". Am Fr., 19. März 2021 um 23:26 Uhr schrieb Kevin Rushforth : I'd be interested to hear from app developers on this list. Here are a few quick thoughts I have. As you note, this is a long-standing problem with layout in FX. You mention in the performance considerations that for most cases this will iterate quickly. It would be interesting to know what some of the corner cases are, so we can see how bad the pathological case will be. I see that you propose to iterate up to 100 times. Maybe a lower threshold would be better? We already run layout passes 3 times in many cases. So also running 3 (or maybe 4 or 5) times seems reasonable, especially when your testing shows that most of the time it converges in <= 2 passes. So a smaller threshold than 100 would seem to make sense. If a control doesn't converge, you might consider logging it (in case an app developer wants to debug it) perhaps at a "fine" level so it isn't shown by default. How do you propose to document this behavioral change -- not so much the fact that it will (usually) get
