Support transforms for each widget #753

Philipp-M · 2024-11-18T21:51:36Z

Checkout this zulip thread for more info/discussion.

~~This is far from the final state, and at this point should more serve for discussion.~~

See the xilem mason or http_cats example, which I have abused to get an initial demo working.

masonry/src/tree_arena.rs

…-tree) and fix transform issues.

…ate, to avoid a lot of boilerplate

…mentation

Philipp-M · 2024-12-07T19:58:00Z

I found a way to avoid the boilerplate on the masonry side, by using WidgetState and adding payload (just the transform for now) to the WidgetPodInner.
If we're having more of those custom global widget attributes, a structure of arrays for these attributes (separate arena?) might make sense (as well as a builder pattern for creating WidgetPods).

But in this state I'm happy enough to merge this (after documentation and tests). As mentioned on zulip, avoiding even more boilerplate on the xilem level, is probably not reasonable, as either the API suffers (by using composition types, which doesn't allow further setting attributes of the underlying views after using a transform wrapper), or requires something like in xilem_web by only using composition types, which would be a big refactor (and a lot more complexity and code)...

Anyway to get a feeling for the added boilerplate on the xilem side, I've implemented transforms now for all views (and masonry also supports it for all widgets). I think it's not too bad and a good trade-off between complexity and boilerplate.

So apart of missing tests and docs, I think this is finished (but I might have missed things, so manual testing for now is appreciated).

PoignardAzur

Sorry for the very late review.

This is not something anyone likes to hear, but I'm pretty confident this needs to be split into multiple PRs.

This PR essentially does three things:

Replace the translation property with a general transform.
Correct the pointer search algorithm to account for OOB sub-children.
Add a way to set transform directly from creation.

I think all three of these changes would be worth their own PR. The second in particular feels like it needs some concepts (declared layout rect vs computed layout rect) to be reified and documented, or at least considered.

PoignardAzur · 2024-12-06T15:03:47Z

masonry/src/testing/harness.rs

-        let widget_rect = self.get_widget(id).ctx().window_layout_rect();
+        let widget_rect = self.get_widget(id).ctx().bounding_rect();


I have a major concern with this PR that shows up in a few places.

It seems the PR conflates two things: the bounding rect as the widget's self-expressed size plus its assigned position, and the bounding rect as the total area that needs to be invalidated if the widget is repainted.

So for example, let's say we have a clickable area, and that area has a child that overflows a thousand pixels right of the area. To click the area, we want to place the cursor in the middle of its self-reported rectangle, and not the rectangle that is the union of the parent and the out-of-bounds child.

Whoops good catch, I think I just didn't pay too much attention to that part.

That's obviously wrong, as my intention was that the bounding_rect is in the window coordinate space. I've corrected this. Where are the other few places though?

Maybe paint_rect which TBH I think we should just remove for now, until we have a concrete use-case for it (e.g. damage regions).
It feels like we drag something along without having use for it currently, and potential to implement/adjust it wrongly without knowing if it's even how we want to do damage regions.
I guess this should otherwise be a (window coordinate) bounding box including the insets, ideally as an OOB in window coordinate space, but then again, it's not clear because it's not really used.

PoignardAzur · 2024-12-11T11:08:54Z

masonry/src/passes/compose.rs

+    let local_translation = state.item.translation + state.item.origin.to_vec2();
+
+    state.item.window_transform =
+        parent_window_transform * state.item.transform.then_translate(local_translation);
+    state.item.window_origin = state.item.window_transform.translation().to_point();


This code first applies the widget's transform, then the translation. Is that the right order? At first I was thinking it should be opposite, but I guess if the transform includes a rotation, you don't want the position vector to be rotated too.

We might want to add a comment to clarify this.

It's the right order, as you can see in the http_cats example (scroll) - at least when we want similar behavior as in CSS.
I've renamed translation in the last commit to scroll_translation so that it's more obvious what happens here, and I don't think translation is used in any other context, but I can of course revert that when there's arguments for it. Also added a comment.

PoignardAzur · 2024-12-11T11:23:02Z

masonry/src/widget/button.rs

-    pub fn new(text: impl Into<ArcStr>) -> Button {
+    pub fn new(text: impl Into<ArcStr>) -> Self {


Not a dealbreaker, but I think this PR has a few too many drive-by fixes given its already large size.

PoignardAzur · 2024-12-11T11:37:10Z

masonry/src/widget/widget.rs

+    if widget.ctx.widget_state.bounding_rect.contains(pos) {
+        let local_pos = widget.ctx().widget_state.window_transform.inverse() * pos;
+
+        if Some(false) == widget.ctx.clip_path().map(|clip| clip.contains(local_pos)) {
+            return None;
+        }
+
+        // Assumes `Self::children_ids` is in increasing "z-order", picking the last child in case
+        // of overlapping children.
+        for child_id in widget.children_ids().iter().rev() {
+            let child_ref = widget.ctx.get(*child_id);
+            if let Some(child) = child_ref.widget.find_widget_at_pos(child_ref.ctx, pos) {
+                return Some(child);
+            }
+        }
+        if !widget.ctx.is_stashed()
+            && widget.ctx.accepts_pointer_interaction()
+            && widget.ctx.size().to_rect().contains(local_pos)
        {
-            return Some(child);
+            Some(*widget)
+        } else {
+            None
        }
+    } else {
+        None
    }


This is a core change in the PR, and it's one with a lot of semantic importance, so I'm giving it extra scrutiny. Some notes:

This code has a lot of rightward drift that could be avoided with early returns.

Again the difference between "declared layout rect" and "bounding rect of self and children" shows up. I think your code handles it correctly, but it does so implicitly. I think that distinction should be documented in a few places and given special attention in this function.

I think is_stashed should be considered before the children. A stashed widget is never going to have un-stashed children. accepts_pointer_interaction is probably fine here, if we want to behave like pointer-events: none in the DOM, but that should be documented.

This code has a lot of rightward drift that could be avoided with early returns.

More than you have listed in the other points? I only see that is_stashed can be moved up (I've fixed this in the latest commit).

Again the difference between "declared layout rect" and "bounding rect of self and children" shows up. I think your code handles it correctly, but it does so implicitly. I think that distinction should be documented in a few places and given special attention in this function.

Does it? the bounding rect is only used to filter out early as optimization, nothing else really. Otherwise I think it's documented what bounding_rect is, do you have more concrete suggestions?

accepts_pointer_interaction is probably fine here, if we want to behave like pointer-events: none in the DOM, but that should be documented.

I agree that things should be more documented (and I've documented this in Widget::find_widget_at_pos now), but it's basically the same behavior as before with get_child_at_pos (which wasn't documented).

PoignardAzur · 2024-12-11T11:40:42Z

masonry/src/widget/widget_pod.rs

+    /// Create a new widget pod with a custom transform.
+    pub fn new_with_transform(inner: W, transform: Affine) -> WidgetPod<W> {
+        Self::new_with_id_and_transform(inner, WidgetId::next(), transform)
+    }


Okay, so I get why the PR does it this way, but I'm somewhat uncomfortable with adding a new constructor and all the special-casing that comes with it for a single attribute.

I don't know how else to do it, ideally it should be part of the styling refactor, but I don't want to block on that.

Yes I don't think this should be a long term solution, what we could also do instead is something like this:

/// Create a new widget pod with fixed id. pub fn new_with_id(inner: impl Into<CreatedWidget<W>>, id: WidgetId) -> WidgetPod<W> { Self::new_with_id_and_transform(inner, id, Affine::IDENTITY) }

and add the relevant From impls, like e.g. this:

impl<W: Widget> From<W> for CreatedWidget<W>{..} impl<W: Widget> From<(W, Affine)> for CreatedWidget<W>{..}

for general attributes.
As mentioned somewhere else we could also introduce a builder type for creating widgets, maybe by just using CreatedWidget for this?

PoignardAzur · 2024-12-11T11:54:02Z

masonry/src/passes/compose.rs

+            parent_state.bounding_rect = parent_state.bounding_rect.union(state.item.bounding_rect);
            parent_state.merge_up(state.item);


By the way, this is needlessly pessimistic when the parent widget has a clipping rect.

You're right for at least the use case of intersecting pointer events and possibly doing some kind of damage regions (but I think this is basically the use case of bounding rects/boxes here). I've used an intersection of the (transformed) clip path and the child bounding rect instead here. There's for sure further optimization potential, but I think that should be probably done in follow-up PRs when it's more clear what shape clip_path will be.

* Consider clip_path when building the bounding rect from the children * Fix pointer issue in the test harness to click a widget

Philipp-M · 2024-12-20T21:02:56Z

This is not something anyone likes to hear, but I'm pretty confident this needs to be split into multiple PRs.

I'm actually not so confident about that... If the PR would be much more complex, and easier to split into changes that could stand for themselves, I would've done so, but as it stands a lot of the code is intertwined, a few parts may or may not be isolated, but then I'm not really sure whether it's worth it to split these up. E.g. transforms without adjusted pointer-intersection is not correct when isolated.

I still need to ideally write some tests (any ideas what may be good?), probably document further, and revert at least the http_cats example (but right now it's good to see that scrolling is handled correctly).

Philipp-M mentioned this pull request Nov 18, 2024

Add zstack view to compose views on top of each other #737

Open

2 tasks

Philipp-M force-pushed the transforms branch from 7da5c16 to b97a9e9 Compare November 23, 2024 00:16

KGrewal1 mentioned this pull request Nov 27, 2024

Tree arena #752

Merged

PoignardAzur reviewed Nov 28, 2024

View reviewed changes

masonry/src/tree_arena.rs Outdated Show resolved Hide resolved

Philipp-M force-pushed the transforms branch from 2601177 to 21499ab Compare December 5, 2024 17:26

Philipp-M added 13 commits December 7, 2024 16:47

Initial dirty hack to get transforms per widget working

3136ecb

Handle pointer-events correctly, with a BVH (hieararchy is the widget…

1606302

…-tree) and fix transform issues.

Fix Text pointer events in TextArea when a transform is applied.

fd8cba1

Remove unused code

61a0b13

Make find_widget_at_pos behavior more correct

7f0b1a7

Fix stack overflow in tests

e9d09b7

Fix get_ime_area()

8b6f311

Replace window_layout_rect with bbox (and thus fix accessibility).

151d0b8

Rename bbox to bounding_rect as it's more appropriate in 2D space

e2ad4de

Revert unnecessary changes

045af4d

Factor the widgets transform out of the widget trait to the widget st…

d8126e3

…ate, to avoid a lot of boilerplate

Move Transformable trait below the view modules, and add basic docu…

e73c9ce

…mentation

xilem: Wire up all views with transforms

4b4969e

Philipp-M force-pushed the transforms branch from 21499ab to 4b4969e Compare December 7, 2024 19:57

Fix clippy lints

90ae403

PoignardAzur reviewed Dec 11, 2024

View reviewed changes

* Rename translation to scroll translation

961caf8

* Consider clip_path when building the bounding rect from the children * Fix pointer issue in the test harness to click a widget

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support transforms for each widget #753

Support transforms for each widget #753

Philipp-M commented Nov 18, 2024 •

edited

Loading

Philipp-M commented Dec 7, 2024

PoignardAzur left a comment

PoignardAzur Dec 6, 2024

Philipp-M Dec 20, 2024

PoignardAzur Dec 11, 2024

Philipp-M Dec 20, 2024

PoignardAzur Dec 11, 2024

PoignardAzur Dec 11, 2024

Philipp-M Dec 20, 2024

PoignardAzur Dec 11, 2024

Philipp-M Dec 20, 2024

PoignardAzur Dec 11, 2024

Philipp-M Dec 20, 2024

Philipp-M commented Dec 20, 2024

		let widget_rect = self.get_widget(id).ctx().window_layout_rect();
		let widget_rect = self.get_widget(id).ctx().bounding_rect();

		pub fn new(text: impl Into<ArcStr>) -> Button {
		pub fn new(text: impl Into<ArcStr>) -> Self {

		parent_state.bounding_rect = parent_state.bounding_rect.union(state.item.bounding_rect);
		parent_state.merge_up(state.item);

Support transforms for each widget #753

Are you sure you want to change the base?

Support transforms for each widget #753

Conversation

Philipp-M commented Nov 18, 2024 • edited Loading

Philipp-M commented Dec 7, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented Dec 20, 2024

Philipp-M commented Nov 18, 2024 •

edited

Loading