Draft: Rework item geometry #3115

Guiguiprim · 2023-07-19T14:03:48Z

Closes #1932

I'm creating this PR to share where I'm at. But it is still buggy...

The following for example does not work:

import { HorizontalBox, GroupBox, TextEdit } from "std-widgets.slint";
export component App inherits Window {
  TextEdit {
    text: @tr("This is our TextEdit widget.");
    wrap: word-wrap;
    width: parent.width;
    height: parent.height;
    x: 0;
    y: 0;
  }
}

From a debug log using the interpreter after the flickable rework we have:

Output


            i-flickable-5 := Flickable {
               height: (i-scroll-view-4.height - 14px);
               interactive: false/*const*/;
               viewport-height: /../;
               viewport-x: 0px;
               viewport-y: 0px;
               width: (i-scroll-view-4.width - 14px);
               x: 2px/*const*/;
               y: 2px/*const*/;
               i-flickable-viewport-6 := Empty {
                  height: ;
                  height <=> i-flickable-5.viewport-height;
                  width: ;
                  width <=> i-flickable-5.viewport-width;
                  x: ;
                  x <=> i-flickable-5.viewport-x;
                  y: ;
                  y <=> i-flickable-5.viewport-y;
                  i-text-input-7 := TextInput {
                     color: if ((root-1.empty-2-state == 1)) { Palette-34.text-disabled } else { Palette-34.text-primary };
                     cursor-position-changed: /../;
                     edited: { root-1.empty-2-edited(i-text-input-7.text, ); };
                     enabled: true;
                     font-size: ((({ font-weight: (400/* as int */), font-size: 1.0766rem,  }/* as TextStyle */).font-size * GetWindowDefaultFontSize())/* as length */);
                     font-weight: ({ font-weight: (400/* as int */), font-size: 1.0766rem,  }/* as TextStyle */).font-weight/*const*/;
                     height: i-flickable-5.viewport-height;
                     read-only: false;
                     selection-background-color: (((4278221012/* as color */)/* as brush */)/* as color */)/*const*/;
                     selection-foreground-color: (i-text-input-7.color/* as color */);
                     single-line: false/*const*/;
                     text: Translate("This is our TextEdit widget.", "App", "", [], 1, "", );
                     text-cursor-width: 1px/*const*/;
                     width: i-flickable-5.viewport-width;
                     wrap: TextWrap.word-wrap;
                  }
               }
            }

And at the end:

Output


   i-text-input-7-preferred-height: ImplicitLayoutInfo(Vertical)((Weak), ).preferred;
   i-text-input-7-preferred-width: ImplicitLayoutInfo(Horizontal)((Weak), ).preferred;
   i-text-input-7-x: ((i-flickable-5.viewport-width - 0px) / 2);
   i-text-input-7-y: ((i-flickable-5.viewport-height - 0px) / 2);
   /.../
        i-flickable-5 := Flickable {
         interactive: false/*const*/;
         viewport-height: /../;
         viewport-x: 0px;
         viewport-y: 0px;
         i-flickable-viewport-6 := Empty {
            i-text-input-7 := TextInput {
               color: if ((root-1.empty-2-state == 1)) { Palette-34.text-disabled } else { Palette-34.text-primary };
               cursor-position-changed: /../;
               edited: { root-1.empty-2-edited(i-text-input-7.text, ); };
               enabled: true;
               font-size: ((({ font-size: 1.0766rem, font-weight: (400/* as int */),  }/* as TextStyle */).font-size * GetWindowDefaultFontSize())/* as length */);
               font-weight: ({ font-size: 1.0766rem, font-weight: (400/* as int */),  }/* as TextStyle */).font-weight/*const*/;
               read-only: false;
               selection-background-color: (((4278221012/* as color */)/* as brush */)/* as color */)/*const*/;
               selection-foreground-color: (i-text-input-7.color/* as color */);
               single-line: false/*const*/;
               text: Translate("This is our TextEdit widget.", "App", "", [], 1, "", );
               text-cursor-width: 1px/*const*/;
               wrap: TextWrap.word-wrap;
               geometry GeometryProps { x: root-1.i-text-input-7-x, y: root-1.i-text-input-7-y, width: i-text-input-7.width, height: i-text-input-7.height }
            }
            geometry GeometryProps { x: i-flickable-5.viewport-x, y: i-flickable-5.viewport-y, width: i-flickable-5.viewport-width, height: i-flickable-5.viewport-height }
         }
         geometry GeometryProps { x: root-1.i-flickable-5-x, y: root-1.i-flickable-5-y, width: root-1.empty-2-visible-width, height: root-1.empty-2-visible-height }
      }

Clearly things goes wrong somewhere:

i-text-input-7 somehow get center to the viewport
i-text-input-7 dimension are not set (should be bind to viewport)

So still some debugging to do, but I'm tacking a break.

Guiguiprim · 2023-07-19T14:24:57Z

I know that i think about it, maybe trying to fix the tests could have help me find the issues... 😓

ogoffart

I haven't look at details yet, but it looks good from a first glance

ogoffart · 2023-07-19T14:28:47Z

internal/backends/qt/qt_widgets.rs

@@ -89,8 +90,9 @@ macro_rules! fn_render {
                backend.draw_cached_pixmap(
                    item_rc,
                    &|callback| {
-                        let width = self.width().get() * $dpr;
-                        let height = self.height().get() * $dpr;
+                        let geo = item_rc.geometry();


Ideally here we should get the size from the call to render that has it instead of doing that.
(because each call to geometry add possibly 4 property dependency we don't need)

But we can leave that as an action item for later.

Guiguiprim · 2023-07-19T14:45:18Z

but it looks good from a first glance

Most commits are from you 😝

But thanks anyway.

ogoffart · 2023-07-19T14:46:39Z

but it looks good from a first glance

Most commits are from you stuck_out_tongue_closed_eyes

Yes, I was referring to the commit you added.

Guiguiprim · 2023-07-19T15:14:56Z

OK so following breaking tests, I found an easier case to study (from slint::partial_render::simple):

export component Ui inherits Window {
    in property <color> c: yellow;
    background: black;
    Rectangle {
        x: 1phx;
        y: 80phx;
        width: 15phx;
        height: 17phx;
        background: red;
    }
    Rectangle {
        x: 10phx;
        y: 19phx;
        Rectangle {
            x: 5phx;
            y: 80phx;
            width: 12phx;
            height: 13phx;
            background: c;
        }
        Rectangle {
            x: 50phx;
            y: 8phx;
            width: 15phx;
            height: 17phx;
            background: c;
        }
    }
}

gives

Output


[internal/interpreter/dynamic_component.rs:771] &*component.root_element.borrow() = root-1 := WindowItem {
   property c;
   property rectangle-2-height;
   property rectangle-2-width;
   property rectangle-2-x;
   property rectangle-2-y;
   property rectangle-4-height;
   property rectangle-4-width;
   property rectangle-4-x;
   property rectangle-4-y;
   property rectangle-5-height;
   property rectangle-5-width;
   property rectangle-5-x;
   property rectangle-5-y;
   property x;
   property y;
   background: ((4278190080/* as color */)/* as brush */)/*const*/;
   c: (4294967040/* as color */)/*const*/;
   rectangle-2-height: (17phx / GetWindowScaleFactor());
   rectangle-2-width: (15phx / GetWindowScaleFactor());
   rectangle-2-x: (1phx / GetWindowScaleFactor());
   rectangle-2-y: (80phx / GetWindowScaleFactor());
   rectangle-4-height: (13phx / GetWindowScaleFactor());
   rectangle-4-width: (12phx / GetWindowScaleFactor());
   rectangle-4-x: (5phx / GetWindowScaleFactor());
   rectangle-4-y: (80phx / GetWindowScaleFactor());
   rectangle-5-height: (17phx / GetWindowScaleFactor());
   rectangle-5-width: (15phx / GetWindowScaleFactor());
   rectangle-5-x: (50phx / GetWindowScaleFactor());
   rectangle-5-y: (8phx / GetWindowScaleFactor());
   title: "Slint Window"/*const*/;
   rectangle-2 := Rectangle {
      background: ((4294901760/* as color */)/* as brush */)/*const*/;
      geometry GeometryProps { x: root-1.rectangle-2-x, y: root-1.rectangle-2-y, width: root-1.rectangle-2-width, height: root-1.rectangle-2-height }
   }
   rectangle-4 := Rectangle {
      background: (root-1.c/* as brush */);
      geometry GeometryProps { x: root-1.rectangle-4-x, y: root-1.rectangle-4-y, width: root-1.rectangle-4-width, height: root-1.rectangle-4-height }
   }
   rectangle-5 := Rectangle {
      background: (root-1.c/* as brush */);
      geometry GeometryProps { x: root-1.rectangle-5-x, y: root-1.rectangle-5-y, width: root-1.rectangle-5-width, height: root-1.rectangle-5-height }
   }
   geometry GeometryProps { x: root-1.x, y: root-1.y, width: root-1.width, height: root-1.height }
}

where we should have rectangle-4-x: (15phx / GetWindowScaleFactor());

And at some point with constant evaluation we should hope for something as simple as (in the spirit at least):

Output


[internal/interpreter/dynamic_component.rs:771] &*component.root_element.borrow() = root-1 := WindowItem {
   property c;
   property x;
   property y;
   background: ((4278190080/* as color */)/* as brush */)/*const*/;
   c: (4294967040/* as color */)/*const*/;
   title: "Slint Window"/*const*/;
   rectangle-2 := Rectangle {
      background: ((4294901760/* as color */)/* as brush */)/*const*/;
      geometry GeometryProps { x: (1phx / GetWindowScaleFactor()), y: (80phx / GetWindowScaleFactor()), width: (15phx / GetWindowScaleFactor()), height: (17phx / GetWindowScaleFactor()) }
   }
   rectangle-4 := Rectangle {
      background: (root-1.c/* as brush */);
      geometry GeometryProps { x: (15phx / GetWindowScaleFactor()), y: (99phx / GetWindowScaleFactor()), width: (12phx / GetWindowScaleFactor()), height: (13phx / GetWindowScaleFactor()) }
   }
   rectangle-5 := Rectangle {
      background: (root-1.c/* as brush */);
      geometry GeometryProps { x: (60phx / GetWindowScaleFactor()), y: (27phx / GetWindowScaleFactor()), width: (15phx / GetWindowScaleFactor()), height: (17phx / GetWindowScaleFactor()) }
   }
   geometry GeometryProps { x: root-1.x, y: root-1.y, width: root-1.width, height: root-1.height }
}

ogoffart · 2023-07-19T15:16:38Z

Do you know if this was caused by one of my commit, or caused by your change?

Guiguiprim · 2023-07-19T15:22:58Z

Do you know if this was caused by one of my commit, or caused by your change?

I'm only sure it was not cause by the first commit (I though is was a flickable issue at first).

I didn't try a bijection on other commits has I not sure it would even build/work.

Guiguiprim · 2023-07-19T15:23:36Z

I guess we are missing a step needed by the addition of GeometryProp

ogoffart · 2023-07-19T15:25:53Z

I see the problem: the optimize_useless_rectangles pass used to not optimize any rectangle that had a x and y property
But now that xand y are not on the rectangle, this means it is bing optimized.

So two options:

check if the x and y geometry from that rectangle are unset or set to zero in the can_optimize
Optimize these item anyway, but create a new property which is parent.x + x and put that property in the geometry

Option 1 is status qui as it used to be. But option 2 is great because it allow to optimize more now that we decouple the item geometry from their language properties.

Guiguiprim · 2023-07-19T15:26:49Z

It does look OK on your last commit:

[internal/interpreter/dynamic_component.rs:771] &*component.root_element.borrow() = root-1 := WindowItem {
   property<color> c;
   property<length> x;
   property<length> y;
   background: ((4278190080/* as color */)/* as brush */)/*const*/;
   c: (4294967040/* as color */)/*const*/;
   title: "Slint Window"/*const*/;
   rectangle-2 := Rectangle {
      background: ((4294901760/* as color */)/* as brush */)/*const*/;
      height: (17phx / GetWindowScaleFactor());
      width: (15phx / GetWindowScaleFactor());
      x: (1phx / GetWindowScaleFactor());
      y: (80phx / GetWindowScaleFactor());
   }
   rectangle-3 := Empty {
      height: root-1.height;
      width: root-1.width;
      x: (10phx / GetWindowScaleFactor());
      y: (19phx / GetWindowScaleFactor());
      rectangle-4 := Rectangle {
         background: (root-1.c/* as brush */);
         height: (13phx / GetWindowScaleFactor());
         width: (12phx / GetWindowScaleFactor());
         x: (5phx / GetWindowScaleFactor());
         y: (80phx / GetWindowScaleFactor());
      }
      rectangle-5 := Rectangle {
         background: (root-1.c/* as brush */);
         height: (17phx / GetWindowScaleFactor());
         width: (15phx / GetWindowScaleFactor());
         x: (50phx / GetWindowScaleFactor());
         y: (8phx / GetWindowScaleFactor());
      }
   }
}

EDIT: and more importantly the result is visibly OK

Guiguiprim · 2023-07-19T15:30:25Z

By the way Empty does not have any properties left, so I was wondering about compiling it away, but I just checked there is still a CacheRenderingData in there so maybe not.

ogoffart · 2023-07-19T15:41:25Z

By the way Empty does not have any properties left, so I was wondering about compiling it away, but I just checked there is still a CacheRenderingData in there so maybe not.

Ideally we can remove them all indeed. (in the optimize_useless_rectangles pass)

CacheRenderingData is another thing that was maybe not such a good idea to put there. and we may consider removing.

Guiguiprim · 2023-07-19T16:27:01Z

To better get familiarize with the code I tried solution 1 first (I want to do solution 2), it fix the rectangle bug.

I will try to do solution 2 tomorrow.

The TextEdit bug is still there, so not done...

Instead: add the viewport propety directly in the Flickable Simplifies the compiler's code generation a bit

(unused yet)

We need to account for the repeater count in the item sub-range

The WindowItem did not have a x or y before, so these property were unused for it's geometry(), but now it is used in the new item_geometry on the ComponentVTable. So make sure we don't initialize them

I would love to add a clippy disallowed method for this, but it's a generic type alias.

Guiguiprim · 2023-07-25T09:55:25Z

I'm moving forward (almost done) for the solution 2 for fixing the rectangle optimization.

But at the time being, it ends up generating more properties, and I feel like more math optimization should be done afterward:

   rectangle-3-x: (10phx / GetWindowScaleFactor());
   rectangle-4-x: (5phx / GetWindowScaleFactor());
   rectangle-4-x-optimized-1: (root-1.rectangle-3-x + root-1.rectangle-4-x);

We could hope for only: rectangle-4-x-optimized-1: (15phx / GetWindowScaleFactor());

ogoffart · 2023-07-25T10:56:08Z

I feel like more math optimization should be done afterward:

Unfortunately, we can't do inlining with the current expression_tree::Expression because property references to inner value are not representable.

That's why there is only const propagation pass and then a inline_expression pass that operate on the llr.

Of course there is room form improvements.

I have the feeling the use of "phx" is not so frequent, so this particular example might not be representative of real code.

But at the time being, it ends up generating more properties

Is it really more properties? Doesn't these new property used to b in a rectangle anyway?

Guiguiprim · 2023-07-25T11:40:42Z

Is it really more properties?

Not really, it's just more than it could be.

I have the feeling the use of "phx" is not so frequent, so this particular example might not be representative of real code.

I just use one of the test case, you are probably right

Guiguiprim · 2023-07-25T11:49:52Z

because property references to inner value are not representable.

I'm not sure what you mean, but I trust you :p

ogoffart · 2023-09-12T16:59:57Z

Thanks for your work. I have taken your commit and put them in #3452

Guiguiprim · 2023-09-13T14:22:14Z

Sorry for abandoning my PR, I wasn't able to make time to finish it.

Anyway glad to see that you managed to push it to the finish line !

ogoffart reviewed Jul 19, 2023

View reviewed changes

Guiguiprim force-pushed the item_geometry branch from 7d06d29 to 0aae496 Compare July 19, 2023 16:25

ogoffart and others added 14 commits July 24, 2023 17:55

Flickable: don't make the viewport special

582a7a3

Instead: add the viewport propety directly in the Flickable Simplifies the compiler's code generation a bit

WIP: Add item_geometry to the ComponentVTable

f6383fa

(unused yet)

Use the item_geometry from ComponentVTable

c15ddc4

Fix computation of the item sub range

7fcb123

We need to account for the repeater count in the item sub-range

Fix generation of geometry prop for some primitives

b6fe5c6

More use of item_geometry

1da9b0d

Fix PopupWindow positioning

f517cd3

The WindowItem did not have a x or y before, so these property were unused for it's geometry(), but now it is used in the new item_geometry on the ComponentVTable. So make sure we don't initialize them

Remove the geometry on ItemVTable

1d60a00

Remove duplicated geometry properties from items

5444cdd

include geometries in Element debug print

a0c51e3

fix item_tree test compilation

713127c

fix resolve_native_classes pass

f7dce18

fix lower_shadow: correctly create ElementRc

238b2b2

I would love to add a clippy disallowed method for this, but it's a generic type alias.

WIP Take new geometry into account in useless_rectangle

aad9133

Guiguiprim force-pushed the item_geometry branch from 1e8bc2e to 7035e5d Compare July 25, 2023 09:52

WIP Take new geometry into account in useless_rectangle

930be7e

Guiguiprim force-pushed the item_geometry branch from 7035e5d to 930be7e Compare July 25, 2023 12:38

ogoffart mentioned this pull request Sep 12, 2023

Move geometry outside of items #3452

Merged

ogoffart closed this Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Rework item geometry #3115

Draft: Rework item geometry #3115

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 19, 2023

ogoffart left a comment

ogoffart Jul 19, 2023

Guiguiprim commented Jul 19, 2023 •

edited

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023 •

edited

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 19, 2023

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

Guiguiprim commented Jul 25, 2023

ogoffart commented Jul 25, 2023

Guiguiprim commented Jul 25, 2023 •

edited

Guiguiprim commented Jul 25, 2023

ogoffart commented Sep 12, 2023 •

edited

Guiguiprim commented Sep 13, 2023

Draft: Rework item geometry #3115

Draft: Rework item geometry #3115

Conversation

Guiguiprim commented Jul 19, 2023 • edited

Guiguiprim commented Jul 19, 2023

ogoffart left a comment

Choose a reason for hiding this comment

ogoffart Jul 19, 2023

Choose a reason for hiding this comment

Guiguiprim commented Jul 19, 2023 • edited

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023 • edited

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023 • edited

Guiguiprim commented Jul 19, 2023

ogoffart commented Jul 19, 2023

Guiguiprim commented Jul 19, 2023

Guiguiprim commented Jul 25, 2023

ogoffart commented Jul 25, 2023

Guiguiprim commented Jul 25, 2023 • edited

Guiguiprim commented Jul 25, 2023

ogoffart commented Sep 12, 2023 • edited

Guiguiprim commented Sep 13, 2023

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 19, 2023 •

edited

Guiguiprim commented Jul 25, 2023 •

edited

ogoffart commented Sep 12, 2023 •

edited