masonry: reimplement `Widget::get_child_at_pos` #565

tomcur · 2024-08-29T12:02:15Z

This is a stab at resolving code comment

Lines 192 to 199 in ac95f25

    
           // TODO - Use Widget::get_child_at_pos method 
        
           if let Some(child) = innermost_widget.children().into_iter().find(|child| { 
        
               !child.widget.skip_pointer() && child.state().window_layout_rect().contains(pos) 
        
           }) { 
        
               innermost_widget = child; 
        
           } else { 
        
               break; 
        
           }

This may or may not be a good resolution. In this implementation Widget::get_child_at_pos gets the signature

get_child_at_pos<'w>(&self, children: &[WidgetRef<'w, dyn Widget>], pos: Point) -> Option<WidgetRef<'w, dyn Widget>>

with children a slice of WidgetRef children in the same order as returned by Widget::children_ids. The default Widget::get_child_at_pos implementation assumes children are in increasing "z-order", picking the last child in case of overlapping children. If that assumption is kept, it should be documented, but perhaps a better solution is needed.

waywardmonkeys

I don't know about the overall strategy that is desired for this, but I'm definitely interested in seeing this feature be underway.

Would be good to have a couple of tests as well though, including checking skip_pointer and some other edge cases.

masonry/src/widget/widget_ref.rs

PoignardAzur · 2024-08-30T10:48:07Z

The default Widget::get_child_at_pos implementation assumes children are in increasing "z-order", picking the last child in case of overlapping children. If that assumption is kept, it should be documented, but perhaps a better solution is needed.

That assumption is likely to remain, since it's how children are painted right now, and the cursor should general pick the widget whose pixels are "visible" under the mouse.

I'm not crazy about the approach of passing an array of WidgetRef, though. I would lean towards passing a nont-mutable LifecycleCtx (soon to be renamed to UpdateCtx) instead. In the cases where that method would be useful (eg a very large container), you want to avoid any kind of O(N) work.

tomcur · 2024-08-30T12:03:23Z

I'm not crazy about the approach of passing an array of WidgetRef, though. I would lean towards passing a nont-mutable LifecycleCtx (soon to be renamed to UpdateCtx) instead. In the cases where that method would be useful (eg a very large container), you want to avoid any kind of O(N) work.

That's a good point.

Currently LifeCycleCtx (UpdateCtx) requires mutable-everything:

xilem/masonry/src/contexts.rs

Lines 65 to 73 in 6c49516

    
           /// A context provided to the [`lifecycle`] method on widgets. 
        
           /// 
        
           /// [`lifecycle`]: Widget::lifecycle 
        
           pub struct LifeCycleCtx<'a> { 
        
               pub(crate) global_state: &'a mut RenderRootState, 
        
               pub(crate) widget_state: &'a mut WidgetState, 
        
               pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>, 
        
               pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>, 
        
           }

That would mean WidgetRef::find_widget_at_pos would need mutable references to construct LifeCycleCtx, but it by design holds shared references. Perhaps a new context type with shared references is required? Alternatively, WidgetMut could get the find_widget_at_pos method. Though querying for a child seems like it should be possible through a shared ref.

waywardmonkeys · 2024-08-30T12:11:45Z

It would be nice if this supported the sort of use case where one might want an acceleration structure (such as a quad tree) for a widget-canvas with a lot of widgets.

tomcur · 2024-08-30T12:16:09Z

It would be nice if this supported the sort of use case where one might want an acceleration structure (such as a quad tree) for a widget-canvas with a lot of widgets.

Indeed, that's a good use-case. If, as @PoignardAzur suggested, a cheaply-constructable context type is passed to the widget, the widget can choose to either do a linear search through its children (for example, sticking to the default behavior), or it can use whatever domain knowledge it has (for example, your use-case of a canvas with a quad tree for searching).

PoignardAzur · 2024-08-30T12:22:40Z

That would mean WidgetRef::find_widget_at_pos would need mutable references to construct LifeCycleCtx, but it by design holds shared references.

Oh, that's right. That makes things more awkward.

Perhaps a new context type with shared references is required?

Probably, yes, but I'm not happy about adding yet another context type.

tomcur · 2024-08-30T15:52:18Z

Instead of creating a new context type, WidgetRef can be provided to the widget. The signature then becomes:

fn get_child_at_pos<'w>(
    &self,
    widget_ref: WidgetRef<'w, dyn Widget>,
    pos: Point,
) -> Option<WidgetRef<'w, dyn Widget>>

I've pushed a commit with this change to illustrate. It is a bit circular, though: WidgetRef carries a reference to the widget it is now calling with a reference to itself.

Going the context route, the context could look as follows. Some of the methods on WidgetRef (like WidgetRef::find_widget_by_id) could be moved/duplicated to QueryCtx.

/// A context provided to widget methods that need read-only (shared) access.
pub struct QueryCtx<'a> {
    pub(crate) global_state: &'a RenderRootState,
    pub(crate) widget_state: &'a WidgetState,
    pub(crate) widget_state_children: ArenaRefChildren<'a, WidgetState>,
    pub(crate) widget_children: ArenaRefChildren<'a, Box<dyn Widget>>,
}

Note QueryCtx itself would not be sufficient to iterate over child WidgetRef, as a reference to the widget is needed to call Widget::children_ids. This is no problem inside Widget::get_child_at_pos, of course.

xilem/masonry/src/widget/widget_ref.rs

Lines 109 to 112 in 6c49516

    
           pub fn children(&self) -> SmallVec<[WidgetRef<'w, dyn Widget>; 16]> { 
        
               let parent_id = self.widget_state.id.to_raw(); 
        
               self.widget 
        
                   .children_ids()

PoignardAzur · 2024-08-30T22:49:30Z

Sounds good. Fair warning, I won't have time to look at this over the weekend, but I'll have a look on Monday.

tomcur · 2024-09-01T18:20:59Z

A context type plus a Widget is similar to a WidgetRef (a WidgetRef can be constructed from those two), with the option of having additional fields. In the example above, QueryCtx has a global_state: &RenderRootState field like the other context types (though shared, not mutable), so it can have methods like QueryCtx::has_pointer_capture.

Unlike WidgetRef, context types don't hold references to the Widget itself, so calling a Widget with a QueryCtx doesn't have that somewhat inelegant circular referencing.

Existing Widget methods requiring state receive it through mut ref context types, using a shared ref QueryCtx is perhaps less surprising than using a WidgetRef. It also prevents the Widget API from needing to know about WidgetRef at all.

DJMcNab

Some "drive-by" thoughts, pending Olivier's review.

masonry/src/widget/widget.rs

PoignardAzur

Something that occurs to me reading this is we probably want to rename find_widget_at_pos to find_widget_under_cursor across the crate. It's of scope for now though.

I think the WidgetRef based API is too awkward. I'd rather have one based on a new QueryCtx type.

masonry/src/widget/widget.rs

tomcur · 2024-09-10T14:39:32Z

Rebased onto main, and added commits. One introduces QueryCtx, replacing WidgetRef in the method call, but otherwise introduces minimal changes.

The commit after explores a more substantial change. WidgetRef and WidgetMut are conceptually similar. WidgetMut is a struct of a Widget and a MutateCtx. For symmetry, in this second commit WidgetRef becomes a struct of a Widget and a QueryCtx.

Some thoughts:

MutateCtx::get_mut was already used to get a WidgetMut, I've added QueryCtx::get to get a WidgetRef.
QueryCtx::get should perhaps take a &WidgetPod, like how MutateCtx::get_mut takes a &mut WidgetPod. It currently takes a WidgetId, because Widget::children_ids returns just that: ids, not pods. Maybe we can construct WidgetPod in the default Widget::get_child_at_pos method implementation by assuming WidgetPodInner::Inserted.
QueryCtx having access to global state, like all other context types, has some ramifications. For example, to construct a WidgetRef containing a QueryCtx, having access to a widget's state is no longer enough, and means WidgetArena cannot construct a WidgetRef by itself anymore. In that commit I've moved the method for getting widget refs to RenderRoot::get_widget, which already had a method for getting a WidgetMut through RenderRoot::edit_widget.

(Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/.60WidgetRef.60.20and.20.60WidgetMut.60)

masonry/src/widget/widget.rs

tomcur · 2024-09-10T14:47:53Z

masonry/src/widget/widget_ref.rs

+                // Get child from the widget arena to bind it to the arena's lifetime. Is there a
+                // way around this?
+                let child = root.get_widget(child.id()).unwrap();


There are lifetime issues here: child widgets fetched from Widget::get_child_at_pos can't be returned from the function because they don't live long enough. To get around this, the render root is passed in to refetch widgets with a longer lifetime from the global widget arena, but maybe there's a more elegant solution.

Yeah, this exact kind of lifetime issue (when you want to return a borrow to something pointed to by a local, but rustc thinks you're returning a borrow to the local) has bitten me before.

I'm not sure there's an elegant solution, but finding one is a mid-high priority.

PoignardAzur

LGTM at a skim. Will look more in-depth later.

PoignardAzur · 2024-09-11T12:23:19Z

masonry/src/widget/widget_ref.rs

+                // Get child from the widget arena to bind it to the arena's lifetime. Is there a
+                // way around this?
+                let child = root.get_widget(child.id()).unwrap();


Yeah, this exact kind of lifetime issue (when you want to return a borrow to something pointed to by a local, but rustc thinks you're returning a borrow to the local) has bitten me before.

I'm not sure there's an elegant solution, but finding one is a mid-high priority.

masonry/src/widget/widget.rs

Didn't mean to approve.

PoignardAzur · 2024-09-11T15:40:38Z

masonry/src/widget/widget_ref.rs

            if let Some(clip) = innermost_widget.state().clip {
-                let relative_pos = pos.to_vec2() - innermost_widget.state().window_origin.to_vec2();
-                // If the widget has a clip, the point must be inside
-                // else we don't iterate over children.
-                if !clip.contains(relative_pos.to_point()) {
+                // If the widget has a clip, the point must be inside, else we don't iterate over
+                // children.
+                let pos = pos - innermost_widget.state().window_origin().to_vec2();
+                if !clip.contains(pos) {
                    break;
                }
            }


This part should probably be in Widget::get_child_at_pos.

Done, Widget::get_child_at_pos now checks the clip path of children.

Because WidgetRef::find_widget_at_pos does explicitly check the layout rect of self before recursing down the tree (else it returns None), I've added a check of the clip path of self, too.

tomcur · 2024-09-12T09:55:27Z

masonry/src/widget/widget.rs

    ///
-    /// The child return is a direct child, not eg a grand-child. The position is in
-    /// relative coordinates. (Eg `(0,0)` is the top-left corner of `self`).
+    /// Has a default implementation that can be overridden to search children more efficiently.


Should implementations overriding the default uphold certain conditions?

Custom implementations should probably uphold the guarantee that pos is actually inside the returned child's layout rect. Must custom custom implementations also uphold the guarantee that pos is contained in the clip path, and that in case of overlap the last child in "z-order" is chosen?

Yes to all of these.

Ideally we should even implement some best-effort checks in debug_assertions mode to enforce that these conditions are upheld.

The low effort version is to check the child contains the position in its layout rect and clip path.

The conditions uniquely identify a child, so we could do the default search alongside Widget::get_child_at_pos and check the same children are returned. For widgets with many children—where custom implementations are most likely—this would be slow. Valid options are to accept that cost or to limit this exhaustive check to widgets with N children for some value of N.

Right, most of that is beyond the scope of this PR.

We could probably add a TODO comment and keep it to that for now.

Valid options are to accept that cost or to limit this exhaustive check to widgets with N children for some value of N.

Sure. I was thinking we'd probably need to add a child_count() method for cases where building a child array is needlessly expensive (eg a Flex with thousands of children).

PoignardAzur

This is roughly what a idiomatic version looks like. The key thing is that since QueryCtx is Copy, you can take it by value in QueryCtx::get, and that means you can request "smaller" lifetimes.

masonry/src/contexts.rs

masonry/src/widget/widget.rs

masonry/src/widget/widget_ref.rs

masonry/src/widget/widget.rs

masonry/src/widget/widget_ref.rs

tomcur · 2024-09-13T14:05:44Z

masonry/src/widget/widget.rs

+        ctx: QueryCtx<'c>,
+        pos: Point,
+    ) -> Option<WidgetRef<'c, dyn Widget>> {
+        let relative_pos = pos - ctx.widget_state.window_origin().to_vec2();


QueryCtx::widget_state is pub(crate), we can access it here but user code currently can't.

Right, same with the clip field access.

PoignardAzur

LGTM.

You could probably add a few methods to QueryCtx so that the default implementation doesn't need to use private fields, but either way, it's a good first implementation.

PoignardAzur · 2024-09-16T16:12:20Z

masonry/src/contexts.rs

+// Methods to get a child WidgetRef from a parent.
+impl<'w> QueryCtx<'w> {
+    /// Return a [`WidgetRef`] to a child widget.
+    pub fn get(self, child: WidgetId) -> WidgetRef<'w, dyn Widget> {


This should probably take WidgetPod for consistency with other context methods.

tomcur force-pushed the widget-get-child-at-pos branch from f56694b to f569230 Compare August 29, 2024 12:06

waywardmonkeys reviewed Aug 30, 2024

View reviewed changes

masonry/src/widget/widget_ref.rs Outdated Show resolved Hide resolved

tomcur mentioned this pull request Aug 30, 2024

masonry: fix updating Portal on scrollbar drag #563

Merged

DJMcNab reviewed Sep 2, 2024

View reviewed changes

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

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

tomcur force-pushed the widget-get-child-at-pos branch from be65d65 to 48c8325 Compare September 3, 2024 11:52

PoignardAzur requested changes Sep 9, 2024

View reviewed changes

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

tomcur force-pushed the widget-get-child-at-pos branch from a5ae090 to 381d996 Compare September 10, 2024 13:50

tomcur added 5 commits September 10, 2024 15:55

masonry: reimplement Widget::get_child_at_pos

098c0bf

wip: shadow pos in WidgetRef::find_widget_at_pos

ee01ea8

wip: provide WidgetRef to Widget::get_child_at_pos

b77bb9f

wip: add QueryCtx, provide it to get_child_at_pos instead of WidgetRef

a17447b

wip: refactor WidgetRef to consist of Widget + QueryCtx

cb2dcaa

tomcur force-pushed the widget-get-child-at-pos branch from 381d996 to cb2dcaa Compare September 10, 2024 13:57

wip: elide lifetime

79348ac

tomcur commented Sep 10, 2024

View reviewed changes

PoignardAzur reviewed Sep 11, 2024

View reviewed changes

PoignardAzur previously approved these changes Sep 11, 2024

View reviewed changes

tomcur added 2 commits September 11, 2024 16:50

wip: refactor Widget::get_child_at_pos to take global coordinates

f0eb0fc

wip: fix global coordinate clip handling

499242c

PoignardAzur reviewed Sep 11, 2024

View reviewed changes

tomcur added 2 commits September 12, 2024 11:41

wip: move clip checking into Widget::get_child_at_pos method

6f88715

wip: improve documentation

7aebe37

tomcur commented Sep 12, 2024

View reviewed changes

tomcur added 2 commits September 12, 2024 11:57

wip: thanks clippy

8c8aa7d

wip: state that the conditions must be upheld

1eac8bc

PoignardAzur reviewed Sep 13, 2024

View reviewed changes

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

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

masonry/src/widget/widget_ref.rs Outdated Show resolved Hide resolved

PoignardAzur reviewed Sep 13, 2024

View reviewed changes

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

masonry/src/widget/widget_ref.rs Outdated Show resolved Hide resolved

tomcur added 5 commits September 13, 2024 15:44

wip: improve lifetimes

f7589ee

wip: elide lifetime

719d9ce

wip: split

0b5528d

wip: split

cfe929e

wip: widget clip only affects children

9591a3c

tomcur commented Sep 13, 2024

View reviewed changes

PoignardAzur approved these changes Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

masonry: reimplement `Widget::get_child_at_pos` #565

masonry: reimplement `Widget::get_child_at_pos` #565

tomcur commented Aug 29, 2024

waywardmonkeys left a comment

PoignardAzur commented Aug 30, 2024

tomcur commented Aug 30, 2024 •

edited

Loading

waywardmonkeys commented Aug 30, 2024

tomcur commented Aug 30, 2024

PoignardAzur commented Aug 30, 2024

tomcur commented Aug 30, 2024

PoignardAzur commented Aug 30, 2024

tomcur commented Sep 1, 2024

DJMcNab left a comment

PoignardAzur left a comment

tomcur commented Sep 10, 2024 •

edited

Loading

tomcur Sep 10, 2024

PoignardAzur Sep 11, 2024

PoignardAzur left a comment

PoignardAzur Sep 11, 2024

PoignardAzur Sep 11, 2024

tomcur Sep 12, 2024

tomcur Sep 12, 2024

PoignardAzur Sep 12, 2024

tomcur Sep 12, 2024

PoignardAzur Sep 13, 2024

PoignardAzur left a comment

tomcur Sep 13, 2024

PoignardAzur Sep 16, 2024

PoignardAzur left a comment

PoignardAzur Sep 16, 2024

	// TODO - Use Widget::get_child_at_pos method
	if let Some(child) = innermost_widget.children().into_iter().find(\|child\| {
	!child.widget.skip_pointer() && child.state().window_layout_rect().contains(pos)
	}) {
	innermost_widget = child;
	} else {
	break;
	}

masonry: reimplement Widget::get_child_at_pos #565

Are you sure you want to change the base?

masonry: reimplement Widget::get_child_at_pos #565

Conversation

tomcur commented Aug 29, 2024

waywardmonkeys left a comment

Choose a reason for hiding this comment

PoignardAzur commented Aug 30, 2024

tomcur commented Aug 30, 2024 • edited Loading

waywardmonkeys commented Aug 30, 2024

tomcur commented Aug 30, 2024

PoignardAzur commented Aug 30, 2024

tomcur commented Aug 30, 2024

PoignardAzur commented Aug 30, 2024

tomcur commented Sep 1, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

PoignardAzur left a comment

Choose a reason for hiding this comment

tomcur commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

PoignardAzur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masonry: reimplement `Widget::get_child_at_pos` #565

masonry: reimplement `Widget::get_child_at_pos` #565

tomcur commented Aug 30, 2024 •

edited

Loading

tomcur commented Sep 10, 2024 •

edited

Loading