The problems with layouts

[farmaazon]

I discovered a problem with layouts when adding and removing children from a display object. This test fails:

    #[test]
    fn test_automatic_layout_update_after_removing_and_adding_children() {
        let test = TestFlatChildren2::new();
        test.root.use_auto_layout();
        test.node1.set_size((10.0, 10.0));
        test.node2.set_size((10.0, 10.0));

        test.run()
            .assert_root_computed_size(20.0, 10.0)
            .assert_node1_position(0.0, 0.0)
            .assert_node2_position(10.0, 0.0);

        test.root.remove_child(&test.node1);
        test.run().assert_root_computed_size(10.0, 10.0).assert_node2_position(0.0, 0.0);

        test.root.add_child(&test.node1);
        test.run()
            .assert_root_computed_size(20.0, 10.0)
            .assert_node1_position(10.0, 0.0)
            .assert_node2_position(0.0, 0.0);
    }

On first resize after removing child.

For information of @wdanilo and @Frizi as they also are about to use a lot of layouts.

[farmaazon]

It seems that modified_children dirty is not set after removing child.

[farmaazon]

I see some inconsistency in the code. The docs for modified_children dirty say (emphasis mine):

A set of children that were added, removed, transformed, moved to a different layer, or whose ancestors were modified in such a way.

But in the code, we have:

impl Drop for ParentBind {
    fn drop(&mut self) {
        if let Some(parent) = self.parent() {
            if let Some(weak_child) = parent.children.borrow_mut().remove(*self.child_index) {
                parent.dirty.modified_children.unset(&self.child_index);
                if let Some(child) = weak_child.upgrade() {
                    child.dirty.new_parent.set();
                    parent.dirty.removed_children.set(weak_child);
                }
            }
        }
    }
}

So we even explicitly unset the modified_children flag when the child is removed, contradicting the documentation. What is the intention here?

[farmaazon]

I found another possibly error in the code:

   fn refresh_layout(&self) {
        self.reset_size_to_static_values(X, 0.0);
        self.refresh_layout_internal(LayoutResolutionPass::X, PassConfig::Default);
        self.reset_size_to_static_values(Y, 0.0);
        self.refresh_layout_internal(LayoutResolutionPass::Y, PassConfig::Default);
    }

This is called at the beginning of update. reset_size_to_static_values is called unconditionally, seemingly always setting the hug sizes to 0.0. However, refresh_layout_internal have a check for dirty flags. I assume the resetting to static must be done only if refreshing layout is actually done.

[Frizi]

I've managed to create a failing test for this.

#[test]
fn test_layout_double_update() {
    let world = World::new();
    let root = Instance::new_named("Root");
    let child = root.new_child();
    child.set_size((10.0, 10.0));
    root.update(&world.default_scene);
    assert_eq!(root.computed_size(), Vector2(10.0, 10.0));
    root.update(&world.default_scene);
    assert_eq!(root.computed_size(), Vector2(10.0, 10.0));
}

The assert after second update fails.

test display::object::instance::layout_tests::test_layout_double_update ... FAILED

failures:

---- display::object::instance::layout_tests::test_layout_double_update stdout ----
thread 'display::object::instance::layout_tests::test_layout_double_update' panicked at 'assertion failed: `(left == right)`
  left: `Matrix { data: [[0.0, 0.0]] }`,
 right: `Matrix { data: [[10.0, 10.0]] }`', lib\rust\ensogl\core\src\display\object\instance.rs:4578:9

[farmaazon]

Created a PR with fixes for above issues: #6066

@wdanilo please review it ASAP, see if those changes make sense in overall picture.

@Frizi I guess you can cherry-pick those fixes to your branch if needed.

// EDIT: the fix has an issue spotted by Paweł. Working on a fix.

// EDIT 2: ok, the issue is fixed, the commit in PR may be reviewed and used in local branches.

[Frizi]

I cannot find a way to create a grid view with a visible background. Creating shapes that cover non-leaf layout nodes seems to be not possible without handling it manually using on_update signals (which also introduces 1 frame lag).

I have attempted a setup where I have an auto-layout element with 3 children, and it is itself a child of a shape element.

    root.use_auto_layout();

    // auto_container is an auto-layout row with 3 columns
    auto_container.add_child(&inner1); // size is fixed
    auto_container.add_child(&inner2); // size is fixed
    auto_container.add_child(&inner3); // size is fixed

    // auto_container is a child of rectangle::View, in order
    // to create a visible shape around the auto-layout
    rect1_bg.add_child(&auto_container);
    rect1.add_child(&rect1_bg);

    // scene has two rects next to each other
    root.add_child(&rect1); // size is hug
    root.add_child(&rect2); // size is fixed
    world.add_child(&root);

The result of that is that the auto-layout inside rect1_bg ends up being shifted by half of its size. It incorrectly treats the shape's center point as its layout origin.

The shape position is also not correctly updated once it is being animated. I have added a very simple animation, which is updated on before_frame signal

let s = t.since_animation_loop_started.as_s();
let x = (s * 2.0).sin() * 30.0 + 35.0;
inner2.set_size_x(x);

This results in an incorrect motion of the animated element and its parent. The size is updated correctly, but the position (shape's center point) doesn't update.

Both of those issues would probably be fixed if we allowed the sprites to treat their bottom-left corner as origin. Then shapes within auto-layout would always use that mode. We would no longer need the forced_origin_alignment workaround within the layout engine as well.

[wdanilo]

Is this fixed by @farmaazon fixes?

[wdanilo]

If not, could you please @Frizi try fixing it and creating PR?

[Frizi]

In layout algorithm, refresh_grid_layout and refresh_manual_layout methods both receive two distinct arguments, x: Dim, pass: LayoutResolutionPass. I've found that those are always matching each other. Whenever pass is LayoutResolutionPass::X, then we are always also using X dimension.

Is it intentional that those two arguments are separated? If so, what is the reason for that?

[wdanilo]

I believe it is accidental and we can simplify it to 1 arg.

[Frizi]

I have some issues with current behavior of content_origin property. When an object has a child that overflows, it will have content_origin placed at the bottom-left-most corner of the contents. Then, that origin will be applied as a position offset at the next auto-layout parent. As a consequence, it is impossible to intentionally overflow the layout in that specific direction. Even using manual layout and additional wrapper elements. The bottom-left content corner will just propagate through the tree until it is resolved by an auto-layout.

Right now, this prevents me from placing dropdowns within the widget view, as they move the node's contents up when they open.

The way this is implemented, documented and tested, all of this behaviour seems to be very intentional. @wdanilo could you explain why it's done that way? Or is it just accidental oversight?

If it makes sense, I would like to change that behavior, so that the content_origin is only based on min_x when the node is hugging its children. For elements with fixed dimensions, their origin would only be determined by forced_origin_alignment, effectively always being at their border or at the center.

        let forced_origin_alignment = self.layout.forced_origin_alignment.get().get_dim(x);
        let origin_shift = forced_origin_alignment.normalized() * self_size;

        let origin_base = if hug_children { min_x } else { 0.0 };
        self.layout.content_origin.set_dim(x, origin_base - origin_shift);

Also I think that current handling of forced_origin_alignment is problematic. It effectively introduces a position shift of all siblings of given node, resolved at the next auto-layout parent. In order to not do that, we would need to split "postition" into two concept by introducing computed_position property, similar to how we already handle size. Then, the manual-layout would be able to apply shifts to the element without affecting its original set position property. By applying forced_origin_alignment as a shift of that element itself, the change would be guaranteed to be local to that element. In fact, I believe that the computed_position could replace existing content_origin completely for all purposes.

[wdanilo]

Hey, could you show me s minimal code or even a draft picture of what you are trying to achieve? Based on the screenshot I did not understand what should overflow what. I need to understand the use case in order to understand the issue.

[Frizi]

I'm trying to move the dropdown list below the node. The "dropdown" widget node is composed of those layout elements:

The dropdown itself is a child of dropdown_wrapper, which is a manual layout with force size 0. I intentionally want the dropdown to be moved down from that wrapper's position, and to not influcence the elements around it when it is opened. It needs to overflow.

[Frizi]

With the changes applied as in the code snippet above, I'm able to do it. So here's a screenshot that's a bit closer to intended.

Obviously the elements are still not positioned completely correctly, as it is a bit painful to do that at the moment. I really wish there was a way to debug the layout tree. Maybe we could create a debug mode which creates floating non-interactive DOM nodes at their computed size/position, so we could track them with devtools.

[wdanilo]

Obviously the elements are still not positioned completely correctly, as it is a bit painful to do that at the moment. I really wish there was a way to debug the layout tree. Maybe we could create a debug mode which creates floating non-interactive DOM nodes at their computed size/position, so we could track them with devtools.

This is an amazing idea, yes! I was thinking how to do it, but never thought of reusing devtools to do it. This is genius.

[wdanilo]

@farmaazon @Frizi I believe the problems are resolved, can we close it?

[farmaazon]

I won't be sure until we finish widgets ;)

[wdanilo]

Sure, but then we can re-open this thread. For now, we don't see any problems, right? :)