Implement compose pass by PoignardAzur · Pull Request #512 · linebender/xilem

PoignardAzur · 2024-08-14T12:35:17Z

Add Widget::compose method.
Add ComposeCtx type.
Add needs_compose, request_compose, translation_changed flags to WidgetState.
Add window_origin attribute to WidgetState.
Add convenience methods to TreeArena.
Remove needs_window_origin flag and parent_window_origin attribute.

waywardmonkeys

Some initial comments...

waywardmonkeys · 2024-08-14T17:45:04Z

masonry/src/contexts.rs

 use std::time::Duration;

 use accesskit::{NodeBuilder, TreeUpdate};
+use kurbo::Vec2;


use crate::kurbo::Vec2 ?

I think it doesn't make a difference, they resolve to the same symbol. (Otherwise I think rustc would complain.)

I don't know if we have a coding style for this, but either way, we're already using both versions throughout Masonry, so I'll keep the ones you pointed out as-is.

waywardmonkeys · 2024-08-14T17:46:26Z

masonry/src/contexts.rs

+    /// Request a compose pass.
+    ///
+    /// The compose pass is often cheaper than the layout pass, because it can only transform individual widgets' position.
+    /// [`compose`]: crate::Widget::compose


There's no thing above this that this would be providing a link target for.

Hmm the Link seems still not to be working using cargo doc --no-deps

masonry/src/contexts.rs

waywardmonkeys · 2024-08-14T17:48:38Z

masonry/src/passes/compose.rs

+// Copyright 2024 the Xilem Authors
+// SPDX-License-Identifier: Apache-2.0
+
+use kurbo::{Point, Vec2};


use crate::kurbo... again

Won't fix, see other comment.

waywardmonkeys · 2024-08-14T17:51:26Z

masonry/src/passes/compose.rs

+                panic!(
+                    "Error in '{}' #{}: cannot find child #{} returned by children_ids()",
+                    parent_name,
+                    parent_id.to_raw(),


Should there be a Display or Debug impl for WidgetId?

Sure, but that's a future PR.

masonry/src/passes/compose.rs

waywardmonkeys · 2024-08-14T18:00:16Z

masonry/src/widget/widget_state.rs


 use std::sync::atomic::{AtomicBool, Ordering};

+use kurbo::Vec2;


use crate::kurbo::Vec2;

Actually, can combine with the import below.

(I mean it's just a nit, but I agree, it's slightly cleaner, same for the other use comments here)

Won't fix, see other comment.

We should probably generally find a unique style in the future, just to be consistent.

We've had an agreement in the past in #223, but I needed to rebase it and it slipped through the cracks.

I've resurrected it.

Now that #223 is in, this should become a use vello::kurbo::…

DJMcNab

It's hard to judge exactly what this is doing with no widgets actually using it...

Code seems about as expected, I believe.

DJMcNab · 2024-08-15T09:30:48Z

masonry/src/widget/widget_state.rs

    pub(crate) is_portal: bool,

+    pub(crate) request_compose: bool,
+    // TODO - Handle matrix transforms


Matrices are tricky with text, due to optical size of text (which we don't yet handle). I think changing the scale of a child can only correctly happen in layout

I'm leaving that debate for once we do a complete layout refactor.

PoignardAzur · 2024-08-15T11:51:57Z

I think this is ready to merge?

Philipp-M

(I think there are a few unhandled review comments yet (apart from mine)?)

masonry/src/contexts.rs

masonry/src/passes/compose.rs

masonry/src/tree_arena.rs

masonry/src/widget/widget.rs

masonry/src/widget/widget_pod.rs

Philipp-M · 2024-08-15T12:49:45Z

masonry/src/widget/widget_state.rs


 use std::sync::atomic::{AtomicBool, Ordering};

+use kurbo::Vec2;


(I mean it's just a nit, but I agree, it's slightly cleaner, same for the other use comments here)

DJMcNab

I think the old ParentWindowOrigin code can be removed now?
I would much prefer that to happen in this PR if so.

DJMcNab · 2024-08-15T13:05:11Z

masonry/src/tree_arena.rs

+        self.children.id.unwrap()
+    }
+
+    /// Returns a shared token equivalent to this one.


Shared doesn't seem quite right, given that ArenaRef cannot be copied (etc.). That is, ArenaRef does not appear to have any additional abilities over ArenaMut, except for being passed to methods which accept ArenaRef.

This method also doesn't seem to be used?

Shared doesn't seem quite right, given that ArenaRef cannot be copied

Well now it can =P

This method also doesn't seem to be used?

Yeah, but I'm writing this as though it was a public crate.

DJMcNab · 2024-08-15T13:06:52Z

masonry/src/widget/widget_pod.rs

                InternalLifeCycle::ParentWindowOrigin { .. } => {
-                    state.parent_window_origin = parent_ctx.widget_state.window_origin();
-                    state.needs_window_origin = false;
+                    // FIXME


Does this event need to be removed?

PoignardAzur · 2024-08-16T11:58:32Z

I've done some small fixes to address code review. Next step is removing Lifecycle::ParentWindowOrigin and associated stuff.

PoignardAzur · 2024-08-16T13:32:31Z

I believe this is ready to merge.

PoignardAzur · 2024-08-16T13:34:15Z

We should probably merge #223 first, then rebase this on top of it.

PoignardAzur · 2024-08-16T17:02:49Z

We should probably merge #223 first, then rebase this on top of it.

Done. I believe this addresses all code review points.

DJMcNab

This is compelling! It's really nice to see the back of the parent window origin things

masonry/src/contexts.rs

PoignardAzur · 2024-08-16T17:20:37Z

... It just now occurs to me, as I start the paint pass, that I completely forgot to hook composition with the Scene transforms. We didn't catch it because widgets don't use the pass yet.

Oh well, I'll hook it up with the Portal widget while I implement the paint pass.

PoignardAzur requested review from DJMcNab, Philipp-M and waywardmonkeys August 14, 2024 12:35

waywardmonkeys reviewed Aug 14, 2024

View reviewed changes

PoignardAzur force-pushed the implement_pass_spec_compose branch from 6346f72 to 188c594 Compare August 14, 2024 20:19

DJMcNab reviewed Aug 15, 2024

View reviewed changes

Philipp-M reviewed Aug 15, 2024

View reviewed changes

DJMcNab reviewed Aug 15, 2024

View reviewed changes

PoignardAzur added 6 commits August 16, 2024 19:00

Implement compose pass

9e5354d

Fix flag-handling code in compose pass

0bb2f28

Small fixes

4bd8453

Small fixes

edccd94

Remove ParentWindowOrigin event

13aab24

Fix kurbo imports

136e54a

PoignardAzur force-pushed the implement_pass_spec_compose branch from 5334339 to 136e54a Compare August 16, 2024 17:02

DJMcNab approved these changes Aug 16, 2024

View reviewed changes

PoignardAzur enabled auto-merge August 16, 2024 17:05

PoignardAzur added this pull request to the merge queue Aug 16, 2024

Philipp-M reviewed Aug 16, 2024

View reviewed changes

masonry/src/contexts.rs Show resolved Hide resolved

Merged via the queue into main with commit 652ee68 Aug 16, 2024

PoignardAzur deleted the implement_pass_spec_compose branch August 16, 2024 17:10

PoignardAzur mentioned this pull request Aug 17, 2024

Reserve scene at the beginning of the paint pass #524

Open


		use std::sync::atomic::{AtomicBool, Ordering};

		use kurbo::Vec2;

Conversation

PoignardAzur commented Aug 14, 2024

Uh oh!

waywardmonkeys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PoignardAzur Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PoignardAzur commented Aug 15, 2024

Uh oh!

Philipp-M left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PoignardAzur commented Aug 16, 2024

Uh oh!

PoignardAzur commented Aug 16, 2024

Uh oh!

PoignardAzur commented Aug 16, 2024

Uh oh!

PoignardAzur commented Aug 16, 2024

Uh oh!

DJMcNab left a comment

PoignardAzur Aug 14, 2024 •

edited

Loading