Skip positioned subtrees only when parent translation doesn't change

[nixonyh]

• Triggers reposition on every set_translation calls too.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Refactor

  • Refined node positioning and translation propagation in the layout system for improved accuracy and consistency.

• Tests

  • Updated unit tests to reflect changes in layout behavior.

Walkthrough

NodeState bitflags are repositioned, and translation propagation is refactored to track whether a parent's translation changed, conditionally re-positioning child nodes only when necessary. The layout pass integrates the new propagation signature with the correct parent world transform.

Changes

Translation Propagation and State Tracking Refactoring

Layer / File(s)	Summary
NodeState bitflag reorganization crates/rectree/src/node.rs	CONSTRAINED, BUILT, and POSITIONED bitflag constants are reassigned to new bit positions. Helper methods is_positioned, needs_reposition, and has_repositioned are relocated within the impl block and tests are expanded to verify the flag-clearing behavior with the new assignments.
Translation marking and propagation refactoring crates/rectree/src/lib.rs	NodeContext::set_translation now calls needs_reposition() to mark nodes as needing re-positioning when translation changes. propagate_translation accepts a new parent_translation_changed: bool flag to conditionally skip subtrees only when both the parent is unchanged and the node is already positioned. It computes world translation, detects changes, and threads the signal to child propagation. Tests are updated to pass the new boolean argument.
Layout pass integration crates/rectree/src/lib.rs	layout computes parent_world from the bubbled node's parent and invokes propagate_translation with the new parent_translation_changed boolean parameter.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• voxell-tech/rectree#23: Both PRs adjust crates/rectree/src/lib.rs::layout to seed propagate_translation with the parent node's world_translation, directly impacting the same translation-propagation flow.

Poem

🐰 Bits rearranged, flags now dance,
Translation threads through nodes' expanse,
Parent change whispers down the tree,
Children positioned, selectively,
Layout sings its new refrain so free!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No description was provided by the author, making it impossible to evaluate whether it relates to the changeset.	Add a pull request description explaining the changes and their purpose to help reviewers understand the rationale for the modifications.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: refactoring translation propagation to skip positioned subtrees only when parent translation doesn't change, which is the core focus of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

crates/rectree/src/lib.rs (2)

122-126: ⚡ Quick win

Avoid clearing POSITIONED when the local offset is unchanged.

This now dirties every child touched by build, even when the computed translation is identical, so the new subtree-skip fast path never applies to those children.

Proposed fix

 fn set_translation(&mut self, id: &Self::Id, translation: Vec2) {
     if let Some(n) = self.get_node_mut(id) {
-        n.translation = translation;
-        n.state.needs_reposition();
+        if n.translation != translation {
+            n.translation = translation;
+            n.state.needs_reposition();
+        }
     }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/rectree/src/lib.rs` around lines 122 - 126, The setter set_translation
currently always writes n.translation and calls n.state.needs_reposition(),
which forces children to be marked dirty even when the translation hasn't
changed; modify set_translation (and the get_node_mut branch) to compare the
existing n.translation with the new translation and return early if they are
equal, only assigning n.translation and calling n.state.needs_reposition() when
the value actually differs so unchanged local offsets don't clear POSITIONED on
descendants.

---

651-675: ⚡ Quick win

Add a regression test for parent_translation_changed = true.

The updated suite still only proves the old skip path. It does not assert the new contract: a positioned child must still be revisited when its parent's world translation changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/rectree/src/lib.rs` around lines 651 - 675, Extend the test around
propagate_translation to cover the new contract: after the initial call that
records wt.tree.for_each_child_calls, invoke propagate_translation again with
parent_translation_changed = true (i.e. pass true as the last argument) and
assert that wt.tree.for_each_child_calls has increased compared to the recorded
value; locate the test function
test_propagate_translation_short_circuits_if_positioned and add a second
propagation call using propagate_translation(&wt.tree, &mut wt.nodes, &0,
Vec2::ZERO, true) followed by an assert_ne!/assert_gt on
wt.tree.for_each_child_calls.get() vs the previous calls to ensure positioned
children are revisited.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/rectree/src/lib.rs`:
- Around line 122-126: The setter set_translation currently always writes
n.translation and calls n.state.needs_reposition(), which forces children to be
marked dirty even when the translation hasn't changed; modify set_translation
(and the get_node_mut branch) to compare the existing n.translation with the new
translation and return early if they are equal, only assigning n.translation and
calling n.state.needs_reposition() when the value actually differs so unchanged
local offsets don't clear POSITIONED on descendants.
- Around line 651-675: Extend the test around propagate_translation to cover the
new contract: after the initial call that records wt.tree.for_each_child_calls,
invoke propagate_translation again with parent_translation_changed = true (i.e.
pass true as the last argument) and assert that wt.tree.for_each_child_calls has
increased compared to the recorded value; locate the test function
test_propagate_translation_short_circuits_if_positioned and add a second
propagation call using propagate_translation(&wt.tree, &mut wt.nodes, &0,
Vec2::ZERO, true) followed by an assert_ne!/assert_gt on
wt.tree.for_each_child_calls.get() vs the previous calls to ensure positioned
children are revisited.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: de3e7c75-ca8b-4000-b6d4-e72b83975a1f

📥 Commits

Reviewing files that changed from the base of the PR and between e9ffbcb and 4aa9a97.

📒 Files selected for processing (2)

• crates/rectree/src/lib.rs
• crates/rectree/src/node.rs