Fix text flow container layout changing after one frame in some cases #6556
+310
−145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes https://discord.com/channels/90072389919997952/1327149041511043134/1351809773510590494. Essentially what's happening here is that new lines have weird sizing rules wherein the height of the new line is relative to the height of the line preceding it. ppy#6545 broke this because prior to it, the lines responsible for this, namely https://github.com/bdach/osu-framework/blob/769a5d5803709a99ef92503d08c192f13e7d913e/osu.Framework/Graphics/Containers/TextFlowContainer.cs#L448-L452 ran in `TextFlowContainer.UpdateAfterChildren()` before the `base` call, and because `TextFlowContainer` inherited `FillFlowContainer`, changing the inheritance to composition essentially reordered the call so that the height set happened *after* `FillFlowContainer`'s layout logic ran, which is why the layout could be essentially wrong for a frame (with the new lines having height of zero). After several hours of trying several hacks to jam this behaviour back where it should be, I noticed that the entire flow was just broken even before I started touching it. There were two completely independent invalidation paths (one in fill flow, one in text flow), and even before any of my recent changes, when (ab)using some rarely-used features like first line indent you could provoke the text flow to start twitching. So what this does is that it takes the entirety of `ComputeLayoutPositions()` from fill flow, and adapts it to actually properly handle all of the features that text flow needs without post-facto position or margin adjusting hacks.
smoogipoo
requested changes
Mar 24, 2025
This was failing 10 steps away from the actual problem due to propagation of weird float values (infinities, NaNs, etc.) The actual problem was that the offset logic was doing questionable things when the flow was doing initial autosizing in the X direction. In that case the maximum width was initialised to `float.MaxValue`, which then in subsequent calculations got reduced to a very-large-but-finite X position of everything in the flow, and things just broke. To circumnavigate that, initialise max size using saner `float.PositiveInfinity` values (which can be used as markers later), store the actual widths of the lines rather than the offsets to the right, and then in the second pass, account for the infinite expansion due to autosize case by overwriting the max width with the width of the broadest row.
smoogipoo
approved these changes
Mar 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC. This probably needs more testing but I want to see what the initial review reaction to this is going to be.
Fixes https://discord.com/channels/90072389919997952/1327149041511043134/1351809773510590494. Probably also the issue mentioned in ppy/osu#32475 (comment), but I haven't checked.
Essentially what's happening here is that new lines have weird sizing rules wherein the height of the new line is relative to the height of the line preceding it. #6545 broke this because prior to it, the lines responsible for this, namely
osu-framework/osu.Framework/Graphics/Containers/TextFlowContainer.cs
Lines 448 to 452 in 769a5d5
ran in
TextFlowContainer.UpdateAfterChildren()before thebasecall, and becauseTextFlowContainerinheritedFillFlowContainer, changing the inheritance to composition essentially reordered the call so that the height set happened afterFillFlowContainer's layout logic ran, which is why the layout could be essentially wrong for a frame (with the new lines having height of zero).After several hours of trying several hacks to jam this behaviour back where it should be, I noticed that the entire flow was just broken even before I started touching it. There were two completely independent invalidation paths (one in fill flow, one in text flow), and even before any of my recent changes, when (ab)using some rarely-used features like first line indent you could provoke the text flow to start twitching.
So what this does is that it takes the entirety of
ComputeLayoutPositions()from fill flow, and adapts it to actually properly handle all of the features that text flow needs without post-facto position or margin adjusting hacks.As much as I hate to say it, for review, it may be actually best to open
FillFlowContainer.ComputeLayoutPositions()next toInnerFlow.ComputeLayoutPositions()added here and go through them both side-by-side, line-by-line.