Record a displacement edit as the brush it edits - #762
Merged
Merged
Conversation
The displacement commands were the biggest group still taking a whole-level undo snapshot. The sculpt drag was the worst of them: one snapshot on mouse-down and another on mouse-up, on a gesture that is held down. Route dock._try_undoable_action() through HFUndoHelper.commit_completed() and give it an optional brush scope. The five dock buttons, the elevation spin, the sew group spin and the sculpt drag all pass one. Painting a material passes one too: the paint writes material_override on the node and never reaches the palette. Sewing keeps the whole snapshot. It pairs displacements by sew group across the level, so it changes brushes the caller never named. A stroke's pre-state is a scope now, so the Escape-cancels-a-sculpt path can no longer hand it to restore_state(), which would read it as a level with no entities, no materials and no visgroups and clear all three. One function owns ending a stroke and picks the restore that matches.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Part of #761. The displacement family, which was the biggest group of commands still recording the whole level on every edit.
What changed:
dock._try_undoable_action()was a hand-rolled capture/call/register-a-restore-pair. It now goes throughHFUndoHelper.commit_completed()and takes an optional brush scope. Its five displacement callers pass one.HFUndoHelper.commit()and only lacked the scope argument.material_overrideon the node, andmaterialsis unchanged even for a material no palette slot holds. There is a test for exactly that.commit_completed()andregister_action()now share one_after_state()for the do side, so the fallback when a scope did not survive the command cannot drift between them.Sewing keeps the whole snapshot.
sew_all()pairs displacements by sew group across the level, so it changes brushes the caller never named. There is a test that pins that too, so a later change cannot quietly scope it.One bug this found in itself: a stroke's pre-state is a brush scope now, and the Escape-cancels-a-sculpt path was handing it to
restore_state(). A scope holds two keys and a level state holds twenty-five, andrestore_state()clears what it is not given, so that would have wiped every entity, material and visgroup in the level. The stroke now carries the scope ids beside the state and one function owns which restore matches, used by the cancel, the recovery and the normal end.Tests:
tests/test_scoped_undo_step.gdruns each newly scoped command between two whole-level captures and assertsbrushesis the only key that differs, plus a round trip per command, plus the Escape regression intests/test_plugin_shortcuts.gd.Still open on #761: bevel and inset are the same hand-rolled shape and are scopeable; the decal, path, polygon, drop and prefab sites change which brushes exist, so most of them cannot scope at all; and the entity scope is still the design question the issue parked.