Keep the brushes an undo would have rebuilt identically - #642
Conversation
Undo and redo are whole-level snapshots, so taking back a one-brush nudge cost what the level costs: 141 ms at 400 brushes, 122 ms of it restore_state clearing the level and recreating every brush node. On a one-brush edit 399 of 400 records are identical. clear_brushes() takes a set of ids to leave alone now, and restore_state registers those brushes again instead of rebuilding them. Restore at 400 brushes goes 122 ms -> 44 ms. A brush that fails the comparison is rebuilt as before, so a no that could have been a yes costs time and nothing else. recursive_equal rather than ==, because a record carries its faces as an array of dictionaries.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
area: build
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39451ace86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if kept_id != "" and reusable.has(kept_id): | ||
| root.brush_system.reregister_draft_brush(reusable[kept_id]) |
There was a problem hiding this comment.
When an undo restores a deleted brush ahead of an unchanged sibling, this re-registers the sibling in place while the missing brush is rebuilt at the end of DraftBrushes; for example, snapshot order [union A, subtract B, union C] becomes [A, C, B]. append_brush_list_to_csg() consumes container.get_children() in that order, so the restored level can bake with different Boolean semantics than the snapshot. Reused nodes need to be moved into the order represented by the incoming brushes array.
Useful? React with 👍 / 👎.
| if keep_ids.has(str((child as DraftBrush).brush_id)): | ||
| continue |
There was a problem hiding this comment.
After a successful bake has consumed all dirty tags, restoring a snapshot whose brush records all still match—such as undoing a palette-only edit—takes this branch for every brush, so no create_brush_from_info() call tags a brush dirty and no full reconcile is requested. The same clear_brushes() call still removes the baked container at line 870, leaving no baked output, but the next Bake Changed reports “No changed brushes since last bake” and cannot recreate it. Retaining brushes must still invalidate the bake when restoration clears its derived output.
Useful? React with 👍 / 👎.
1 participant
Fixes #600
Undo and redo are whole-level snapshots, so the price of taking back a one-brush nudge was set by the size of the level rather than the size of the edit. At 400 brushes an undo step was 141 ms, of which
restore_state()was 122 ms, because it cleared the level and recreated every brush node.The issue's own measurement is the argument: 22 of the 23 top-level keys in a snapshot are untouched by a brush move, and inside the one that did change, 399 of 400 brush records are identical too.
The rule
A brush whose record is identical to what it would capture right now is the brush that record describes, so restoring it would put back the brush that is already there.
clear_brushes()takes a set of ids to leave alone, andrestore_state()registers those brushes again rather than freeing and rebuilding them. The kept brushes never leave the tree, so there is no reparenting, no owner reassignment and no mesh rebuild; only the registries are cleared and refilled.What is left of restore is mostly the comparison, which is the same walk the capture already does.
Why this cannot be wrong in the direction that matters
A brush that fails the comparison is rebuilt exactly as before, so a comparison that says no when it could have said yes costs time and nothing else.
It cannot say yes wrongly: two brushes with identical records are identical to anything that reads a record, and undo has only ever been a record. If a property is not in the record then the snapshot never captured it, so rebuilding from that snapshot would not have reproduced it either.
The comparison is
recursive_equal()rather than==, because a record carries its faces as an array of dictionaries and==does not go down into those.test_a_face_edit_is_seen_even_though_it_is_nested_in_the_recordis what holds that.Tests
Eleven new, and I checked them in both directions rather than only watching them pass.
Forced reuse to accept everything. Four failed, on the level being wrong rather than on bookkeeping: a resized brush stayed 6x6x6 instead of returning to 2x2x2, the nudge was not taken back, the face edit survived. That is the only way this change can be wrong.
Forced reuse to accept nothing. Every correctness test still passed, which is the property worth having: the level is right whether or not the optimization fires. Only the two tests that exist to pin the optimization failed.
That second run also found a flaw in one of my own tests.
test_an_untouched_brush_is_the_same_node_after_a_restorepassed with reuse fully disabled, becauseclear_brushes()callsqueue_free()and the free is deferred to the end of the frame, sois_instance_valid()stays true either way. It checks the parent now, which is what actually separates kept from rebuilt, and it fails correctly when reuse is off.Docs
CHANGELOG.
tools/vibe/scenarios/level_scale.gdkeeps itsknown()for this issue, rewritten as a regression guard that fires if an undo step goes back over 100 ms, plus a softer note if restore ever creeps back above four times the capture cost.