Improve bounding rect code #874

PoignardAzur · 2025-02-20T10:12:49Z

Document concepts of "layout rect" and "bounding rect".
Deprecate local_layout_rect() method.
Add global_layout_rect() method.
Remove transform_changed() method.
Improve bounding rect merging code.
Tweak TestHarness::mouse_move_to.
Tweak WidgetState doc.

masonry/src/core/widget_state.rs

waywardmonkeys · 2025-02-20T11:35:19Z

masonry/src/passes/compose.rs

@@ -75,18 +75,10 @@ fn compose_widget(
            );
            let parent_bounding_rect = parent_state.bounding_rect;

-            // This could be further optimized by more tightly clipping the child bounding rect according to the clip path.


This note about optimization is not present after the code was moved. Is it still a valid future thing to do?

Or is that the new call to overlaps? Either way, the logic in this code has changed, so the PR title that this is just adding documentation starts to become misleading.

Ah, you renamed it after I started looking at it.

This note about optimization is not present after the code was moved. Is it still a valid future thing to do?

I'd have to ask @Philipp-M. I'm not sure the note about optimization made sense in the original PR. I'd rather remove it unless we can point at a specific optimization we expect to do.

Well since the bounds might need to be bigger when transforming the AABB (due to not being able to tighly fit a transformed AABB), it's probably larger afterwards.
An optimization might be an OOB to more tightly represent the widget, or more optimally an arbitrary shape.
I'm not sure how useful that comment is, but at least it hints that this is not yet optimal.

I'm fine not hinting at a future optimization, because I highly doubt there will be one. Tightly clipping an arbitrary path in an oriented bounding box is a hard problem, and it's unclear that it would have any performance benefit at all.

99.9% of cases are going to be AABBs anyway, for which the current code is close to optimal.

Yeah I'm not too attached to that comment, feel free to remove it.

PoignardAzur · 2025-03-01T11:40:33Z

@Philipp-M Any chance you can review this weekend? Otherwise, I expect to merge on Monday.

Philipp-M

I'm a little bit uncomfortable with this change in this form TBH.

I do think we may even want to get rid of layout rects completely, and just keep the "OOB" (ignoring children, i.e. transformed local layout rect?) which is defined by the origin and size * window_transform to keep it more simple.

Philipp-M · 2025-03-01T12:52:49Z

masonry/src/core/contexts.rs

+        pub fn global_layout_rect(&self) -> Rect {
+            Rect::from_origin_size(self.widget_state.window_origin(), self.widget_state.size)
+        }


I'm not sure whether this is really useful at all, as this doesn't really represent the (global) widgets position, bounds or size (due to potential transformations).

I wonder whether we should get rid of "layout rects"?
I think they cause more confusion than being helpful.

Philipp-M · 2025-03-01T12:53:23Z

masonry/src/doc/06_masonry_concepts.md

+## Layout rect
+
+A widget's layout rect includes its self-declared size, and the position attributed by its parent (local position) / parents (global position).
+
+Pointer events cannot target a widget if they are outside its layout rect, though they can target its children (see **Bounding rect** section).
+
+A layout rect doesn't necessarily have a formal definition; it's generally where the widget will be drawn, though a widget can be drawn outside its layout rect; it's usually a widget's own space and containers will try to avoid having its children's layout rects overlap.


I think this section should somehow explain, that a widget can be transformed by its transform.

Pointer events also only reach the widget, when they intersect the transformed widget, thus I don't think it's currently correct. Try for example transformations on the left portal in the xilem http_cats example, to see what I mean.

Philipp-M · 2025-03-01T12:53:31Z

masonry/src/doc/06_masonry_concepts.md

+A widget's bounding rect is a rectangle inside of which pointer events might affect either the widget or its descendants.
+
+In general, the bounding rect is a union or a widget's layout rect and the bounding rects of all its descendants.
+
+The bounding rects of the widget tree form a kind of "bounding volume hierarchy": when looking to find which widget a pointer is on, Masonry will automatically exclude any widget if the pointer is outside its bounding rect.


Might be worth mentioning that bounding rects currently are in window space and axis aligned.

Philipp-M · 2025-03-01T12:54:04Z

masonry/src/passes/compose.rs

@@ -75,18 +75,10 @@ fn compose_widget(
            );
            let parent_bounding_rect = parent_state.bounding_rect;

-            // This could be further optimized by more tightly clipping the child bounding rect according to the clip path.


Well since the bounds might need to be bigger when transforming the AABB (due to not being able to tighly fit a transformed AABB), it's probably larger afterwards.
An optimization might be an OOB to more tightly represent the widget, or more optimally an arbitrary shape.
I'm not sure how useful that comment is, but at least it hints that this is not yet optimal.

Philipp-M · 2025-03-01T12:54:17Z

masonry/src/testing/harness.rs

@@ -472,8 +472,7 @@ impl TestHarness {
    #[track_caller]
    pub fn mouse_move_to(&mut self, id: WidgetId) {
        let widget = self.get_widget(id);
-        let local_widget_center = (widget.ctx().size() / 2.0).to_vec2().to_point();
-        let widget_center = widget.ctx().widget_state.window_transform * local_widget_center;
+        let widget_center = widget.ctx().global_layout_rect().center();


I don't think this change is correct, in that a transformed widget might not be hit.

PoignardAzur · 2025-03-05T09:17:22Z

On second thought, I agree with @Philipp-M that the "layout rect" concept doesn't really make sense anymore with arbitrary transforms.

I've edited the documentation to reflect this.

Philipp-M

Thanks, I'm much more comfortable with this.
I think the PR could already be handling the added TODOs (as IME merged TODOs are less likely to be handled, and I don't think they are a lot of work to handle), but I'm not going to block on this.

Philipp-M · 2025-03-06T08:34:06Z

masonry/src/core/contexts.rs

-    /// This method will panic if [`LayoutCtx::run_layout`] and [`LayoutCtx::place_child`]
-    /// have not been called yet for the child.
+    // TODO - Remove (used in Flex)
+    #[doc(hidden)]
    #[track_caller]
    pub fn child_layout_rect(&self, child: &WidgetPod<impl Widget + ?Sized>) -> Rect {


If it's only used in Flex maybe:

Suggested change

pub fn child_layout_rect(&self, child: &WidgetPod<impl Widget + ?Sized>) -> Rect {

pub(crate) fn child_layout_rect(&self, child: &WidgetPod<impl Widget + ?Sized>) -> Rect {

Might also be worth to fix the parts where the code is used in Flex?
So that we can get rid of it entirely here?

I think that's a whole PR on its own.

Philipp-M · 2025-03-06T08:40:54Z

masonry/src/doc/06_masonry_concepts.md

+<!-- TODO - Include illustration. -->
+
+<!-- TODO - Add section about clip paths and pointer detection. -->


Do you plan to handle the TODOs in this PR?

We need to refactor how clip paths work at some point, it would be part of that.

Philipp-M · 2025-03-06T08:41:05Z

masonry/src/doc/06_masonry_concepts.md

+## Layout rect
+
+Previous versions of Masonry had a concept of a widget's "layout rect", composed of its self-declared size and the position attributed by its parent.
+
+However, given that widgets can have arbitrary transforms, the concept of an axis-aligned layout rect doesn't really make sense anymore.


Not sure if it's even worth mentioning anymore, when all the methods are all "private" and planned to be deleted?

I'd like to keep it at least until all references are removed from the code, including private code.

Philipp-M · 2025-03-06T08:41:10Z

masonry/src/passes/compose.rs

@@ -75,18 +75,10 @@ fn compose_widget(
            );
            let parent_bounding_rect = parent_state.bounding_rect;

-            // This could be further optimized by more tightly clipping the child bounding rect according to the clip path.


Yeah I'm not too attached to that comment, feel free to remove it.

Deprecate local_layout_rect method. Add global_layout_rect method.

PoignardAzur changed the title ~~Document concepts of "Layout rect" and "bounding rect".~~ Improve bounding rect code Feb 20, 2025

PoignardAzur marked this pull request as ready for review February 20, 2025 11:04

waywardmonkeys reviewed Feb 20, 2025

View reviewed changes

Philipp-M self-requested a review February 20, 2025 12:48

Philipp-M reviewed Mar 1, 2025

View reviewed changes

Philipp-M approved these changes Mar 6, 2025

View reviewed changes

PoignardAzur added 7 commits March 6, 2025 13:52

Document concepts of "Layout rect" and "bounding rect".

434efae

Deprecate local_layout_rect method. Add global_layout_rect method.

Remove transform_changed() Ctx method

b74f3f7

Improve bounding_rect merging code

795fccd

Tweak TestHarness::mouse_move_to

230af7c

Tweak WidgetState doc

132e64c

Twek clip_child doc.

aba4a14

Remove references to layout rect

b7fc034

PoignardAzur force-pushed the transform_fixup branch from 83cfa80 to b7fc034 Compare March 6, 2025 12:54

PoignardAzur enabled auto-merge March 6, 2025 12:54

PoignardAzur added this pull request to the merge queue Mar 6, 2025

Merged via the queue into main with commit 93e000d Mar 6, 2025
18 checks passed

PoignardAzur deleted the transform_fixup branch March 6, 2025 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve bounding rect code #874

Improve bounding rect code #874

PoignardAzur commented Feb 20, 2025 •

edited

Loading

waywardmonkeys Feb 20, 2025

waywardmonkeys Feb 20, 2025

waywardmonkeys Feb 20, 2025

PoignardAzur Feb 20, 2025

Philipp-M Mar 1, 2025

PoignardAzur Mar 5, 2025

Philipp-M Mar 6, 2025

PoignardAzur commented Mar 1, 2025

Philipp-M left a comment

Philipp-M Mar 1, 2025

Philipp-M Mar 1, 2025

Philipp-M Mar 1, 2025

Philipp-M Mar 1, 2025

Philipp-M Mar 1, 2025

PoignardAzur commented Mar 5, 2025

Philipp-M left a comment

Philipp-M Mar 6, 2025

PoignardAzur Mar 6, 2025

Philipp-M Mar 6, 2025

PoignardAzur Mar 6, 2025

Philipp-M Mar 6, 2025

PoignardAzur Mar 6, 2025

Philipp-M Mar 6, 2025

	pub fn child_layout_rect(&self, child: &WidgetPod<impl Widget + ?Sized>) -> Rect {
	pub(crate) fn child_layout_rect(&self, child: &WidgetPod<impl Widget + ?Sized>) -> Rect {

		<!-- TODO - Include illustration. -->

		<!-- TODO - Add section about clip paths and pointer detection. -->

Improve bounding rect code #874

Improve bounding rect code #874

Conversation

PoignardAzur commented Feb 20, 2025 • edited Loading

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 commented Mar 1, 2025

Philipp-M 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

PoignardAzur commented Mar 5, 2025

Philipp-M 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 commented Feb 20, 2025 •

edited

Loading