Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support transforms for each widget #753

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions masonry/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use dpi::LogicalPosition;
use parley::{FontContext, LayoutContext};
use tracing::{trace, warn};
use tree_arena::{ArenaMutChildren, ArenaRefChildren};
use vello::kurbo::Vec2;
use vello::peniko::Color;
use winit::window::ResizeDirection;

Expand All @@ -19,8 +18,11 @@ use crate::passes::layout::run_layout_on;
use crate::render_root::{MutateCallback, RenderRootSignal, RenderRootState};
use crate::text::BrushIndex;
use crate::theme::get_debug_color;
use crate::widget::{WidgetMut, WidgetRef, WidgetState};
use crate::{AllowRawMut, BoxConstraints, Insets, Point, Rect, Size, Widget, WidgetId, WidgetPod};
use crate::widget::{CreatedWidget, WidgetMut, WidgetRef, WidgetState};
use crate::{
Affine, AllowRawMut, BoxConstraints, Insets, Point, Rect, Size, Vec2, Widget, WidgetId,
WidgetPod,
};

// Note - Most methods defined in this file revolve around `WidgetState` fields.
// Consider reading `WidgetState` documentation (especially the documented naming scheme)
Expand Down Expand Up @@ -258,8 +260,9 @@ impl_context_method!(
self.widget_state.window_origin()
}

pub fn window_layout_rect(&self) -> Rect {
self.widget_state.window_layout_rect()
/// The axis aligned bounding rect of this widget in window coordinates.
pub fn bounding_rect(&self) -> Rect {
self.widget_state.bounding_rect()
}

pub fn paint_rect(&self) -> Rect {
Expand All @@ -278,7 +281,7 @@ impl_context_method!(
///
/// The returned point is relative to the content area; it excludes window chrome.
pub fn to_window(&self, widget_point: Point) -> Point {
self.window_origin() + widget_point.to_vec2()
self.widget_state.window_transform * widget_point
}
}
);
Expand Down Expand Up @@ -537,6 +540,12 @@ impl_context_method!(MutateCtx<'_>, EventCtx<'_>, UpdateCtx<'_>, {
self.widget_state.request_compose = true;
}

pub fn transform_changed(&mut self) {
trace!("transform_changed");
self.widget_state.transform_changed = true;
self.request_compose();
}

/// Request an animation frame.
pub fn request_anim_frame(&mut self) {
trace!("request_anim_frame");
Expand Down Expand Up @@ -584,6 +593,11 @@ impl_context_method!(MutateCtx<'_>, EventCtx<'_>, UpdateCtx<'_>, {
self.widget_state.needs_update_disabled = true;
self.widget_state.is_explicitly_disabled = disabled;
}

pub fn set_transform(&mut self, transform: Affine) {
self.widget_state.transform = transform;
self.transform_changed();
}
});

// --- MARK: OTHER METHODS ---
Expand Down Expand Up @@ -851,7 +865,7 @@ impl RegisterCtx<'_> {
/// Container widgets should call this on all their children in
/// their implementation of [`Widget::register_children`].
pub fn register_child(&mut self, child: &mut WidgetPod<impl Widget>) {
let Some(widget) = child.take_inner() else {
let Some(CreatedWidget { widget, transform }) = child.take_inner() else {
return;
};

Expand All @@ -861,7 +875,8 @@ impl RegisterCtx<'_> {
}

let id = child.id();
let state = WidgetState::new(child.id(), widget.short_type_name());
let mut state = WidgetState::new(child.id(), widget.short_type_name());
state.transform = transform;

self.widget_children.insert_child(id, Box::new(widget));
self.widget_state_children.insert_child(id, state);
Expand Down Expand Up @@ -944,7 +959,7 @@ impl LayoutCtx<'_> {
) -> Insets {
self.assert_layout_done(child, "compute_insets_from_child");
self.assert_placed(child, "compute_insets_from_child");
let parent_bounds = Rect::ZERO.with_size(my_size);
let parent_bounds = my_size.to_rect();
let union_paint_rect = self
.get_child_state(child)
.paint_rect()
Expand Down Expand Up @@ -1092,7 +1107,7 @@ impl LayoutCtx<'_> {
}
if origin != self.get_child_state_mut(child).origin {
self.get_child_state_mut(child).origin = origin;
self.get_child_state_mut(child).translation_changed = true;
self.get_child_state_mut(child).transform_changed = true;
}
self.get_child_state_mut(child)
.is_expecting_place_child_call = false;
Expand Down Expand Up @@ -1133,7 +1148,7 @@ impl ComposeCtx<'_> {
let child = self.get_child_state_mut(child);
if translation != child.translation {
child.translation = translation;
child.translation_changed = true;
child.transform_changed = true;
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions masonry/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use std::path::PathBuf;

use vello::kurbo::Point;
use winit::event::{Force, Ime, KeyEvent, Modifiers};
use winit::keyboard::ModifiersState;

Expand Down Expand Up @@ -379,6 +380,12 @@ impl PointerEvent {
PointerEvent::Pinch(_, _) => true,
}
}

// TODO Logical/PhysicalPosition as return type instead?
pub fn local_position(&self, ctx: &crate::EventCtx) -> Point {
let position = self.pointer_state().position;
ctx.widget_state.window_transform.inverse() * Point::new(position.x, position.y)
}
}

impl TextEvent {
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/passes/accessibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn build_accessibility_tree(
// --- MARK: BUILD NODE ---
fn build_access_node(widget: &mut dyn Widget, ctx: &mut AccessCtx) -> Node {
let mut node = Node::new(widget.accessibility_role());
node.set_bounds(to_accesskit_rect(ctx.widget_state.window_layout_rect()));
node.set_bounds(to_accesskit_rect(ctx.widget_state.bounding_rect()));

node.set_children(
widget
Expand Down
33 changes: 21 additions & 12 deletions masonry/src/passes/compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use tracing::info_span;
use tree_arena::ArenaMut;
use vello::kurbo::Vec2;
use vello::kurbo::Affine;

use crate::passes::{enter_span_if, recurse_on_children};
use crate::render_root::{RenderRoot, RenderRootSignal, RenderRootState};
Expand All @@ -14,8 +14,8 @@ fn compose_widget(
global_state: &mut RenderRootState,
mut widget: ArenaMut<'_, Box<dyn Widget>>,
mut state: ArenaMut<'_, WidgetState>,
parent_moved: bool,
parent_translation: Vec2,
parent_transformed: bool,
parent_window_transform: Affine,
) {
let _span = enter_span_if(
global_state.trace.compose,
Expand All @@ -24,14 +24,21 @@ fn compose_widget(
state.reborrow(),
);

let moved = parent_moved || state.item.translation_changed;
let translation = parent_translation + state.item.translation + state.item.origin.to_vec2();
state.item.window_origin = translation.to_point();
let transformed = parent_transformed || state.item.transform_changed;

if !parent_moved && !state.item.translation_changed && !state.item.needs_compose {
if !transformed && !state.item.needs_compose {
return;
}

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let local_rect = state.item.size.to_rect();
state.item.bounding_rect = state.item.window_transform.transform_rect_bbox(local_rect);

let mut ctx = ComposeCtx {
global_state,
widget_state: state.item,
Expand All @@ -43,7 +50,7 @@ fn compose_widget(
}

// TODO - Add unit tests for this.
if moved && state.item.accepts_text_input && global_state.is_focused(state.item.id) {
if transformed && state.item.accepts_text_input && global_state.is_focused(state.item.id) {
let ime_area = state.item.get_ime_area();
global_state.emit_signal(RenderRootSignal::new_ime_moved_signal(ime_area));
}
Expand All @@ -55,9 +62,10 @@ fn compose_widget(

state.item.needs_compose = false;
state.item.request_compose = false;
state.item.translation_changed = false;
state.item.transform_changed = false;

let id = state.item.id;
let parent_transform = state.item.window_transform;
let parent_state = state.item;
recurse_on_children(
id,
Expand All @@ -68,9 +76,10 @@ fn compose_widget(
global_state,
widget,
state.reborrow_mut(),
moved,
translation,
transformed,
parent_transform,
);
parent_state.bounding_rect = parent_state.bounding_rect.union(state.item.bounding_rect);
parent_state.merge_up(state.item);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
);
Expand All @@ -92,6 +101,6 @@ pub(crate) fn run_compose_pass(root: &mut RenderRoot) {
root_widget,
root_state,
false,
Vec2::ZERO,
Affine::IDENTITY,
);
}
10 changes: 5 additions & 5 deletions masonry/src/passes/paint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use std::collections::HashMap;

use tracing::{info_span, trace};
use tree_arena::ArenaMut;
use vello::kurbo::{Affine, Stroke};
use vello::peniko::Mix;
use vello::Scene;

use crate::paint_scene_helpers::stroke;
use crate::passes::{enter_span_if, recurse_on_children};
use crate::render_root::{RenderRoot, RenderRootState};
use crate::theme::get_debug_color;
Expand Down Expand Up @@ -53,7 +53,7 @@ fn paint_widget(

let clip = state.item.clip_path;
let has_clip = clip.is_some();
let transform = Affine::translate(state.item.window_origin.to_vec2());
let transform = state.item.window_transform;
let scene = scenes.get(&id).unwrap();

if let Some(clip) = clip {
Expand All @@ -63,7 +63,7 @@ fn paint_widget(
complete_scene.append(scene, Some(transform));

let id = state.item.id;
let size = state.item.size;
let bounding_rect = state.item.bounding_rect;
let parent_state = state.item;
recurse_on_children(
id,
Expand Down Expand Up @@ -93,9 +93,9 @@ fn paint_widget(

if debug_paint {
const BORDER_WIDTH: f64 = 1.0;
let rect = size.to_rect().inset(BORDER_WIDTH / -2.0);
let color = get_debug_color(id.to_raw());
complete_scene.stroke(&Stroke::new(BORDER_WIDTH), transform, color, None, &rect);
let rect = bounding_rect.inset(BORDER_WIDTH / -2.0);
stroke(complete_scene, &rect, color, BORDER_WIDTH);
}

if has_clip {
Expand Down
4 changes: 2 additions & 2 deletions masonry/src/testing/harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ impl TestHarness {
///
/// Combines [`mouse_move`](Self::mouse_move), [`mouse_button_press`](Self::mouse_button_press), and [`mouse_button_release`](Self::mouse_button_release).
pub fn mouse_click_on(&mut self, id: WidgetId) {
let widget_rect = self.get_widget(id).ctx().window_layout_rect();
let widget_rect = self.get_widget(id).ctx().bounding_rect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let widget_center = widget_rect.center();

self.mouse_move(widget_center);
Expand All @@ -430,7 +430,7 @@ impl TestHarness {
pub fn mouse_move_to(&mut self, id: WidgetId) {
// FIXME - handle case where the widget isn't visible
// FIXME - assert that the widget correctly receives the event otherwise?
let widget_rect = self.get_widget(id).ctx().window_layout_rect();
let widget_rect = self.get_widget(id).ctx().bounding_rect();
let widget_center = widget_rect.center();

self.mouse_move(widget_center);
Expand Down
20 changes: 13 additions & 7 deletions masonry/src/testing/helper_widgets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use accesskit::{Node, Role};
use smallvec::SmallVec;
use tracing::trace_span;
use vello::Scene;
use widget::widget::get_child_at_pos;
use widget::widget::{find_widget_at_pos, AsDynWidget as _};
use widget::WidgetRef;

use crate::event::{PointerEvent, TextEvent};
Expand Down Expand Up @@ -390,12 +390,18 @@ impl<S: 'static> Widget for ModularWidget<S> {
CursorIcon::Default
}

fn get_child_at_pos<'c>(
&self,
fn find_widget_at_pos<'c>(
&'c self,
ctx: QueryCtx<'c>,
pos: Point,
) -> Option<WidgetRef<'c, dyn Widget>> {
get_child_at_pos(self, ctx, pos)
find_widget_at_pos(
&WidgetRef {
widget: self.as_dyn(),
ctx,
},
pos,
)
}

fn type_name(&self) -> &'static str {
Expand Down Expand Up @@ -590,12 +596,12 @@ impl<W: Widget> Widget for Recorder<W> {
self.child.get_cursor(ctx, pos)
}

fn get_child_at_pos<'c>(
&self,
fn find_widget_at_pos<'c>(
&'c self,
ctx: QueryCtx<'c>,
pos: Point,
) -> Option<WidgetRef<'c, dyn Widget>> {
self.child.get_child_at_pos(ctx, pos)
self.child.find_widget_at_pos(ctx, pos)
}

fn type_name(&self) -> &'static str {
Expand Down
4 changes: 2 additions & 2 deletions masonry/src/widget/button.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl Button {
///
/// let button = Button::new("Increment");
/// ```
pub fn new(text: impl Into<ArcStr>) -> Button {
pub fn new(text: impl Into<ArcStr>) -> Self {
Philipp-M marked this conversation as resolved.
Show resolved Hide resolved
Button::from_label(Label::new(text))
}

Expand All @@ -56,7 +56,7 @@ impl Button {
/// let label = Label::new("Increment").with_brush(Color::rgb(0.5, 0.5, 0.5));
/// let button = Button::from_label(label);
/// ```
pub fn from_label(label: Label) -> Button {
pub fn from_label(label: Label) -> Self {
Button {
label: WidgetPod::new(label),
}
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/widget/flex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ impl Widget for Flex {
// or be clipped (e.g. if its parent is a Portal).
let my_size: Size = self.direction.pack(major, minor_dim).into();

let my_bounds = Rect::ZERO.with_size(my_size);
let my_bounds = my_size.to_rect();
let insets = child_paint_rect - my_bounds;
ctx.set_paint_insets(insets);

Expand Down
1 change: 1 addition & 0 deletions masonry/src/widget/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ pub use widget_pod::WidgetPod;
pub use widget_ref::WidgetRef;

pub(crate) use widget_arena::WidgetArena;
pub(crate) use widget_pod::CreatedWidget;
pub(crate) use widget_state::WidgetState;

use crate::{Affine, Size};
Expand Down
Loading
Loading