Potential issue when child expands container using margin

[abvadabra]

While using layout library in my project I've come across somewhat unexpected layouting behaviour.

Minimal reproducible example follows:

1. Create root item of arbitrary size, without any additional configuration it acts as layout container
2. In that root insert row container with unfixed size
3. In that row container insert item with fixed size, and margin from the bottom

Equivalent code:

    lay_id root = lay_item(ctx);
    lay_set_size_xy(ctx, root, 1, 100);

    lay_id row = lay_item(ctx);
    lay_set_contain(ctx, row, LAY_ROW);
    lay_insert(ctx, root, row);

    lay_id child = lay_item(ctx);
    lay_set_size_xy(ctx, child, 1, 50);
    lay_set_margins_ltrb(ctx, child, 0, 0, 0, 10);
    lay_insert(ctx, row, child);

In a case like that I would expect the following

1. row's height becomes 60, since item's height is 50 + item's bottom margin is 10
2. item is placed at the begginng of the row (row pos = [0, 20], item pos = [0,20])

It can be validated by a test:

    lay_run_context(ctx);
    lay_vec4 root_rect = lay_get_rect(ctx, root);
    lay_vec4 row_rect = lay_get_rect(ctx, row);
    lay_vec4 child_rect = lay_get_rect(ctx, child);

    LTEST_VEC4EQ(root_rect, 0, 0, 1, 100);
    LTEST_VEC4EQ(row_rect, 0, 20, 1, 60);
    LTEST_VEC4EQ(child_rect, 0, 20, 1, 50);

It is consistent with like other layout engines implement margins (like yoga layout or flexboxes in basically all modern browsers).

However, in current version of the library:

1. row's height indeed calculated as 60
2. but item is placed above the row, at position [0,15]

From my understanding, the culprit is the lay_arrange_overlay_squeezed_range function, specifically line 1104:

// in LAY_HCENTER case
rect[dim] += (space - rect[2 + dim]) / 2 - margins[wdim];

The following change seems to be fixing the problem, and doesn't break any of the tests

--- a/layout.h
+++ b/layout.h
@@ -1101,7 +1101,7 @@ void lay_arrange_overlay_squeezed_range(
         switch (b_flags & LAY_HFILL) {
             case LAY_HCENTER:
                 rect[2 + dim] = lay_scalar_min(rect[2 + dim], min_size);
-                rect[dim] += (space - rect[2 + dim]) / 2 - margins[wdim];
+                rect[dim] += (space - rect[2 + dim] - margins[wdim]) / 2;
                 break;
             case LAY_RIGHT:
                 rect[2 + dim] = lay_scalar_min(rect[2 + dim], min_size);

Before submitting an actual pull request I would like to ask:

• Whether or not there is any actual reason this function implemented the way it is, or nobody just ever tried such use case
• If you can spot any other side effects of the suggested chang which are not covered by tests, but may turn out to be a breaking change

[randrew]

Hmm. I'm not sure why it's implemented the way it is. It's been a long time since I've looked at it. You might be right that this is a bug or mistake. I wonder if it would mess up anyone else's projects by making this change. Maybe it's fine to change it. Hmm.

[codecat]

This change doesn't appear to break anything in my projects, and I think the suggested behavior makes more sense than the current behavior.

If this change is made, I think you'd probably also want to make the same change in lay_arrange_overlay.

[haoyu234]

I have a use case here, the patch mentioned above would cause a calculation error.

	lay_id root = lay_item(ctx);
	lay_set_size_xy(ctx, root, 400, 400);
	lay_set_margins_ltrb(ctx, root, 50, 50, 50, 50);
	lay_set_contain(ctx, root, LAY_COLUMN | LAY_WRAP);

	lay_id child1 = lay_item(ctx);
	lay_set_size_xy(ctx, child1 , 50, 50);
	lay_set_margins_ltrb(ctx, child1 , 5, 5, 5, 5);
	lay_insert(ctx, root, child1);

	lay_id child2 = lay_item(ctx);
	lay_set_size_xy(ctx, child2, 50, 50);
	lay_set_margins_ltrb(ctx, child2, 5, 5, 5, 5);
	lay_insert(ctx, root, child2);

	lay_run_context(ctx);

	lay_vec4 root_rect = lay_get_rect(ctx, root);
	lay_vec4 child1_rect = lay_get_rect(ctx, child1);
	lay_vec4 child2_rect = lay_get_rect(ctx, child2);

	LTEST_VEC4EQ(root_rect, 50, 50, 60, 400);
	LTEST_VEC4EQ(child1_rect, 57, 195, 50, 50);
	LTEST_VEC4EQ(child2_rect, 57, 255, 50, 50);

Here's the visual output:

Here's my workaround:

// in LAY_HCENTER case
rect[dim] += lay_scalar_max(0, (space - rect[2 + dim] - margins[wdim]) / 2);

After applying the new patch, everything is perfect.
Here's my visualization tool after applying the new patch: online tool

This is the layout file I used for my test, which can be imported directly into the visualizer:
upstream_issue15.json
buju.json

Expected results:

[abvadabra]

@haoyu234

I've also stumbled upon incorrect behaviour being caused by the suggested change in the original issue. However, I believe your workound incorrectly halves margins.

In my application I've been using the following fix, which respects margins at the start and end of the container,

// in LAY_HCENTER case
rect[dim] = lay_scalar_max(rect[dim], rect[dim] + (space - rect[2 + dim]) / 2 - margins[wdim]);

Sadly, I am unable to recall the exact case which prompted such fix

[haoyu234]

You're right, as you can see, I'm using the nim language, and somehow copied the wrong code when translating to the C test case, here are the new expected coordinates:

	LTEST_VEC4EQ(root_rect, 50, 50, 60, 400);
	LTEST_VEC4EQ(child1_rect, 55, 195, 50, 50);
	LTEST_VEC4EQ(child2_rect, 55, 255, 50, 50);

rect[dim] += lay_scalar_max(0, (space - rect[2 + dim]) / 2 - margins[wdim]);

On the bus, I'm still thinking about how x=57 is calculated, and it doesn't feel very reasonable.