diff --git a/CHANGELOG.md b/CHANGELOG.md
index b43f52b6..f4aad342 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,26 @@ The format is based on Keep a Changelog, and this project follows semantic versi
document look healthier.
### Fixed
+- **Undo keeps the brushes it would have rebuilt identically** (#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. Twenty-two of the
+ twenty-three 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. A
+ brush whose record is identical to what it would capture right now is the brush
+ that record describes, so `clear_brushes()` now takes a set of ids to leave
+ alone and those brushes are registered again rather than freed and rebuilt.
+ Restore at 400 brushes went from 122 ms to 44 ms, and what is left is mostly
+ the comparison, which is the same walk the capture already does. 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; there is no way for it
+ to say yes wrongly, because two brushes with identical records are identical to
+ anything that reads a record, and undo has always been exactly that. 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. The
+ snapshot is still a whole-level snapshot and still costs what it costs to take;
+ this is the half that was rebuilding what nobody changed.
- **The threaded `.hflevel` save does its serializing on the thread** (#601). The
write was threaded and the work in front of it was not, so two thirds of a save
happened on the calling thread. Autosave runs on a timer nobody chose the
diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md
index 379940cd..9c8376fc 100644
--- a/DEVELOPMENT.md
+++ b/DEVELOPMENT.md
@@ -431,7 +431,7 @@ The project has a GitHub Actions workflow (`.github/workflows/ci.yml`) that runs
- `gdformat --check` -- verifies formatting
- `gdlint` -- checks lint rules (configured in `.gdlintrc`)
- `tools/check_placement_order.py` -- refuses a world transform written to a node that is not in the tree yet
-- **GUT unit + integration tests** -- 4,062 tests across 219 test scripts (4,055 passing plus seven intentional no-assert safety tests; 19,673 assertions; verified in CI on September 16, 2026; runs Godot headless)
+- **GUT unit + integration tests** -- 4,073 tests across 220 test scripts (4,066 passing plus seven intentional no-assert safety tests; 19,699 assertions; verified in CI on September 16, 2026; runs Godot headless)
Run locally before pushing:
```
diff --git a/HammerForge_SPEC.md b/HammerForge_SPEC.md
index 36ba45ce..47c75a95 100644
--- a/HammerForge_SPEC.md
+++ b/HammerForge_SPEC.md
@@ -544,6 +544,6 @@ Unit tests use the [GUT](https://github.com/bitwes/Gut) framework and run headle
| `test_selection_gesture.gd` | 40 | Native widget/Object Select ownership, modal Face Select, recovery, focus/scope guards, native duplicate/reparent repair, and Inspector/undo change tracking |
| `test_viewport_outlines.gd` | 39 | Sparse semantic outlines, exact/composite entity collision, visibility/transforms, and shape-aware resize recovery |
-Full suite (verified in CI on September 16, 2026): **4,062 tests** across **219 scripts** (**4,055 passing** plus seven intentional no-assert safety tests; **19,673 assertions**).
+Full suite (verified in CI on September 16, 2026): **4,073 tests** across **220 scripts** (**4,066 passing** plus seven intentional no-assert safety tests; **19,699 assertions**).
Tests use root shim scripts (dynamically created GDScript) to provide the LevelRoot interface without circular preload dependencies. Configuration in `.gutconfig.json`.
diff --git a/README.md b/README.md
index 22c178dd..1dc9f769 100644
--- a/README.md
+++ b/README.md
@@ -17,7 +17,7 @@
- 
+ 
@@ -99,7 +99,7 @@ HammerForge is a single `addons/` folder. No external tools, no custom builds, n
| | |
|---|---|
-| **Subsystem-based coordinator architecture** | **4,062 unit + integration tests** with CI on every push |
+| **Subsystem-based coordinator architecture** | **4,073 unit + integration tests** with CI on every push |
| **15 brush shapes** (box through dodecahedron) | **150 built-in prototype textures** for instant greyboxing |
| **Quake `.map`** + **glTF `.glb`** export | **.hflevel** native format with threaded I/O |
| **Customizable keymaps** (JSON) | **Plugin API** for custom tools |
diff --git a/ROADMAP.md b/ROADMAP.md
index 2209658c..b87a4181 100644
--- a/ROADMAP.md
+++ b/ROADMAP.md
@@ -933,7 +933,7 @@ Completion is responsibility-based rather than tied to an arbitrary line count.
- Headless editor tests retain the complete tool graph, with focused export-playtest coverage guarding the runtime boundary.
### Risk-focused test gaps
-The current suite covers 4,062 tests across 219 scripts, including the large brush, bake, paint, vertex, transform, generator, baker, brush-instance, and map-I/O systems. No issues are open, and every known limitation is either covered by tests or written down beside the wave that introduced it.
+The current suite covers 4,073 tests across 220 scripts, including the large brush, bake, paint, vertex, transform, generator, baker, brush-instance, and map-I/O systems. No issues are open, and every known limitation is either covered by tests or written down beside the wave that introduced it.
The last one on this list is **resolved**: a `.map` entity property value
containing a quote used to come back truncated, silently, because four quotes is
diff --git a/addons/hammerforge/level_root.gd b/addons/hammerforge/level_root.gd
index 485d5e11..339e1696 100644
--- a/addons/hammerforge/level_root.gd
+++ b/addons/hammerforge/level_root.gd
@@ -1456,8 +1456,8 @@ func restore_committed_cuts() -> void:
brush_system.restore_committed_cuts()
-func clear_brushes() -> void:
- brush_system.clear_brushes()
+func clear_brushes(keep_ids: Dictionary = {}) -> void:
+ brush_system.clear_brushes(keep_ids)
func _clear_generated() -> void:
diff --git a/addons/hammerforge/systems/hf_brush_system.gd b/addons/hammerforge/systems/hf_brush_system.gd
index f3dac0a9..de649e97 100644
--- a/addons/hammerforge/systems/hf_brush_system.gd
+++ b/addons/hammerforge/systems/hf_brush_system.gd
@@ -567,6 +567,64 @@ func next_brush_id() -> String:
return _next_brush_id()
+## How deep a brush record can nest before the comparison gives up. A record is
+## a handful of values and an array of faces, so this is far past anything real.
+const _RECORD_COMPARE_DEPTH := 32
+
+
+## The draft brushes a restore can keep, as `{brush_id: DraftBrush}`.
+##
+## A brush is keepable when the record the incoming state holds for it is
+## identical to what that brush would capture right now, which means restoring it
+## would rebuild the brush that is already there. A brush that fails the test is
+## rebuilt as before, so a comparison that says no when it could have said yes
+## costs time and nothing else. There is no way for it to say yes wrongly: two
+## brushes with identical records are identical as far as anything that reads a
+## record is concerned, and undo has always been exactly that.
+func reusable_draft_brushes(records: Array) -> Dictionary:
+ var keep: Dictionary = {}
+ if not root.draft_brushes_node:
+ return keep
+ var wanted: Dictionary = {}
+ for entry in records:
+ if entry is Dictionary and (entry as Dictionary).has("brush_id"):
+ wanted[str((entry as Dictionary)["brush_id"])] = entry
+ if wanted.is_empty():
+ return keep
+ for child in root.draft_brushes_node.get_children():
+ if not (child is DraftBrush):
+ continue
+ var brush := child as DraftBrush
+ var brush_id := str(brush.brush_id)
+ if not wanted.has(brush_id) or keep.has(brush_id):
+ continue
+ var live: Dictionary = get_brush_info_from_node(brush)
+ if live.is_empty():
+ continue
+ var record: Dictionary = wanted[brush_id]
+ # `recursive_equal` rather than `==`, because a record carries an array of
+ # face dictionaries and `==` does not go down into those.
+ if live.recursive_equal(record, _RECORD_COMPARE_DEPTH):
+ keep[brush_id] = brush
+ return keep
+
+
+## Put a kept brush back into the registries a cleared level no longer has it in.
+##
+## Its node was never touched, so there is no tree work here: this is the half of
+## `create_brush_from_info()` that is bookkeeping.
+func reregister_draft_brush(brush: DraftBrush) -> void:
+ if not is_instance_valid(brush):
+ return
+ var brush_id := str(brush.brush_id)
+ if brush_id == "":
+ return
+ _brush_cache[brush_id] = brush
+ _brush_count += 1
+ _advance_id_counter(brush_id)
+ _legacy_manager_add(brush)
+
+
func _register_brush_id(brush_id: String, brush_node: Node = null) -> void:
if brush_id == "":
return
@@ -777,7 +835,15 @@ func restore_committed_cuts() -> void:
root._log("Restored committed cuts (%s)" % restored)
-func clear_brushes() -> void:
+## Take the level down to nothing, except any draft brush named in `keep_ids`.
+##
+## The keep set exists for `restore_state()`, which used to free and rebuild every
+## brush in the level to take back a nudge of one of them (#600). A kept brush
+## stays where it is, in the tree, still owned: only the registries are cleared,
+## and the caller puts the kept ones back into them with
+## `reregister_draft_brush()`. Everything else about the clear is unchanged, and
+## every other caller passes nothing and gets what it always got.
+func clear_brushes(keep_ids: Dictionary = {}) -> void:
clear_face_selection()
_brush_cache.clear()
_brush_count = 0
@@ -786,6 +852,8 @@ func clear_brushes() -> void:
if root.draft_brushes_node:
for child in root.draft_brushes_node.get_children():
if child is DraftBrush:
+ if keep_ids.has(str((child as DraftBrush).brush_id)):
+ continue
root.draft_brushes_node.remove_child(child)
child.queue_free()
_clear_generated()
diff --git a/addons/hammerforge/systems/hf_state_system.gd b/addons/hammerforge/systems/hf_state_system.gd
index ed467b04..c0bbf6e8 100644
--- a/addons/hammerforge/systems/hf_state_system.gd
+++ b/addons/hammerforge/systems/hf_state_system.gd
@@ -164,7 +164,14 @@ func restore_state(state: Dictionary) -> void:
if root.has_signal("user_message"):
root.user_message.emit("Level not loaded: %s" % problem, 2)
return
- root.clear_brushes()
+ # Undo and redo are whole-level snapshots, so taking back a nudge of one brush
+ # used to free and rebuild every brush in the level (#600). A brush whose
+ # record is identical to what it would capture right now is the brush that
+ # record describes, so it is kept across the clear and put back into the
+ # registries rather than rebuilt. At 400 brushes a one-brush edit rebuilds one.
+ var brushes: Array = state.get("brushes", [])
+ var reusable: Dictionary = root.brush_system.reusable_draft_brushes(brushes)
+ root.clear_brushes(reusable)
root.entity_system.clear_entities()
var region_data = state.get("terrain_regions", {})
if region_data is Dictionary and not region_data.is_empty():
@@ -192,8 +199,16 @@ func restore_state(state: Dictionary) -> void:
# already works this way for a malformed brush (#318), and a state can hold
# one for the same reasons — an older version, a partial write, a hand edit.
var skipped := 0
- var brushes: Array = state.get("brushes", [])
for info in brushes:
+ var kept_id := ""
+ if info is Dictionary:
+ kept_id = str((info as Dictionary).get("brush_id", ""))
+ if kept_id != "" and reusable.has(kept_id):
+ root.brush_system.reregister_draft_brush(reusable[kept_id])
+ # Erased so a record that names the same brush twice cannot register
+ # one node into the level twice.
+ reusable.erase(kept_id)
+ continue
if not _restored_brush(info, {}):
skipped += 1
var pending: Array = state.get("pending", [])
diff --git a/docs/features.md b/docs/features.md
index 57de74ac..41a0ea5d 100644
--- a/docs/features.md
+++ b/docs/features.md
@@ -387,7 +387,7 @@ transform group while paint mode is on, so only one of the two is ever live.
## Testing
-The verified Godot 4.7 suite on September 16, 2026 contains **4,062 tests across 219 scripts**: **4,055 passing tests**, seven intentional no-assert safety tests, and **19,673 assertions**. All checks run on every push and pull request via GitHub Actions.
+The verified Godot 4.7 suite on September 16, 2026 contains **4,073 tests across 220 scripts**: **4,066 passing tests**, seven intentional no-assert safety tests, and **19,699 assertions**. All checks run on every push and pull request via GitHub Actions.
```bash
# Run all tests headless
diff --git a/tests/test_restore_reuses_unchanged_brushes.gd b/tests/test_restore_reuses_unchanged_brushes.gd
new file mode 100644
index 00000000..c04af5d8
--- /dev/null
+++ b/tests/test_restore_reuses_unchanged_brushes.gd
@@ -0,0 +1,238 @@
+extends GutTest
+
+## Undo and redo are whole-level snapshots, and `restore_state()` used to free and
+## rebuild every brush in the level to take back an edit to one of them (#600). At
+## 400 brushes that was 122 ms of a 141 ms undo step.
+##
+## 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. Those are kept across the clear and registered again rather than
+## rebuilt.
+##
+## The assertions below are mostly about what a mapper gets, not about how many
+## nodes were spared: a restore has to land the same level whether it rebuilt
+## everything or nothing. The ones that would catch a brush being kept when it
+## should have been rebuilt are the important half, because that is the only way
+## this can be wrong.
+
+const LevelRootType = preload("res://addons/hammerforge/level_root.gd")
+
+var root: LevelRoot
+
+
+func before_each():
+ root = LevelRootType.new()
+ root.auto_spawn_player = false
+ root.hflevel_autosave_enabled = false
+ add_child_autoqfree(root)
+
+
+func _box(at: Vector3, size := Vector3(2, 2, 2)) -> Node:
+ return (
+ root
+ . create_brush_from_info(
+ {
+ "shape": root.BrushShape.BOX,
+ "size": size,
+ "transform": Transform3D(Basis.IDENTITY, at),
+ "operation": CSGShape3D.OPERATION_UNION,
+ }
+ )
+ )
+
+
+func _draft_brushes() -> Array:
+ var out: Array = []
+ for child in root.draft_brushes_node.get_children():
+ if child.has_method("serialize_faces"):
+ out.append(child)
+ return out
+
+
+func _ids() -> Array:
+ var out: Array = []
+ for brush in _draft_brushes():
+ out.append(str(brush.brush_id))
+ out.sort()
+ return out
+
+
+# ===========================================================================
+# The level a restore lands
+# ===========================================================================
+
+
+func test_restoring_an_unchanged_level_leaves_the_same_brushes():
+ _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ _box(Vector3(0, 0, 8))
+ var before := _ids()
+ var snapshot: Dictionary = root.capture_state()
+
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(_ids(), before, "the same brushes, by id")
+ assert_eq(_draft_brushes().size(), 3, "and no more of them")
+
+
+func test_an_untouched_brush_is_the_same_node_after_a_restore():
+ # The point of the change: the node survives rather than being replaced by an
+ # identical one. If this ever has to stop being true, every other test here
+ # still holds, because they are about the level and not about the nodes.
+ # Checked on the parent rather than on `is_instance_valid()`: `clear_brushes()`
+ # takes a brush out of the tree and then `queue_free()`s it, and the free is
+ # deferred to the end of the frame, so a rebuilt brush is still a valid
+ # instance for the whole of this test. Being back under `DraftBrushes` is what
+ # actually separates kept from rebuilt.
+ var kept := _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ var snapshot: Dictionary = root.capture_state()
+ var kept_id := kept.get_instance_id()
+
+ root.state_system.restore_state(snapshot)
+
+ assert_true(is_instance_valid(kept), "the untouched brush was not freed")
+ assert_eq(kept.get_parent(), root.draft_brushes_node, "it never left the level")
+ assert_eq(kept.get_instance_id(), kept_id, "and it is the same node")
+ var ids: Array = []
+ for brush in _draft_brushes():
+ ids.append(brush.get_instance_id())
+ assert_true(ids.has(kept_id), "the brush in the level is that same node, not a copy")
+
+
+func test_a_moved_brush_is_put_back_where_the_snapshot_says():
+ # The case the whole thing is for: nudge one brush on a level of several and
+ # take it back. The moved one cannot be reused, the rest can, and the level
+ # has to come out the same either way.
+ var moved := _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ _box(Vector3(0, 0, 8))
+ var snapshot: Dictionary = root.capture_state()
+
+ moved.global_position = Vector3(0, 16, 0)
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(_draft_brushes().size(), 3, "the level still has three brushes")
+ var heights: Array = []
+ for brush in _draft_brushes():
+ heights.append(roundi(brush.global_position.y))
+ assert_false(heights.has(16), "the nudge was taken back, not left at its new height")
+
+
+func test_a_resized_brush_is_rebuilt_rather_than_kept():
+ # A size change is in the record, so the brush fails the comparison and is
+ # rebuilt. Keeping it would silently leave the level at the wrong size, which
+ # is the one way this can be wrong.
+ var resized := _box(Vector3.ZERO, Vector3(2, 2, 2))
+ var snapshot: Dictionary = root.capture_state()
+
+ resized.size = Vector3(6, 6, 6)
+ root.state_system.restore_state(snapshot)
+
+ var brushes := _draft_brushes()
+ assert_eq(brushes.size(), 1, "still one brush")
+ assert_eq(brushes[0].size, Vector3(2, 2, 2), "and it is the size the snapshot recorded")
+
+
+func test_a_brush_added_after_the_snapshot_is_gone_after_a_restore():
+ _box(Vector3.ZERO)
+ var snapshot: Dictionary = root.capture_state()
+ _box(Vector3(8, 0, 0))
+ assert_eq(_draft_brushes().size(), 2, "two before the restore")
+
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(_draft_brushes().size(), 1, "the brush the snapshot never had is gone")
+
+
+func test_a_brush_deleted_after_the_snapshot_comes_back():
+ var doomed := _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ var snapshot: Dictionary = root.capture_state()
+
+ root.delete_brush(doomed)
+ assert_eq(_draft_brushes().size(), 1, "one after the delete")
+
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(_draft_brushes().size(), 2, "both are back")
+
+
+# ===========================================================================
+# The registries a cleared level no longer holds
+# ===========================================================================
+
+
+func test_a_kept_brush_is_still_in_the_caches_a_restore_clears():
+ # `clear_brushes()` empties the id cache, the count and the BrushManager
+ # mirror. A brush kept across it has to go back into all three, or it is a
+ # node in the tree that nothing can find.
+ _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ var snapshot: Dictionary = root.capture_state()
+
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(root.get_live_brush_count(), 2, "the count knows about both")
+ assert_eq(root.brush_system.get_cached_brush_count(), 2, "and so does the id cache")
+ if root.brush_manager:
+ assert_eq(root.brush_manager.brushes.size(), 2, "and the manager mirror")
+ for brush in _draft_brushes():
+ assert_eq(
+ root.brush_system.find_brush_by_id(str(brush.brush_id)),
+ brush,
+ "every kept brush is findable by its own id"
+ )
+
+
+func test_two_restores_in_a_row_do_not_double_register():
+ _box(Vector3.ZERO)
+ _box(Vector3(8, 0, 0))
+ var snapshot: Dictionary = root.capture_state()
+
+ root.state_system.restore_state(snapshot)
+ root.state_system.restore_state(snapshot)
+
+ assert_eq(_draft_brushes().size(), 2, "still two brushes in the tree")
+ assert_eq(root.get_live_brush_count(), 2, "and the count did not climb")
+
+
+# ===========================================================================
+# The comparison itself
+# ===========================================================================
+
+
+func test_reusable_draft_brushes_offers_only_what_matches():
+ var kept := _box(Vector3.ZERO)
+ var moved := _box(Vector3(8, 0, 0))
+ var records: Array = root.capture_state().get("brushes", [])
+ moved.global_position = Vector3(0, 16, 0)
+
+ var reusable: Dictionary = root.brush_system.reusable_draft_brushes(records)
+
+ assert_true(reusable.has(str(kept.brush_id)), "the untouched brush is offered")
+ assert_false(reusable.has(str(moved.brush_id)), "the moved one is not")
+
+
+func test_nothing_is_offered_against_a_state_with_no_brushes():
+ _box(Vector3.ZERO)
+ assert_eq(
+ root.brush_system.reusable_draft_brushes([]).size(),
+ 0,
+ "a state with no brushes keeps none of them"
+ )
+
+
+func test_a_face_edit_is_seen_even_though_it_is_nested_in_the_record():
+ # A record carries its faces as an array of dictionaries, and plain `==` does
+ # not go down into those. This is what `recursive_equal` is for: a brush whose
+ # geometry moved but whose transform and size did not must still fail.
+ var brush := _box(Vector3.ZERO)
+ var records: Array = root.capture_state().get("brushes", [])
+ assert_gt(brush.faces.size(), 0, "the brush has faces to edit")
+ brush.faces[0].local_verts[0] += Vector3(0, 1, 0)
+
+ assert_false(
+ root.brush_system.reusable_draft_brushes(records).has(str(brush.brush_id)),
+ "a brush whose faces moved is not the brush the record describes"
+ )
diff --git a/tools/vibe/scenarios/level_scale.gd b/tools/vibe/scenarios/level_scale.gd
index 9264a600..01142c3c 100644
--- a/tools/vibe/scenarios/level_scale.gd
+++ b/tools/vibe/scenarios/level_scale.gd
@@ -150,19 +150,30 @@ func _the_cost_curve() -> void:
% [undo_ms, last["capture_ms"], last["restore_ms"], last["state_kb"]]
)
)
+ # #600 was fixed by keeping the brushes a restore would have rebuilt
+ # identically, so restore is no longer the expensive half. This is the guard
+ # that says so if it ever becomes one again.
if undo_ms > 100.0:
known(
600,
- "an undo step at %d brushes costs %.0f ms" % [last["brushes"], undo_ms],
+ "an undo step at %d brushes costs %.0f ms again" % [last["brushes"], undo_ms],
(
(
- "undo is a whole-level snapshot -- capture_state() walks every brush, face, "
- + "entity and paint cell and restore_state() clears the level and rebuilds "
- + "it -- so the price of undoing a one-brush nudge is set by the size of the "
- + "level, not the size of the edit. The snapshot itself is %s KB, and the "
- + "editor keeps a stack of them"
+ "restore_state() keeps a brush whose record matches what it would capture "
+ + "right now rather than rebuilding it, so an undo should cost about what "
+ + "the capture costs. It is costing %.0f ms of restore against %.0f ms of "
+ + "capture, so either the comparison stopped matching or the snapshot grew. "
+ + "The snapshot is %s KB"
)
- % last["state_kb"]
+ % [last["restore_ms"], last["capture_ms"], last["state_kb"]]
+ )
+ )
+ if float(last["restore_ms"]) > float(last["capture_ms"]) * 4.0:
+ note(
+ "restore is still much dearer than capture",
+ (
+ "restore %.1f ms against capture %.1f ms at %d brushes"
+ % [last["restore_ms"], last["capture_ms"], last["brushes"]]
)
)