slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
16.94k stars 568 forks source link

set_row_data on a single row invalidates the whole screen #2143

Closed jturcotte closed 7 months ago

jturcotte commented 1 year ago

Noticed when using the software renderer, any single set_row_data on a model used on the screen invalidates the whole region of their root layout for some reason.

To reproduce, overwrite examples/carousel/ui/carousel_demo.slint with this:

struct Data := { bg: brush }

export MainWindow := Window {
    width: 240px;
    height: 160px;
    property<[Data]> model: [
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
        { bg: Colors.lightpink },
    ];

    VerticalLayout {
        x: 5px; y: 5px; width: 230px; height: 150px;

        // Works as expected, only the region of the affected rectangle is marked dirty.
        HorizontalLayout{
            for i in 8:
            r := Rectangle {
                border-width: 1px;
                border-color: black;
                // When this line is commented, clicking won't trigger any redraw at all:
                background: lightblue;
                TouchArea {
                    clicked => {
                        r.background = r.background.darker(0.1);
                    }
                }
            }        
        }

        // Clicking any of those invalidates and redraws the whole VerticalLayout,
        // the same happens if a row of the model is invalidated by set_row_data from Rust.
        HorizontalLayout{
            for data in model:
            Rectangle {
                border-width: 1px;
                border-color: black;
                // When this line is commented, clicking still invalidates the whole layout:
                background: data.bg;
                TouchArea {
                    clicked => {
                        data.bg = data.bg.darker(0.1);
                    }
                }
            }        
        }

    }
}

And apply this debug output patch:

diff --git a/internal/core/item_rendering.rs b/internal/core/item_rendering.rs
index 44d8447b3..e0cb452a3 100644
--- a/internal/core/item_rendering.rs
+++ b/internal/core/item_rendering.rs
@@ -428,6 +428,7 @@ pub fn compute_dirty_regions(&mut self, component: &ComponentRc, origin: Logical
     }

     fn mark_dirty_rect(&mut self, rect: LogicalRect, offset: euclid::Vector2D<Coord, LogicalPx>) {
+        crate::debug_log!("mark_dirty_rect: {:?} {:?}", rect, offset);
         if !rect.is_empty() {
             self.dirty_region = self.dirty_region.union(&rect.translate(offset).to_box2d());
         }
diff --git a/internal/core/software_renderer.rs b/internal/core/software_renderer.rs
index 4935f50d6..39ec292c0 100644
--- a/internal/core/software_renderer.rs
+++ b/internal/core/software_renderer.rs
@@ -183,8 +183,10 @@ pub fn render(&self, buffer: &mut [impl TargetPixel], buffer_stride: usize) {
             }

             let dirty_region = (renderer.dirty_region.to_rect().cast() * factor).round_out().cast();
+            crate::debug_log!("dirty_region: {:?}", dirty_region);

             let to_draw = self.apply_dirty_region(dirty_region, size);
+            crate::debug_log!("to_draw: {:?}", to_draw);

             renderer.combine_clip(
                 (to_draw.cast() / factor).cast(),

Then run using: cargo run -p carousel --no-default-features --features=simulator

Clicking one of the blue rectangles in the top row (the ones not using a model) will output this as expected:

mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 5.0)
dirty_region: Rect(30x75 at (33, 5))
to_draw: Rect(240x160 at (0, 0))

But clicking one of the red rectangles backed by a model of the bottom row I get this, and the whole area of the VerticalLayout is repainted:

mark_dirty_rect: Rect(230.0x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(230.0x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (33.75, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (33.75, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (57.5, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (57.5, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (62.5, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (62.5, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (86.25, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (86.25, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (91.25, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (91.25, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (115.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (115.0, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (120.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (120.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (143.75, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (143.75, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (148.75, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (148.75, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (172.5, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (172.5, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (177.5, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (177.5, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (201.25, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (201.25, 0.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (206.25, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (206.25, 5.0)
mark_dirty_rect: Rect(230.0x75.0 at (0.0, 75.0)) (5.0, 5.0)
mark_dirty_rect: Rect(230.0x75.0 at (0.0, 75.0)) (5.0, 5.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (28.75, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (33.75, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (33.75, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (57.5, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (57.5, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (62.5, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (62.5, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (86.25, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (86.25, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (91.25, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (91.25, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (115.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (115.0, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (120.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (120.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (143.75, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (143.75, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (148.75, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (148.75, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (172.5, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (172.5, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (177.5, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (177.5, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (201.25, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (201.25, 0.0)) (5.0, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (206.25, 80.0)
mark_dirty_rect: Rect(28.75x75.0 at (0.0, 0.0)) (206.25, 80.0)
dirty_region: Rect(230x150 at (5, 5))
to_draw: Rect(240x160 at (0, 0))
ogoffart commented 1 year ago

Changing the data in a model will mark the RepeaterTracker::is_dirty property as dirty. https://github.com/slint-ui/slint/blob/143a1881f1125ad197cf4afe4c5cf74e56f44e06/internal/core/model.rs#L682 And the layout depends on that property, so the layout cache is dirty.

Now, we need the layout to depend on that property because then accessing the layout will have the side effect of updating the properties in the model.

Ideally we shouldn't do that indeed. Instead, each component's model property could be added as a dependency somehow.

This clearly need to be improved.

Workaround for now:

        HorizontalLayout{
            for idx in 8:
            Rectangle {
                property<Data> data: model[idx];
                border-width: 1px;
                border-color: white;
                background: data.bg;
                TouchArea {
                    clicked => {
                        model[idx].bg = data.bg.darker(0.1);
                    }
                }
            }
        }

But I reallize that the model[idx] is still marked dirty way too often

We would also need a mechanism for the [] to only mark itself as dirty when the data changes

jturcotte commented 1 year ago

But I reallize that the model[idx] is still marked dirty way too often

Humm yes, the whole layout isn't affected, but it seems to still invalidates the area covered by all components depending on that model.

I guess I'll try to avoid models and go with something like this for now:

struct Data := {
    bg: brush,
}

Col := Rectangle {
    property<Data> data;
    border-width: 1px;
    border-color: white;
    background: data.bg;
    TouchArea {
        clicked => {
            data.bg = data.bg.darker(0.1);
        }
    }
}

MainWindow := Window {
    width: 240px;
    height: 160px;
    property<Data> data0: { bg: Colors.lightpink};
    property<Data> data1: { bg: Colors.lightpink};
    property<Data> data2: { bg: Colors.lightpink};
    property<Data> data3: { bg: Colors.lightpink};
    property<Data> data4: { bg: Colors.lightpink};
    property<Data> data5: { bg: Colors.lightpink};
    property<Data> data6: { bg: Colors.lightpink};
    property<Data> data7: { bg: Colors.lightpink};

    HorizontalLayout{
        Col { data: data0; }
        Col { data: data1; }
        Col { data: data2; }
        Col { data: data3; }
        Col { data: data4; }
        Col { data: data5; }
        Col { data: data6; }
        Col { data: data7; }
    }
}
ogoffart commented 7 months ago

I think this is no longer the case: This is even tested in https://github.com/slint-ui/slint/blob/09a5c724bf3ba6e60acfb8ca6c6f3b2f14ee696d/api/rs/slint/tests/partial_renderer.rs#L244-L246