Fix translation seed for non-root layout - #23
Conversation
`layout` seeded `propagate_translation` with the node's own world_translation, so laying out a non-root node re-added its own translation each pass, making it drift by its offset on every layout. This was masked for root layout, where both world and translation are zero. Seed from the parent's world translation instead (zero when the node has no parent). Also prefix the `Id is invalid!` panics with the originating function name.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesLayout parent-world fix and error diagnostics
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rectree/src/lib.rs (1)
222-230: ⚡ Quick winCorrectly fixes the translation drift bug.
The refactored logic now seeds
parent_worldfrom the parent node's storedworld_translation(orVec2::ZEROwhen no parent exists), preventing non-root nodes from accumulating their own offset on each layout pass. The chain ofand_thencalls correctly handles the root case (no parent → zero) and non-root case (parent's absolute position).Optional: clarify "translation" terminology in the comment
Line 223 says "own translation" but in context means "own world_translation" (the absolute position stored in
world_translation). To avoid confusion with thetranslationfield (which holds the local relative offset), consider:- // world translation. Seeding from `bubbled_id`'s own translation + // world translation. Seeding from `bubbled_id`'s own world_translationThis makes it unambiguous that the old behavior read the absolute position, not the local offset.
🤖 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 222 - 230, The current code correctly seeds parent_world from the parent's stored world_translation to prevent drift, but the inline comment is ambiguous about "translation"; update the comment around the let parent_world = ... block to explicitly say "world_translation (absolute position)" rather than "translation" so it’s clear we seed from the parent's world_translation (or Vec2::ZERO when no parent) and not the node's local translation field; reference bubbled_id, nodes.get_node, parent_world, world_translation, and Vec2::ZERO when editing the comment.
🤖 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 222-230: The current code correctly seeds parent_world from the
parent's stored world_translation to prevent drift, but the inline comment is
ambiguous about "translation"; update the comment around the let parent_world =
... block to explicitly say "world_translation (absolute position)" rather than
"translation" so it’s clear we seed from the parent's world_translation (or
Vec2::ZERO when no parent) and not the node's local translation field; reference
bubbled_id, nodes.get_node, parent_world, world_translation, and Vec2::ZERO when
editing the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c14e2241-083d-4b1f-9835-62d5ce54ce5d
📒 Files selected for processing (1)
crates/rectree/src/lib.rs
layoutseededpropagate_translationwith the node's own world_translation, so laying out a non-root node re-added its own translation each pass, making it drift by its offset on every layout. This was masked for root layout, where both world and translation are zero.Seed from the parent's world translation instead (zero when the node has no parent). Also prefix the
Id is invalid!panics with the originating function name.