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

Migrate layout pass #529

Merged
merged 5 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion masonry/examples/calc_masonry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ impl Widget for CalcButton {
}

fn layout(&mut self, ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size {
let size = self.inner.layout(ctx, bc);
let size = ctx.run_layout(&mut self.inner, bc);
ctx.place_child(&mut self.inner, Point::ORIGIN);

size
Expand Down
18 changes: 17 additions & 1 deletion masonry/src/contexts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ use tracing::{trace, warn};
use vello::kurbo::Vec2;

use crate::action::Action;
use crate::passes::layout::run_layout_on;
use crate::render_root::{MutateCallback, RenderRootSignal, RenderRootState};
use crate::text::TextBrush;
use crate::text_helpers::{ImeChangeSignal, TextFieldRegistration};
use crate::tree_arena::ArenaMutChildren;
use crate::widget::{WidgetMut, WidgetState};
use crate::{AllowRawMut, CursorIcon, Insets, Point, Rect, Size, Widget, WidgetId, WidgetPod};
use crate::{
AllowRawMut, BoxConstraints, CursorIcon, Insets, Point, Rect, Size, Widget, WidgetId, WidgetPod,
};

/// A macro for implementing methods on multiple contexts.
///
Expand Down Expand Up @@ -738,6 +741,19 @@ impl LayoutCtx<'_> {
}
}

// TODO - Reorder methods so that methods necessary for layout
// appear higher in documentation.

/// Compute layout of a child widget.
///
/// Container widgets must call this on every child as part of
/// their [`layout`] method.
///
/// [`layout`]: Widget::layout
pub fn run_layout<W: Widget>(&mut self, child: &mut WidgetPod<W>, bc: &BoxConstraints) -> Size {
run_layout_on(self, child, bc)
}

/// Set explicit paint [`Insets`] for this widget.
///
/// You are not required to set explicit paint bounds unless you need
Expand Down
217 changes: 217 additions & 0 deletions masonry/src/passes/layout.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
// Copyright 2024 the Xilem Authors
// SPDX-License-Identifier: Apache-2.0

DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
//! The layout pass, which provides the size and position of each widget
//! before any translations applied in [`compose`](crate::passes::compose).
//! Most of the logic for this pass happens in [`Widget::layout`] implementations.

use tracing::{info_span, trace};
use vello::kurbo::{Point, Rect, Size};

use crate::render_root::RenderRoot;
use crate::widget::WidgetState;
use crate::{BoxConstraints, LayoutCtx, Widget, WidgetPod};

// TODO - Replace with contains_rect once new Kurbo version is released.
// See https://github.com/linebender/kurbo/pull/347
/// Return `true` if all of `smaller` is within `larger`.
fn rect_contains(larger: &Rect, smaller: &Rect) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

smaller.x0 >= larger.x0
&& smaller.x1 <= larger.x1
&& smaller.y0 >= larger.y0
&& smaller.y1 <= larger.y1
}

// Returns "true" if the Widget's layout method was called, in which case debug checks
// need to be run. (See 'called_widget' in WidgetPod::call_widget_method_with_checks)
pub(crate) fn run_layout_inner<W: Widget>(
parent_ctx: &mut LayoutCtx<'_>,
pod: &mut WidgetPod<W>,
bc: &BoxConstraints,
) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

The semantics of this bool return value are very unclear. I can see through digging through the code it's called_widget, i.e. whether the widget's method was actually called and therefore whether the checks need to be ran?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this pattern pulled more weight when every WidgetPod method used it and call_widget_method_with_checks was used everywhere. We'll probably refactor it away soon. In the meantime, I added a comment for clarity.

let id = pod.id().to_raw();
let widget_mut = parent_ctx
.widget_children
.get_child_mut(id)
.expect("WidgetPod: inner widget not found in widget tree");
let mut state_mut = parent_ctx
.widget_state_children
.get_child_mut(id)
.expect("WidgetPod: inner widget not found in widget tree");
let widget = widget_mut.item;
let state = state_mut.item;

if state.is_stashed {
debug_panic!(
"Error in '{}' #{}: trying to compute layout of stashed widget.",
widget.short_type_name(),
id,
);
state.size = Size::ZERO;
return false;
}

// TODO - Not everything that has been re-laid out needs to be repainted.
state.needs_paint = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why no request_paint/request_compose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I wrote this code before I renamed those flags. Will fix.

state.needs_compose = true;
state.needs_accessibility = true;
state.request_paint = true;
state.request_compose = true;
state.request_accessibility = true;

bc.debug_check(widget.short_type_name());
trace!("Computing layout with constraints {:?}", bc);

state.local_paint_rect = Rect::ZERO;

let new_size = {
let mut inner_ctx = LayoutCtx {
widget_state: state,
widget_state_children: state_mut.children.reborrow_mut(),
widget_children: widget_mut.children,
global_state: parent_ctx.global_state,
mouse_pos: parent_ctx.mouse_pos,
};

// TODO - If constraints are the same and request_layout isn't set,
// skip calling layout
inner_ctx.widget_state.request_layout = false;
widget.layout(&mut inner_ctx, bc)
Copy link
Member

Choose a reason for hiding this comment

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

Would this be the case to evaluate a potentially cached layout result? I guess that would be what request_layout would be needed for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I'm adding a comment and leaving it for a future PR.

};
if state.request_layout {
debug_panic!(
"Error in '{}' #{}: layout request flag was set during layout pass",
widget.short_type_name(),
id,
);
}

state.needs_layout = false;
Copy link
Member

Choose a reason for hiding this comment

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

There's a little bit of knowledge encoded in this line, so it could be worth having a comment here.

Not going to block on that, though

state.is_expecting_place_child_call = true;

state.local_paint_rect = state
.local_paint_rect
.union(new_size.to_rect() + state.paint_insets);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of using Add for Insets, but that's a Kurbo thing - this is an idiomatic usage thereof


#[cfg(debug_assertions)]
{
for child_id in widget.children_ids() {
let child_id = child_id.to_raw();
let child_state_mut = state_mut
.children
.get_child_mut(child_id)
.unwrap_or_else(|| panic!("widget #{child_id} not found"));
let child_state = child_state_mut.item;
if child_state.is_expecting_place_child_call {
debug_panic!(
"Error in '{}' #{}: missing call to place_child method for child widget '{}' #{}. During layout pass, if a widget calls WidgetPod::layout() on its child, it then needs to call LayoutCtx::place_child() on the same child.",
widget.short_type_name(),
id,
child_state.widget_name,
child_state.id.to_raw(),
);
}

// TODO - This check might be redundant with the code updating local_paint_rect
let child_rect = child_state.paint_rect();
if !rect_contains(&state.local_paint_rect, &child_rect) && !state.is_portal {
debug_panic!(
"Error in '{}' #{}: paint_rect {:?} doesn't contain paint_rect {:?} of child widget '{}' #{}",
widget.short_type_name(),
id,
state.local_paint_rect,
child_rect,
child_state.widget_name,
child_state.id.to_raw(),
);
}
}
}

// TODO - Figure out how to deal with the overflow problem, eg:
// What happens if a widget returns a size larger than the allowed constraints?
// Some possibilities are:
// - Always clip: might be expensive
// - Display it anyway: might lead to graphical bugs
// - Panic: too harsh?
// Also, we need to avoid spurious crashes when we initialize the app and the
// size is (0,0)
// See https://github.com/linebender/xilem/issues/377

let state_mut = parent_ctx
.widget_state_children
.get_child_mut(id)
.expect("WidgetPod: inner widget not found in widget tree");
parent_ctx.widget_state.merge_up(state_mut.item);
state_mut.item.size = new_size;

{
if new_size.width.is_infinite() {
debug_panic!(
"Error in '{}' #{}: width is infinite",
widget.short_type_name(),
id,
);
}
if new_size.height.is_infinite() {
debug_panic!(
"Error in '{}' #{}: height is infinite",
widget.short_type_name(),
id,
);
}
}

true
}

/// Run [`Widget::layout`] method on the widget contained in `pod`.
/// This will be called by [`LayoutCtx::run_layout`], which is itself called in the parent widget's `layout`.
pub(crate) fn run_layout_on<W: Widget>(
Copy link
Member

Choose a reason for hiding this comment

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

We should have a small doc comment here. E.g.:

Suggested change
pub(crate) fn run_layout_on<W: Widget>(
/// Run [`Widget::layout`] method on the widget contained in `pod`.
/// This will be called by [`LayoutCtx::run_layout`], which is itself called in the parent widget's `layout`.
pub(crate) fn run_layout_on<W: Widget>(

parent_ctx: &mut LayoutCtx<'_>,
pod: &mut WidgetPod<W>,
bc: &BoxConstraints,
) -> Size {
pod.call_widget_method_with_checks(
"layout",
parent_ctx,
|ctx| {
(
ctx.widget_state_children.reborrow(),
ctx.widget_children.reborrow(),
)
},
|child, ctx| run_layout_inner(ctx, child, bc),
);

let id = pod.id().to_raw();
let state_mut = parent_ctx
.widget_state_children
.get_child_mut(id)
.expect("run_layout_on: inner widget not found in widget tree");
state_mut.item.size
}

pub(crate) fn root_layout(
root: &mut RenderRoot,
synthetic_root_state: &mut WidgetState,
bc: &BoxConstraints,
) -> Size {
let _span = info_span!("layout").entered();

let mouse_pos = root.last_mouse_pos.map(|pos| (pos.x, pos.y).into());
let root_state_token = root.widget_arena.widget_states.root_token_mut();
let root_widget_token = root.widget_arena.widgets.root_token_mut();
let mut ctx = LayoutCtx {
global_state: &mut root.state,
widget_state: synthetic_root_state,
widget_state_children: root_state_token,
widget_children: root_widget_token,
mouse_pos,
};

let size = run_layout_on(&mut ctx, &mut root.root, bc);
ctx.place_child(&mut root.root, Point::ORIGIN);

size
}
1 change: 1 addition & 0 deletions masonry/src/passes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::{Widget, WidgetId, WidgetState};
pub mod accessibility;
pub mod compose;
pub mod event;
pub mod layout;
pub mod mutate;
pub mod paint;
pub mod update;
Expand Down
36 changes: 8 additions & 28 deletions masonry/src/render_root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,22 @@ use accesskit::{ActionRequest, Tree, TreeUpdate};
use parley::fontique::{self, Collection, CollectionOptions};
use parley::{FontContext, LayoutContext};
use tracing::{info_span, warn};
use vello::kurbo::{self, Point, Rect};
use vello::kurbo::{self, Rect};
use vello::Scene;

#[cfg(not(target_arch = "wasm32"))]
use std::time::Instant;
#[cfg(target_arch = "wasm32")]
use web_time::Instant;

use crate::contexts::{LayoutCtx, LifeCycleCtx};
use crate::contexts::LifeCycleCtx;
use crate::debug_logger::DebugLogger;
use crate::dpi::{LogicalPosition, LogicalSize, PhysicalSize};
use crate::event::{PointerEvent, TextEvent, WindowEvent};
use crate::passes::accessibility::root_accessibility;
use crate::passes::compose::root_compose;
use crate::passes::event::{root_on_access_event, root_on_pointer_event, root_on_text_event};
use crate::passes::layout::root_layout;
use crate::passes::mutate::{mutate_widget, run_mutate_pass};
use crate::passes::paint::root_paint;
use crate::passes::update::{
Expand Down Expand Up @@ -466,41 +467,20 @@ impl RenderRoot {

// --- MARK: LAYOUT ---
pub(crate) fn root_layout(&mut self) {
let mut dummy_state = WidgetState::synthetic(self.root.id(), self.get_kurbo_size());
let size = self.get_kurbo_size();
let mouse_pos = self.last_mouse_pos.map(|pos| (pos.x, pos.y).into());
let root_state_token = self.widget_arena.widget_states.root_token_mut();
let root_widget_token = self.widget_arena.widgets.root_token_mut();
let mut layout_ctx = LayoutCtx {
global_state: &mut self.state,
widget_state: &mut dummy_state,
widget_state_children: root_state_token,
widget_children: root_widget_token,
mouse_pos,
};

let window_size = self.get_kurbo_size();
let bc = match self.size_policy {
WindowSizePolicy::User => BoxConstraints::tight(size),
WindowSizePolicy::User => BoxConstraints::tight(window_size),
WindowSizePolicy::Content => BoxConstraints::UNBOUNDED,
};

let size = {
layout_ctx
.global_state
.debug_logger
.push_important_span("LAYOUT");
let _span = info_span!("layout").entered();
self.root.layout(&mut layout_ctx, &bc)
};
layout_ctx.place_child(&mut self.root, Point::ORIGIN);
layout_ctx.global_state.debug_logger.pop_span();
let mut dummy_state = WidgetState::synthetic(self.root.id(), self.get_kurbo_size());
let size = root_layout(self, &mut dummy_state, &bc);

if let WindowSizePolicy::Content = self.size_policy {
let new_size = LogicalSize::new(size.width, size.height).to_physical(self.scale_factor);
if self.size != new_size {
self.size = new_size;
layout_ctx
.global_state
self.state
.signal_queue
.push_back(RenderRootSignal::SetSize(new_size));
}
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/testing/helper_widgets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ impl Widget for ReplaceChild {
}

fn layout(&mut self, ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size {
self.child.layout(ctx, bc)
ctx.run_layout(&mut self.child, bc)
}

fn compose(&mut self, _ctx: &mut ComposeCtx) {}
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/widget/align.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl Widget for Align {
fn on_status_change(&mut self, _ctx: &mut LifeCycleCtx, _event: &StatusChange) {}

fn layout(&mut self, ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size {
let size = self.child.layout(ctx, &bc.loosen());
let size = ctx.run_layout(&mut self.child, &bc.loosen());

log_size_warnings(size);

Expand Down
2 changes: 1 addition & 1 deletion masonry/src/widget/button.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ impl Widget for Button {
let padding = Size::new(LABEL_INSETS.x_value(), LABEL_INSETS.y_value());
let label_bc = bc.shrink(padding).loosen();

let label_size = self.label.layout(ctx, &label_bc);
let label_size = ctx.run_layout(&mut self.label, &label_bc);

let baseline = ctx.child_baseline_offset(&self.label);
ctx.set_baseline_offset(baseline + LABEL_INSETS.y1);
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/widget/checkbox.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl Widget for Checkbox {
let x_padding = theme::WIDGET_CONTROL_COMPONENT_PADDING;
let check_size = theme::BASIC_WIDGET_HEIGHT;

let label_size = self.label.layout(ctx, bc);
let label_size = ctx.run_layout(&mut self.label, bc);
ctx.place_child(&mut self.label, (check_size + x_padding, 0.0).into());

let desired_size = Size::new(
Expand Down
Loading