Compute shrink-to-fit width for flex blocks #559

alml · 2024-01-01T19:38:33Z

mikke89 · 2024-01-04T23:37:40Z

Thanks a lot for the pull request, enhancements to this part of the layout engine is very much welcome.

Can you expand a bit on how this relates to the specific issues in #552? Before and after comparisons would be helpful. If you are still working on this, let me know when you feel it is ready for review.

alml · 2024-01-05T20:41:55Z

The implementation passes my own tests. In particular, I was able to correctly render a pretty complex UI in my MMO game. I made a few corrections to the PR recently after I discovered a couple more corner cases. The major one was an assertion in the flex layout engine where it incorrectly estimated size of nested elements when both element and container had an auto width. Examples that cover those corner cases are available below.

From the correctness standpoint, you can review and even merge this PR right away. I'm pretty confident that it won't break existing applications and can only make it better for auto-sized flex elements.

However, from performance standpoint, it's terribly slow. Existing applications may be hit by the combinatorial explosion that you highlighted in #552 (comment). The engine tries to compute the same layouts of nested elements with the same parameters over and over again. I think if we do some memoization of computed layouts to return the same result if called with the same parameters, it would immediately yield enormous performance benefits reducing algorithmic complexity from O(2^N) to O(N).

I would appreciate it if you could review the code as is (maybe move it to another branch) and then perhaps help with caching/memoization, because it's going to be a pretty deep change to the layout engine.

If you have other ideas, I'd love to hear them.

Case 1

<rml>
	<head>
		<style>
			* {
				box-sizing: border-box;
		    	        font-family: Play;
			}

			.outer {
			        display: flex;
			        border: 1px red;
                                padding: 50px;
			}

                       .inner {
                                border: 1px blue;
                                padding: 50px;
                       }
		</style>
	</head>
	<body>
                <div class="outer"><div class="inner">Block 1</div></div>
	</body>
</rml>

Before:

After:

Case 2

<rml>
    <head>
        <style>
            * {
                box-sizing: border-box;
                font-family: Play;
            }

            .block {
                display: block;
                width: 50px;
                height: 50px;
                margin: 10px;
            }

            .red {
                background-color: red;
            }

            .green {
                background-color: green;
            }

            .blue {
                background-color: blue;
            }

            .outer {
                display: flex;
                flex-direction: row;
            }

            .inner {
                display: flex;
                flex-direction: column;
            }
        </style>
    </head>
    <body>
        <div class="outer">
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
        </div>
    </body>
</rml>

Before:

After:

Case 3

<rml>
    <head>
        <style>
            * {
                box-sizing: border-box;
                font-family: Play;
            }

            .block {
                display: block;
                width: 50px;
                height: 50px;
                margin: 10px;
            }

            .red {
                background-color: red;
            }

            .green {
                background-color: green;
            }

            .blue {
                background-color: blue;
            }

            .outer {
                display: flex;
                flex-direction: column;
            }

            .inner {
                display: flex;
                flex-direction: row;
            }
        </style>
    </head>
    <body>
        <div class="outer">
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
            <div class="inner">
                <div class="block red"></div>
                <div class="block green"></div>
                <div class="block blue"></div>
            </div>
        </div>
    </body>
</rml>

Before:

After:

Case 4

<rml>
	<body style="display: flex; flex-direction: column">
	        <div style="padding: 42px; background: blue; border: 5px #00ffff">
	        	<div style="display: flex; background: green; padding: 10px; border: 5px #ff00ff">
				<div style="width: 10px; height: 10px; background: red; border: 5px #ffff00">
				</div>
			</div>
        	</div>
	</body>
</rml>

Before:

And a failed assertion in the console:

2024-01-05T20:27:52.7760615+00:00 [ERROR] RML: RMLUI_ASSERT(containing_block.x >= 0.f)
external/rmlui~override/Source/Core/Layout/FlexFormattingContext.cpp:53

After:

Case 5

<rml>
    <head>
        <style>
            * { align-items: flex-start; }
        </style>
    </head>
    <body id="o" style="display: flex; flex-direction: row; width: 100%; height: 100%">
        <div id="a" style="display: flex; flex-direction: row; border: 5px yellow">
            <div id="b" style="display: flex; flex-direction: row; border: 5px red; width: 100px; height: 100px;">
                <div id="c" style="display: flex; flex-direction: row; border: 5px green">
                    <div id="d" style="display: flex; flex-direction: row; border: 5px blue; width: 50px; height: 50px;">
                    </div>
                </div>
            </div>
        </div>
    </body>
</rml>

Before:

After:

Case 6

<rml>
    <head>
        <style>
            * { align-items: flex-end; }
        </style>
    </head>
    <body id="o" style="display: flex; flex-direction: column; width: 100%; height: 100%">
        <div id="a" style="display: flex; flex-direction: column; border: 5px yellow">
            <div id="b" style="display: flex; flex-direction: column; border: 5px red; width: 100px; height: 100px;">
                <div id="c" style="display: flex; flex-direction: column; border: 5px green">
                    <div id="d" style="display: flex; flex-direction: column; border: 5px blue; width: 50px; height: 50px;">
                    </div>
                </div>
            </div>
        </div>
    </body>
</rml>

Before:

After:

alml · 2024-01-06T10:38:56Z

Actually, you know what? it turned out that caching shrink-to-fit width is pretty simple. Check it out - d9a4627. Layout performance immediately went back to normal.

mikke89

This approach looks simple and effective, nice! :) The spec goes on about handling different flex factors and such, but a simpler approach could be more suitable for us. Regardless, this is non-breaking behavior, as we didn't support shrink-to-fit here previously. And then we could improve later if we find a case for that.

I believe we might have some issues with handling percentage-based sizing here (and generally in shrink-to-fit contexts?), which is something we might want to improve later. But this probably requires more thorough layout engine-wide changes implementing stronger typing for indefinite sizing and sizing constraints.

I ran this through all of our own and generated layout tests to confirm we didn't break anything. Most tests are equal to before, only two CSS flex tests had been changed. The changes being improvements - now both of them matches their reference! Attaching screenshots of the changes at the bottom here .

Your examples here look great too, appreciate all the details. I wonder if you could add some, or all of these to the VisualTests? That way we ensure they don't break in the future.

I only have some minor comments for this first commit, otherwise I'm mostly ready to merge this.

When it comes to the caching commit, this is something I have very much been interested in looking at for a while. However, that is also a very big change (code is small but implications are large), and requires a lot more consideration. We should move that to a new PR and discuss things thoroughly.

Yellow frame is before, document to the right is reference.

mikke89 · 2024-01-08T22:55:41Z

Source/Core/Layout/FlexFormattingContext.cpp

@@ -315,10 +336,11 @@ void FlexFormattingContext::Format(Vector2f& flex_resulting_content_size, Vector
 			RMLUI_ASSERT(initial_box_size.y < 0.f);

 			Box format_box = item.box;
-			if (initial_box_size.x < 0.f)
+			if (initial_box_size.x < 0.f && flex_available_content_size.x >= 0.f)


Does flex_available_content_size.x < 0.f mean indefinite size? Does it carry the meaning of a max-content sizing context, or can it be negative any other way? I think we should give this condition a named bool if we can.

Frankly I don't fully understand this code and can't exhaustively list all cases when it can happen. It definitely happened in shrink-to-fit context, and there it meant infinite max-content box. What I can say for sure that if a negative width makes its way to SetContent, then it will definitely fail an assertion later on. See my original comment. I was able to trip that assertion even before my changes, so I think this change is an improvement. Whether it's universally correct, I don't know.

Yeah, I believe generally this is considered indefinite, which in the context of available space would mean infinite. The spec goes into a lot more detail for these calculations, but his should definitely be an improvement.

I think it is fine as it is written here.

Source/Core/Layout/FlexFormattingContext.cpp

Source/Core/Layout/LayoutDetails.cpp

Source/Core/Layout/ContainerBox.cpp

Source/Core/Layout/LayoutDetails.cpp

mikke89 · 2024-01-14T23:24:49Z

I'm happy with these changes now, thanks! The only thing I am hoping for is to add some or all of these cases to the visual tests.

Paril · 2024-01-18T04:31:11Z

Just out of curiosity, does this change allow inline-flex to be removed from the various checks and use the shrink-to-width now, or is that a different can of worms?

alml · 2024-01-18T07:07:47Z

Hey @Paril, I did not test if my change is sufficient to support inline-flex. Please look at https://github.com/mikke89/RmlUi/pull/559/files#diff-a61d267ac4ed1715193d50f64ac3b0458e22790a8a270be7a7e9c89d5970e0f4R261 - try to remove InlineFlex check from the condition and see what happens. If it works correctly, I'm happy to update my PR.

@mikke89 Could you merge it as is and help with visual tests as a follow-up? My environment is non-standard - I use bazel to build RmlUi instead of cmake, and don't have much time figuring out how to install all the necessary dependencies on Windows.

Paril · 2024-01-18T07:12:48Z

Hey @Paril, I did not test if my change is sufficient to support inline-flex. Please look at https://github.com/mikke89/RmlUi/pull/559/files#diff-a61d267ac4ed1715193d50f64ac3b0458e22790a8a270be7a7e9c89d5970e0f4R261 - try to remove InlineFlex check from the condition and see what happens. If it works correctly, I'm happy to update my PR.

Surprisingly, I did do this, and it appeared to work - but I don't know enough about the codebase to know if that's the only thing that prevents it from working properly, heh. See #577 (comment)

I might be able to help with the visual tests, since I pretty much exclusively use flexbox in any web development that I do.

alml · 2024-01-18T07:42:29Z

I don't think it's that surprising. IIUC inline-flex has only minor differences compared to flex - basically how the resulting block is laid out, not how its content is laid out (which this PR is about). Since you confirmed that it worked for you, I removed the check. Anyway it's going to be an improvement compared to what RmlUi had before, so @mikke89 seems to be open to such changes.

Paril · 2024-01-18T07:45:27Z

Keep in mind it was two places - I had to fix this line as well:

RmlUi/Source/Core/Layout/LayoutDetails.cpp

Line 453 in d0de094

const bool shrink_to_fit = !replaced_element &&

Refs mikke89#552

alml · 2024-01-18T07:48:13Z

Done

Paril · 2024-01-18T08:03:38Z

Cool, let me re-merge and see how it looks. I'll edit this comment when I test it.

EDIT: yep, this fixes #577 with inline-flex; it is now shrinking to content properly.

mikke89 · 2024-01-21T16:00:41Z

Great, thanks a lot! This is a very nice addition.

It definitely makes sense to enable this behavior for inline-flex too.

I'll go ahead and add those visual tests.

mikke89 · 2024-01-21T18:27:30Z

Visual tests added here: ab20b36

Also, added a benchmark: 37d0001

This PR didn't affect the numbers for our existing benchmarks. After adding some new shrink-to-fit cases, naturally we see a slow-down in these cases, compared to the before the changes in this PR, where we just didn't really format it.

Before:

relative	ns/op	op/s	err%	Flexbox shrink-to-fit
100.0%	1,891.67	528,634.36	1.5%	`Reference`
8.4%	22,620.00	44,208.66	1.3%	`Basic shrink-to-fit`
1.2%	156,100.00	6,406.15	0.6%	`Nested shrink-to-fit`

After:

relative	ns/op	op/s	err%	total	Flexbox shrink-to-fit
100.0%	1,894.64	527,803.96	0.4%	0.00	`Reference`
6.7%	28,375.00	35,242.29	1.5%	0.00	`Basic shrink-to-fit`
0.1%	2,911,200.00	343.50	0.3%	0.03	`Nested shrink-to-fit`

As discussed earlier, the nested case is definitely something to be aware of.

alml force-pushed the master branch 4 times, most recently from c68bfdc to f51b5a1 Compare January 4, 2024 23:32

mikke89 added enhancement New feature or request layout Layout engine issues and enhancements labels Jan 4, 2024

mikke89 reviewed Jan 8, 2024

View reviewed changes

alml force-pushed the master branch from d9a4627 to 6bacefb Compare January 14, 2024 13:24

Paril mentioned this pull request Jan 18, 2024

Flex expanding width to edge #577

Closed

alml force-pushed the master branch from 6bacefb to d0de094 Compare January 18, 2024 07:42

Compute shrink-to-fit width for flex blocks

8f1d503

Refs mikke89#552

alml force-pushed the master branch from d0de094 to 8f1d503 Compare January 18, 2024 07:47

mikke89 merged commit 5961ff5 into mikke89:master Jan 21, 2024
18 checks passed

mikke89 mentioned this pull request Sep 14, 2024

Table columns are overlapped inside flexbox #671

Closed

AmaiKinono mentioned this pull request Sep 17, 2024

Compute shrink-to-fit width for tables #673

Closed

mikke89 mentioned this pull request Dec 11, 2024

Improve shrink-to-fit layout (fit-content sizing) #552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute shrink-to-fit width for flex blocks #559

Compute shrink-to-fit width for flex blocks #559

alml commented Jan 1, 2024

mikke89 commented Jan 4, 2024

alml commented Jan 5, 2024

alml commented Jan 6, 2024

mikke89 left a comment

mikke89 Jan 8, 2024

alml Jan 14, 2024

mikke89 Jan 14, 2024

mikke89 commented Jan 14, 2024

Paril commented Jan 18, 2024

alml commented Jan 18, 2024

Paril commented Jan 18, 2024

alml commented Jan 18, 2024 •

edited

Loading

Paril commented Jan 18, 2024

alml commented Jan 18, 2024

Paril commented Jan 18, 2024 •

edited

Loading

mikke89 commented Jan 21, 2024

mikke89 commented Jan 21, 2024

Compute shrink-to-fit width for flex blocks #559

Compute shrink-to-fit width for flex blocks #559

Conversation

alml commented Jan 1, 2024

mikke89 commented Jan 4, 2024

alml commented Jan 5, 2024

Case 1

Case 2

Case 3

Case 4

Case 5

Case 6

alml commented Jan 6, 2024

mikke89 left a comment

Choose a reason for hiding this comment

mikke89 Jan 8, 2024

Choose a reason for hiding this comment

alml Jan 14, 2024

Choose a reason for hiding this comment

mikke89 Jan 14, 2024

Choose a reason for hiding this comment

mikke89 commented Jan 14, 2024

Paril commented Jan 18, 2024

alml commented Jan 18, 2024

Paril commented Jan 18, 2024

alml commented Jan 18, 2024 • edited Loading

Paril commented Jan 18, 2024

alml commented Jan 18, 2024

Paril commented Jan 18, 2024 • edited Loading

mikke89 commented Jan 21, 2024

mikke89 commented Jan 21, 2024

alml commented Jan 18, 2024 •

edited

Loading

Paril commented Jan 18, 2024 •

edited

Loading