Conversation
Add incremental layout support that finds the optimal ancestor to restart layout from when a node is marked dirty, rather than always re-laying out from that node. Layout restarts only at nodes with stable, reproducible sizes (Pixels or Stretch), while bubbling through Auto and Percentage-sized ancestors. New methods: - `is_restartable()`: checks if a node can serve as a layout restart point - `find_relayout_root()`: walks up the tree to find the optimal restart ancestor Includes comprehensive tests for fixed parents, auto-sizing parents, stretch ancestors, and percentage-sized nodes. Stop relayout walk at absolute ancestors Update `find_relayout_root` to treat absolutely positioned nodes as relayout boundaries. The method now returns the parent immediately when the dirty node is `PositionType::Absolute`, and stops upward traversal when an absolute ancestor is reached. This keeps relayouts for popups/menus and other out-of-flow subtrees from propagating unnecessarily to the root.
There was a problem hiding this comment.
Pull request overview
This PR introduces incremental relayout behavior by selecting an ancestor “restart root” for layout after a node becomes dirty, and adds tests to validate incremental-vs-full layout equivalence across several sizing scenarios. It also removes border and scroll-offset support across the layout engine and ECS wrapper APIs.
Changes:
- Update
Node::layoutto perform incremental relayout by walking ancestors to choose an optimal restart root. - Add an incremental relayout regression test suite covering fixed/auto/stretch/percentage ancestor cases.
- Remove border + scroll-offset fields/APIs and strip border/scroll handling from the layout algorithm.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/node.rs |
Implements incremental relayout root selection; adds parent() requirement; removes border/scroll trait methods. |
src/layout.rs |
Removes border and scroll-offset influence from sizing/positioning calculations. |
src/types.rs |
Adds #[allow(dead_code)] to LayoutType::select (now unused). |
ecs/src/implementations.rs |
Adds parent() for ECS Entity nodes; removes border/scroll getters. |
ecs/src/world.rs |
Removes ECS setters for border and scroll offsets. |
ecs/src/store.rs |
Removes stored border and scroll-offset data. |
examples/advanced.rs |
Updates example node impl to satisfy new parent() requirement; removes scroll fields. |
tests/incremental.rs |
Adds new tests validating incremental relayout matches full layout in key scenarios. |
tests/border.rs |
Deletes border-related tests (border feature removed). |
Comments suppressed due to low confidence (1)
src/types.rs:28
LayoutType::selectis now unused (and suppressed with#[allow(dead_code)]). Since it’spub(crate), keeping dead code adds maintenance burden; consider removing the helper entirely instead of suppressing the lint.
#[allow(dead_code)]
// Helper function for selecting between optional values depending on the layout type.
pub(crate) fn select<T: Default, S>(
&self,
s: S,
first: impl FnOnce(S) -> Option<T>,
second: impl FnOnce(S) -> Option<T>,
) -> Option<T> {
match self {
LayoutType::Row | LayoutType::Overlay => first(s),
LayoutType::Column | LayoutType::Grid => second(s),
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+371
to
375
| fn is_restartable(&self, store: &Self::Store) -> bool { | ||
| let width = self.width(store).unwrap_or(Units::Stretch(1.0)); | ||
| let height = self.height(store).unwrap_or(Units::Stretch(1.0)); | ||
| (width.is_pixels() || width.is_stretch()) && (height.is_pixels() || height.is_stretch()) | ||
| } |
Comment on lines
+407
to
+418
| // Walk up while the current ancestor's size could affect its own parent. | ||
| while let Some(parent) = root.parent(tree) { | ||
| // An absolutely-positioned node is out of its parent's flow, so its size never affects | ||
| // the parent's layout — stop here regardless of its own sizing. | ||
| if root.position_type(store).unwrap_or_default() == PositionType::Absolute { | ||
| break; | ||
| } | ||
| if root.is_restartable(store) { | ||
| break; | ||
| } | ||
| root = parent; | ||
| } |
Comment on lines
+385
to
+388
| /// An [absolutely-positioned](PositionType::Absolute) ancestor is also a valid stopping point: | ||
| /// it is taken out of its parent's flow, so its size cannot affect the parent's layout even when | ||
| /// it is [`Units::Auto`] sized. This keeps a relayout of e.g. an absolutely-positioned popup or | ||
| /// menu contained to that subtree instead of propagating up to the root. |
Comment on lines
292
to
296
| // Absolute children are sized in the same box model used by stack/wrap: | ||
| // padding box (content + padding), excluding border. | ||
| let abs_width = computed_width - border_left - border_right; | ||
| let abs_height = computed_height - border_top - border_bottom; | ||
| let abs_width = computed_width - padding_left - padding_right; | ||
| let abs_height = computed_height - padding_top - padding_bottom; | ||
|
|
Comment on lines
+646
to
+649
| // Available space for children after subtracting padding and border. | ||
| let avail_main = parent_main - padding_main_before - padding_main_after - border_main_before - border_main_after; | ||
| let avail_main = parent_main - padding_main_before - padding_main_after; | ||
| let avail_cross = | ||
| parent_cross - padding_cross_before - padding_cross_after - border_cross_before - border_cross_after; | ||
| parent_cross - padding_cross_before - padding_cross_after; |
Comment on lines
187
to
193
| /// Set the desired maximum horizontal (column) space between children of the given entity. | ||
| pub fn set_max_horizontal_gap(&mut self, entity: Entity, value: Units) { | ||
| self.store.max_horizontal_gap.insert(entity, value); | ||
| } | ||
|
|
||
| /// Set the desired vertical scroll offset. | ||
| pub fn set_vertical_scroll(&mut self, entity: Entity, value: f32) { | ||
| self.store.vertical_scroll.insert(entity, value); | ||
| } | ||
|
|
||
| /// Set the desired horizontal scroll offset. | ||
| pub fn set_horizontal_scroll(&mut self, entity: Entity, value: f32) { | ||
| self.store.horizontal_scroll.insert(entity, value); | ||
| } | ||
|
|
||
| pub fn set_grid_columns(&mut self, entity: Entity, value: Vec<Units>) { | ||
| self.store.grid_columns.insert(entity, value); |
Comment on lines
+108
to
+146
| /// A `Stretch`-sized ancestor is a valid restart point: its size is determined by the parent's | ||
| /// allocation (unaffected by its own descendants) and is reproduced from the cached size. | ||
| #[test] | ||
| fn incremental_restarts_at_stretch_ancestor() { | ||
| let mut world = World::default(); | ||
|
|
||
| let root = world.add(None); | ||
| world.set_width(root, Units::Pixels(600.0)); | ||
| world.set_height(root, Units::Pixels(600.0)); | ||
| world.set_alignment(root, Alignment::TopLeft); | ||
| world.set_layout_type(root, LayoutType::Row); | ||
|
|
||
| // Stretch on both axes -> fills the root. | ||
| let stretch = world.add(Some(root)); | ||
| world.set_width(stretch, Units::Stretch(1.0)); | ||
| world.set_height(stretch, Units::Stretch(1.0)); | ||
| world.set_layout_type(stretch, LayoutType::Column); | ||
|
|
||
| let child = world.add(Some(stretch)); | ||
| world.set_width(child, Units::Pixels(100.0)); | ||
| world.set_height(child, Units::Pixels(100.0)); | ||
|
|
||
| let child2 = world.add(Some(stretch)); | ||
| world.set_width(child2, Units::Pixels(100.0)); | ||
| world.set_height(child2, Units::Pixels(100.0)); | ||
|
|
||
| full_layout(&mut world, root); | ||
| assert_eq!(world.cache.bounds(child2).unwrap().posy, 100.0); | ||
|
|
||
| world.set_height(child, Units::Pixels(180.0)); | ||
| incremental_layout(&mut world, child); | ||
| let incremental = snapshot(&world, &[root, stretch, child, child2]); | ||
|
|
||
| full_layout(&mut world, root); | ||
| let full = snapshot(&world, &[root, stretch, child, child2]); | ||
|
|
||
| assert_eq!(incremental, full); | ||
| assert_eq!(world.cache.bounds(child2).unwrap().posy, 180.0); | ||
| } |
Comment on lines
+49
to
+52
| // Incremental layout: `self` is the node which has been marked as dirty. Rather than | ||
| // always laying out from `self`, find the best ancestor to restart layout from based on | ||
| // whether the change can affect the ancestor. Layout is then performed from that ancestor, | ||
| // recursing through all of its descendants (no unchanged descendants are skipped). |
Adjust relayout root selection so dirty nodes inside absolutely-positioned ancestors restart from the absolute node’s parent, ensuring right/bottom anchored positions are recomputed when content size changes. Also tighten restartability checks to require stable min/max width/height units (pixels/stretch), and align absolute-child sizing to use the full computed size (content + padding). Adds an incremental test that verifies anchored absolute ancestors are repositioned and match a full layout pass.
Comment on lines
+68
to
+74
| let (width, height) = if root.parent(tree).is_some() { | ||
| (cache.width(root), cache.height(root)) | ||
| } else { | ||
| let width = root.width(store).unwrap_or(Units::Pixels(0.0)).to_px(0.0, 0.0); | ||
| let height = root.height(store).unwrap_or(Units::Pixels(0.0)).to_px(0.0, 0.0); | ||
| (width, height) | ||
| }; |
Comment on lines
942
to
+944
| // Phase 7: Lay out absolute children against the container bounds. | ||
| // Absolute children are sized against the padding box (content box + padding, excluding border). | ||
| let abs_avail_main = final_main - border_main_before - border_main_after; | ||
| let abs_avail_cross = final_cross - border_cross_before - border_cross_after; | ||
| let abs_avail_main = final_main; |
Comment on lines
+80
to
+82
| fn parent<'t>(&'t self, _tree: &'t Self::Tree) -> Option<&'t Self> { | ||
| None | ||
| } |
Comment on lines
+94
to
+95
| /// Returns an optional reference to the parent of the node. | ||
| fn parent<'t>(&'t self, tree: &'t Self::Tree) -> Option<&'t Self>; |
Handle incremental relayout calls made before any full/root layout pass by falling back to the tree root when a non-root restart ancestor still has uninitialized 0x0 cached bounds. This avoids deriving constraints from stale cache values. Also adds a regression test that runs incremental layout first and verifies it matches a subsequent full layout, plus a small doc typo fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.