diff --git a/bindings/c/clay.h b/bindings/c/clay.h index 029d1552..1d56c001 100644 --- a/bindings/c/clay.h +++ b/bindings/c/clay.h @@ -1235,11 +1235,20 @@ clay_result clay_remove_node(clay_document* doc, clay_layer_id layer, clay_node_ * together, bit-exact. Through 0.120.0 none of them recorded anything, and an * undo after a dial reverted the PASS onto cells the dial had moved. * + * CREATING A VOXEL SCULPT LAYER IS A STEP, and so is every edit made while one + * is recording: clay_voxel_begin_sculpt_layer records the creation, and an edit + * inside the layer records its cells AND what it did to the layer's record. + * Through 0.120.0 neither recorded the layer: undoing a dab left its cells + * listed in the record, so the next dial put them back, and a journal replayed + * onto a snapshot older than the layer was refused at the first operation + * naming it. Undoing a creation ENDS the recording; redoing it brings the layer + * back CLOSED, so a host mid-pass that undoes past the begin begins again. An + * edit inside a layer that changed no cell is dropped with its entries in the + * record, where with undo off the record still lists them. + * * WHAT IS STILL NOT A STEP, because nothing records it: creating a MASK — mask - * EDITS record, since 0.47.0, but the mask's existence does not — creating a - * voxel SCULPT LAYER (clay_voxel_begin_sculpt_layer; the pass's cells are steps, - * the layer's record is not), and dropping a resolution level, which destroys - * history itself. Consolidate IS undoable and is worth naming because it is the + * EDITS record, since 0.47.0, but the mask's existence does not — and dropping + * a resolution level, which destroys history itself. Consolidate IS undoable and is worth naming because it is the * one most often assumed otherwise. * * ENABLING MID-SESSION starts an EMPTY history and is never refused: what the diff --git a/bindings/c/clay_c.cpp b/bindings/c/clay_c.cpp index 3d84c141..397c777f 100644 --- a/bindings/c/clay_c.cpp +++ b/bindings/c/clay_c.cpp @@ -13096,7 +13096,11 @@ clay_result clay_voxel_begin_sculpt_layer(clay_voxel_grid* grid, const char* nam if (r != CLAY_OK) return r; if (g->recording_sculpt_layer()) return fail(CLAY_ERROR_INVALID_ARGUMENT, "a sculpt layer is already recording"); - const std::size_t layer = g->begin_sculpt_layer(name ? std::string(name) : std::string()); + // Creation is a step (#642), so undoing back past it removes the layer and + // a journal replayed onto an older snapshot has a layer to fill. + LayerOpStep step(grid); + const std::size_t layer = + g->begin_sculpt_layer(name ? std::string(name) : std::string(), step.record()); if (out_layer) *out_layer = layer; return CLAY_OK; } diff --git a/bindings/python/pyclay_module.cpp b/bindings/python/pyclay_module.cpp index 3b24efcd..d8a4cc9f 100644 --- a/bindings/python/pyclay_module.cpp +++ b/bindings/python/pyclay_module.cpp @@ -11139,7 +11139,8 @@ NB_MODULE(pyclay, m) { [](PySculptLayerScope& s) { if (s.grid.grid().recording_sculpt_layer()) throw nb::value_error("a sculpt layer is already recording"); - s.index = s.grid.grid().begin_sculpt_layer(s.name); + PyLayerOpStep step(s.grid); // creation is a step (#642) + s.index = s.grid.grid().begin_sculpt_layer(s.name, step.record()); return s.index; }) .def("__exit__", @@ -11464,7 +11465,8 @@ NB_MODULE(pyclay, m) { [](PyVoxelGrid& g, const std::string& name) { if (g.grid().recording_sculpt_layer()) throw nb::value_error("a sculpt layer is already recording"); - return g.grid().begin_sculpt_layer(name); + PyLayerOpStep step(g); // creation is a step (#642) + return g.grid().begin_sculpt_layer(name, step.record()); }, "name"_a = std::string(), "Start recording; every edit until end_sculpt_layer joins this pass") diff --git a/bindings/python/tests/test_voxel_layer_history.py b/bindings/python/tests/test_voxel_layer_history.py index 7fac5d6b..7d242d43 100644 --- a/bindings/python/tests/test_voxel_layer_history.py +++ b/bindings/python/tests/test_voxel_layer_history.py @@ -30,11 +30,11 @@ def test_undoing_a_dial_restores_the_dial_not_the_pass(): doc, grid = _doc() _pass(grid, "wrinkles", 6, 2) full, cells = doc.to_bytes(), grid.sculpt_layer_cell_count(0) - assert doc.undo_depth == 1 + assert doc.undo_depth == 2 # the layer's creation, and the pass grid.set_sculpt_layer_strength(0, 0.4) dialled = doc.to_bytes() - assert doc.undo_depth == 2 + assert doc.undo_depth == 3 assert doc.undo() assert grid.sculpt_layer_strength(0) == 1.0 @@ -47,6 +47,32 @@ def test_undoing_a_dial_restores_the_dial_not_the_pass(): assert doc.to_bytes() == dialled +def test_undoing_a_pass_takes_it_out_of_the_layer_and_then_the_layer(): + # Regression (#642): the undo reverted the pass's cells and left the + # layer's record listing them, so the next dial put them back. + doc, grid = _doc() + start = doc.to_bytes() + with grid.sculpt_layer("wrinkles"): + opened, occupied = doc.to_bytes(), grid.occupied_count + grid.sculpt_inflate((0, 6, 0), size=9, amount=2) + + assert doc.undo() + assert grid.sculpt_layer_cell_count(0) == 0 + assert doc.to_bytes() == opened + grid.set_sculpt_layer_strength(0, 0.5) + assert grid.occupied_count == occupied + dialled = doc.to_bytes() + assert doc.undo() # the dial + assert doc.undo() # the layer itself + assert grid.sculpt_layer_count == 0 + assert doc.to_bytes() == start + + # The dial was a new edit, so the redo side holds the layer and the dial. + assert doc.redo() and doc.redo() + assert doc.to_bytes() == dialled + assert not grid.recording_sculpt_layer # a redone creation comes back closed + + def test_undoing_a_merge_down_restores_both_layers(): doc, grid = _doc() _pass(grid, "lower", 6, 2) diff --git a/docs/05-claycore-library.md b/docs/05-claycore-library.md index 0abf4c9a..59da0dae 100644 --- a/docs/05-claycore-library.md +++ b/docs/05-claycore-library.md @@ -1491,14 +1491,33 @@ folded upper pass plus what it overwrote in the lower one, not a second copy of the lower. The mesh stack has recorded the same operations since 0.76.0 (`MultiresLayerProperty`); the two stacks now agree. +**Creating a voxel sculpt layer is a step too, and so is every edit made inside +one** (#642). `clay_voxel_begin_sculpt_layer` / `begin_sculpt_layer` records the +creation, and an edit made while a layer is recording records its cells *and* +what it did to the layer's record of the pass. Before that, the layer was the +one part of a pass the history never saw: undoing a dab reverted its cells and +left the record listing them, so a later dial of the layer recomposed and put +the undone cells back; and a journal replayed onto a snapshot taken before the +layer existed rebuilt the cells and not the stack, and was refused at the first +operation naming the layer. Now a pass costs two undos — the edit, then the +layer — and the journal carries both. Three details a host can see: + +- **Undoing a creation ends the recording**, since the layer being recorded + into is gone; **redoing it brings the layer back closed**. Whether a layer is + recording is the state of a gesture, not of the document (it is not saved + either), and reopening one on redo would refuse a host's next begin. +- **An edit inside a layer that changed no cell leaves no trace**, with undo on: + it is dropped like any other no-op edit, and its entries in the layer's record + are dropped with it. With undo off the record lists them as before. + Keeping them would leave entries no step accounts for, and every later + edit's undo, redo and journal replay would find the record a different + length from the one it recorded and refuse. +- **Undoing a creation whose record is not empty is refused**, like any step + whose layer no longer has the shape it names — the passes inside a layer are + later steps and are undone first. + What genuinely is not: -- **Creating a VOXEL sculpt layer.** `begin_sculpt_layer` adds a record the - history does not see. The pass's *cells* are steps, so undoing them restores - the grid — but the layer's record keeps the cells it recorded, and a later - dial of that layer replays them. A journal replayed onto a snapshot taken - before the layer existed rebuilds the cells and not the stack, and is refused - at the first operation that names the missing layer. - **Operations that destroy history itself** — dropping a resolution level. - **Creating a mask.** Mask *edits* record; the mask's existence does not. It is the same shape of gap that layer creation had until #341 closed it. diff --git a/include/clay/session/history.h b/include/clay/session/history.h index b6273d92..4a605cb1 100644 --- a/include/clay/session/history.h +++ b/include/clay/session/history.h @@ -147,7 +147,11 @@ struct Step { // to the stroke before is a history that lied about what it holds. MultiresLayerProperty, // ONE OPERATION ON A VOXEL GRID'S SCULPT-LAYER STACK: a strength, a - // visibility, a reorder, a removal or a merge-down. + // visibility, a reorder, a removal or a merge-down — and, since #642, + // a layer's creation and every voxel edit made while one is recording. + // Such an edit is a Voxel step's cells plus what it did to the layer's + // record; recorded as a Voxel step, its undo reverted the cells and + // left the record listing them, and the next dial put them back. // // The voxel half of what MultiresLayerProperty is for the mesh stack, // and a separate kind from Voxel for the reason that one gives: a @@ -274,6 +278,11 @@ class History { // history's, so every cell the verbs write is journaled in order. Nested // calls are refused rather than nested: a step is one edit. // + // While the grid is recording a sculpt layer the step is recorded as a + // VoxelLayerProperty step instead, carrying the same cells and what the + // edit did to the layer's record, so undoing it takes the cells out of the + // pass as well as off the grid. + // // The grid is taken by reference on both sides rather than resolved, // because a caller recording a step is holding the grid already. bool begin_voxel_step(scene::LayerId layer, voxel::VoxelGrid& grid); @@ -632,6 +641,9 @@ class History { static std::size_t step_bytes(const Step& s); static std::size_t event_bytes(const JournalEvent& e); std::vector open_cells_; + // What the open voxel step does to a recording sculpt layer's record; see + // VoxelGrid::begin_pass_capture. Empty when no layer is recording. + voxel::VoxelGrid::SculptLayerOp open_pass_; scene::LayerId open_layer_ = 0; bool voxel_open_ = false; bool mask_open_ = false; diff --git a/include/clay/voxel/grid.h b/include/clay/voxel/grid.h index bc5a3e9f..55c43441 100644 --- a/include/clay/voxel/grid.h +++ b/include/clay/voxel/grid.h @@ -200,9 +200,16 @@ class VoxelGrid { void reapply_changes(const std::vector& changes); std::size_t sculpt_layer_count() const { return sculpt_layers_.size(); } + struct SculptLayerOp; // Start recording. Writes from here on are attributed to the new layer, // which begins at full strength and visible. Returns its index. - std::size_t begin_sculpt_layer(std::string name = {}); + // + // `record`, when given, receives the CREATION as an undo step (Kind::Begin, + // #642): undoing it takes the layer off the stack, redoing it puts it back. + // Before that record existed a layer was the one part of a pass the history + // never saw, so an undone pass left its cells listed in the record and a + // journal replayed onto an older snapshot had no layer to put them in. + std::size_t begin_sculpt_layer(std::string name = {}, SculptLayerOp* record = nullptr); // Stop recording. Further writes are attributed to no layer, exactly as // they were before any layer existed. void end_sculpt_layer(); @@ -244,7 +251,25 @@ class VoxelGrid { // None is what an operation that changed nothing leaves behind — the // same strength again, a move onto itself — and a caller recording // steps drops it, as every other recorder drops a no-op. - enum class Kind : std::uint8_t { None, Strength, Visible, Move, Remove, Merge }; + // + // Begin and Pass are APPENDED (#642), so every value an earlier record + // carries keeps its meaning. + // + // Begin: a layer's creation. `layer` is its index, always the top, and + // `held` its name and seed; nothing else is set. + // + // Pass: one edit made while a layer was recording. The edit is two + // things — the cells it wrote, which `cells` carries exactly as a Voxel + // step would, and what it did to the layer's RECORD of the pass, which + // is what makes it a layer operation at all. An undo that restored the + // cells alone would leave the record listing them, and the next dial + // would recompose and put them back. The record half is `lower_count` + // (the record's length before the edit), `lower_afters` (the entries + // it rewrote, with the `after` each had), `pass_afters` (the same + // entries with the `after` the edit left) and `held.changes` (the + // entries it appended). The field names are the merge's, whose record + // half is the same shape: a truncation and some overwritten afters. + enum class Kind : std::uint8_t { None, Strength, Visible, Move, Remove, Merge, Begin, Pass }; Kind kind = Kind::None; std::size_t layer = 0; // Move: the index it moved FROM std::size_t to = 0; // Move only @@ -260,6 +285,8 @@ class VoxelGrid { // which would double the step for the layer most likely to be large. std::size_t lower_count = 0; std::vector> lower_afters; + // Pass only: parallel to lower_afters, the value each entry was left at. + std::vector> pass_afters; // Every cell the recompose wrote, in the order it wrote them. std::vector cells; @@ -272,11 +299,33 @@ class VoxelGrid { }; // Replay a recorded operation backwards (forward = false) or forwards. Refused // — false, grid untouched — when the stack no longer has the shape the - // record names: an index past the end, or a lower layer shorter than a - // merge's record of it. Neither direction recomposes, and neither records - // into a sculpt layer or a change sink. + // record names: an index past the end, a lower layer shorter than a + // merge's record of it, a creation that is not the top layer, or a pass + // record of a different length than the one the edit left. Neither + // direction recomposes, and neither records into a sculpt layer or a + // change sink. + // + // Undoing a creation also ENDS recording, since the layer being recorded + // into is gone; redoing it brings the layer back CLOSED. Whether a layer + // is recording is the state of a gesture, not of the document — it is not + // saved either — and reopening one on redo would have a host that had + // already ended the pass refused at its next begin. bool apply_sculpt_layer_op(const SculptLayerOp& op, bool forward); + // What an edit does to the OPEN layer's record, captured as a Kind::Pass + // operation (#642). A third channel beside the change sink, for the reason + // the change sink is a second one: the choke point sees every write, and a + // caller decides what an undo step is. Between these two calls, every + // entry the recording hook rewrites is noted in `record`; closing it fills + // in what was appended and what the rewritten entries were left at. + // + // `record` is reset on begin, and stays `empty()` when no layer was + // recording or the edit did not touch its record. It does not receive the + // cells — the change sink does, and a caller holding both joins them. + // Starting another layer inside the bracket closes it. + void begin_pass_capture(SculptLayerOp* record); + void end_pass_capture(); + // Both recompose the grid: the layers above this one are reverted, the // change applied, and they are replayed. False for a layer this grid does // not have. `record`, when given, receives the operation as an undo step @@ -931,6 +980,12 @@ class VoxelGrid { void unfold_down(const SculptLayerOp& op); void redo_layer_property(const SculptLayerOp& op); void undo_layer_property(const SculptLayerOp& op); + // The record halves of a creation and of a pass, in either direction. + void redo_begin(const SculptLayerOp& op); + void undo_begin(); + void redo_pass(const SculptLayerOp& op); + void undo_pass(const SculptLayerOp& op); + static void truncate_record(SculptLayerRecord& rec, std::size_t count); void move_record(std::size_t from, std::size_t to); static SculptLayerData to_data(SculptLayerRecord rec); static SculptLayerRecord from_data(SculptLayerData data); @@ -938,6 +993,8 @@ class VoxelGrid { std::vector sculpt_layers_; // Not owned. Null is off; see set_change_sink. std::vector* change_sink_ = nullptr; + // Not owned. Null is off; see begin_pass_capture. + SculptLayerOp* pass_capture_ = nullptr; bool recording_ = false; std::uint32_t next_sculpt_seed_ = 1; diff --git a/openspec/ROADMAP.md b/openspec/ROADMAP.md index 1c7e934b..f9bde9c1 100644 --- a/openspec/ROADMAP.md +++ b/openspec/ROADMAP.md @@ -1421,7 +1421,7 @@ moves are the ones already shipped. | ~~**P0**~~ | ~~`add-history-budget`~~ **landed 2026-08-24** | Unbounded allocation in a multi-hour session on an OS that kills for memory. Bytes, a budget, on-demand trim, and a horizon a host can show. **What building it found:** the expensive entries are the inverses of REMOVALS, since the stack stores inverses — deleting an item records a whole node while adding one records an id, so a session of deletes and a session of adds cost very differently and nothing told the host which it was in | | ~~**P0**~~ | ~~`add-surface-groups`~~ **landed 2026-08-24** | The largest genuinely-absent workflow primitive. **Landed TWICE, and the first time did not count**: the lattice, `isolate` and the visibility flags shipped with tests and were reachable from no host at all — no C entry point, no pyclay, no serialisation, and the mesher never asked, so hiding a group hid nothing. The second half is what made it a workflow: grow/shrink/border, a `'GRUP'` chunk, undo, both bindings, and geometry that actually disappears. Hiding filters the produced MESH rather than the field, so it is exactly reversible and cannot change what the document evaluates to | | ~~**P0**~~ | ~~A version tag~~ **caught up 2026-08-25 (v0.52.0)** | The oldest row on this list, closed twice: v0.49.0 took the ten-minor backlog and v0.52.0 takes the three that landed after it. **What both bumps found is the same defect, and it is worth a gate rather than a habit:** the minor is bumped in `CMakeLists.txt` by the feature that grows the ABI, and `clay.h` and `pyproject.toml` are left behind — v0.49.0 shipped from a tree whose header and wheel said 0.48.0, and v0.52.0 from one where both said 0.49.0 against a CMake at 0.52.0. `release_check.py`'s `version` row is the only thing that catches it, and it catches it at tag time rather than at merge time | -| ~~**P0**~~ | ~~`unify-the-undo-history`~~ **engine half landed 2026-08-23; finished and archived 2026-09-23** | Three history mechanisms and no step spanning two — `correct-the-undo-scope` found it and only wrote it down. Closed by a `session` module above scene/voxel/mesh, because `check_layering.py` forbids `scene` from seeing the other two. Three things the plan got wrong, all caught by tests: consolidate IS undoable (the barrier examples were consolidate and rasterize, and both are recorded), the cell sink first journaled writes that changed nothing and a unit test enshrined it, and `UndoStack::begin_group` pushes its entry at BEGIN so grouped edits recorded no step at all. **The finding worth carrying forward: `voxel::MaskField` is a FOURTH representation** — twenty mutating ABI entry points, zero command variants — which the audit that counted three did not count. The voxel sculpt-layer property changes this row left open were closed on 2026-09-23 in that shape — one `VoxelLayerProperty` kind through the existing `GridFor`, no new resolver. Still open around it: creating a voxel sculpt layer is not a history event (#642), a mesh sculptor stamp is not a document step (#643), a mid-session enable pairs the journal with a stale snapshot (#641), and the device cost of the journal (#644) | +| ~~**P0**~~ | ~~`unify-the-undo-history`~~ **engine half landed 2026-08-23; finished and archived 2026-09-23** | Three history mechanisms and no step spanning two — `correct-the-undo-scope` found it and only wrote it down. Closed by a `session` module above scene/voxel/mesh, because `check_layering.py` forbids `scene` from seeing the other two. Three things the plan got wrong, all caught by tests: consolidate IS undoable (the barrier examples were consolidate and rasterize, and both are recorded), the cell sink first journaled writes that changed nothing and a unit test enshrined it, and `UndoStack::begin_group` pushes its entry at BEGIN so grouped edits recorded no step at all. **The finding worth carrying forward: `voxel::MaskField` is a FOURTH representation** — twenty mutating ABI entry points, zero command variants — which the audit that counted three did not count. The voxel sculpt-layer property changes this row left open were closed on 2026-09-23 in that shape — one `VoxelLayerProperty` kind through the existing `GridFor`, no new resolver. Creating a voxel sculpt layer, and each edit made inside one, became steps on 2026-09-23 as two appended `SculptLayerOp` kinds, Begin and Pass (#642, `make-voxel-sculpt-layer-creation-a-step`). Still open around it: a mesh sculptor stamp is not a document step (#643), a mid-session enable pairs the journal with a stale snapshot (#641), and the device cost of the journal (#644) | | ~~**P1**~~ | ~~`add-operation-cancellation`~~ **landed 2026-08-24** | The third budget class had no exit: `mask_extrude` measures 4403 ms and `sdf_consolidate` 661 ms on the reference iPad, and a host could neither cancel one nor draw a progress bar — the threading rule forbids reading the document from another thread while it runs. `cancel()` is now the one call in the library safe from another thread, and the token carries progress the host POLLS rather than a callback the engine fires. A cancel is a DISCARD: the document is byte-identical afterwards, so a host never has to undo one. **The constraint that shaped it:** `parallel_for`'s join waits on `done >= num_tasks` and increments only after `fn` returns, so a cancelled chunk must return normally and never throw, or the join hangs forever | | ~~**P1**~~ | ~~Procedural masks~~ **landed 2026-08-24, reachable 2026-08-24** | Cheap on a field representation, high artist value — and it repeated surface groups' mistake exactly: curvature, cavity, convexity and normal-direction shipped in C++ with tests, no C entry point, no pyclay, and no change folder. `add-claycore-bridge` closed it, and moved the measure to a per-point form with the mask as one of its callers, so a cavity mask and a baked map cannot disagree about the same surface | | ~~**P1**~~ | ~~`add-item-spatial-index`~~ **closed 2026-09-02, archived 2026-09-05, merged 2026-09-06 (#470)** | **Measured 2026-08-24, and the implementation was REVERTED rather than shipped.** A median-split BVH made `plan()` 590x faster and the whole thing 2.4x SLOWER: build 2.584 -> 9.228 ms at 50 000 items against a query saving of 0.14 ms. The query really did become sublinear — per-item cost fell from a flat 2.8 ns to near zero across a 300x range — it is simply the smaller term. **The ratio that decides it is BUILD-TO-PLAN, and it is 1:1**: the index is cached on the document revision, every stamp bumps it, and `CullPlan` exists so one cull serves every brick in a dab. No tree amortises against that. The only remaining direction is incremental insertion, so the build is paid per EDIT rather than per document; the tests that would guard any index shipped without one. diff --git a/openspec/changes/make-voxel-sculpt-layer-creation-a-step/design.md b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/design.md new file mode 100644 index 00000000..00298b41 --- /dev/null +++ b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/design.md @@ -0,0 +1,54 @@ +# Design + +## One kind, not a third mechanism + +The edit's record half has the shape a merge-down's undo already has — truncate +a record to a known length and put back some overwritten `after` values — so a +Pass is a `SculptLayerOp` and replays through `apply_sculpt_layer_op`, the +`VoxelLayerProperty` step kind and its journal event. The field names are the +merge's (`lower_count`, `lower_afters`, `held.changes`) plus `pass_afters`, the +value each rewritten entry was left at, which a merge never needs because its +redo re-runs the fold. + +## How the record half is captured + +`VoxelGrid::set` is the choke point, and its recording hook is where a record +changes, so the capture lives there: a third channel beside the change sink, +null when off. A first touch appends and is recovered at close as the tail past +`lower_count`; a rewrite of an older entry notes `(index, previous after)`. At +close the notes are deduplicated keeping the FIRST (the value the edit found), +rewrites that ended where they began are dropped, and the new values are read. +The grid still knows nothing about steps; `History` decides. + +## Why a no-op edit's record entries are dropped + +The recording hook lists every cell a pass TOUCHED, including writes that +changed nothing, while a step exists only for an edit that changed a cell. The +first cut left those entries in the record when the step was dropped. That is +not benign: a Pass records positions, so after undoing an earlier Pass (which +truncates past them) and redoing it (which does not bring them back) every +later Pass found a record of a different length from the one it recorded and +was refused — and a journal, which never saw the dropped edit, refused on +replay. The test "an edit inside a layer that changed nothing leaves no trace" +fails exactly that way with the rollback removed. + +Recording the no-op edit as a step instead would be an undo that changes +nothing on screen, the defect every recorder here exists to avoid. So the edit +is dropped and its record entries are rolled back with it. With undo off +nothing captures, and the record lists them as before; `sculpt_layer_cell_count` +is documented as "how many cells the pass changed", which the undo-on behaviour +matches more closely, not less. + +## Recording state across undo and redo + +Undoing a creation must end recording — the layer being recorded into is gone. +Redo brings the layer back CLOSED rather than reopening it: recording is not +saved, it is a gesture's state, and `end_sculpt_layer` is not a step, so a redo +that reopened it would have a host that had already ended the pass refused at +its next `begin` ("a sculpt layer is already recording"). + +## Strictness + +A Pass replays only onto a record of exactly the length it left (undo) or found +(redo), and a Begin undoes only the top layer with an empty record. Anything +else is refused before a cell moves, like every other layer operation. diff --git a/openspec/changes/make-voxel-sculpt-layer-creation-a-step/proposal.md b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/proposal.md new file mode 100644 index 00000000..7bf42d9d --- /dev/null +++ b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/proposal.md @@ -0,0 +1,47 @@ +# Proposal: a voxel sculpt layer's creation, and every edit inside it, is a step + +## Why + +`unify-the-undo-history` made the five operations ON a voxel sculpt layer undo +steps, and left the layer itself outside the history (#642). +`clay_voxel_begin_sculpt_layer` / `VoxelGrid::begin_sculpt_layer` adds a record +to the grid's stack that the session never sees, and the edits made inside the +layer record their CELLS as ordinary Voxel steps and not what they did to that +record. Two defects follow, both reproduced on `main` before this change through +the C ABI (`tests/unit/test_c_voxel_layer_history.cpp`): + +| probe | on `main` | with this change | +|---|---|---| +| begin a layer, one inflate, undo: the layer's cell count | 110 | 0 | +| same, then dial the now-"empty" layer to 0.5: occupied cells vs before the inflate | 2251 vs 2197 — 54 undone cells put back | 2197 vs 2197 | +| same, document bytes after the undo vs before the inflate | differ | identical | +| snapshot, two passes and a dial, journal replayed onto the snapshot | refused after 2 of 3 events | all 5 applied, bytes identical | + +The mesh stack never had this gap: `MultiresSurface::add_sculpt_layer` fills a +`SculptLayerProperty` (Structural), so creation is a step there. + +## What changes + +- `SculptLayerOp` gains two APPENDED kinds. **Begin** is a layer's creation: + undo removes the top layer, redo puts it back. **Pass** is one edit made while + a layer was recording: the cells it wrote, exactly as a Voxel step carries + them, plus what it did to the layer's record — the length before, the entries + it rewrote with their old and new `after`, and the entries it appended. +- `begin_sculpt_layer` takes the same optional record out-parameter as its five + siblings, and both bindings pass one. +- `History::begin_voxel_step` also opens a *pass capture* on the grid; a step + that closes with the record changed is recorded as a `VoxelLayerProperty` + step (Pass) instead of a Voxel step. The journal carries it through the + existing `VoxelLayerProperty` event, so replay needs no new path. +- No entry point is added and no signature changes at the C ABI; the version + line does not move. The `SculptLayerOp` journal encoding gains one field; it + has not shipped in a release (it arrived after 0.120.0), so no journal on disk + carries the old layout. + +## Host-visible consequences + +- A pass inside a new layer is two undos: the edit, then the layer. +- Undoing a creation ends the recording; redoing it brings the layer back + closed. +- With undo on, an edit inside a layer that changed no cell leaves the layer's + record as it found it (see design.md for why). diff --git a/openspec/changes/make-voxel-sculpt-layer-creation-a-step/specs/scene-model/spec.md b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/specs/scene-model/spec.md new file mode 100644 index 00000000..fc5acb5d --- /dev/null +++ b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/specs/scene-model/spec.md @@ -0,0 +1,32 @@ +## ADDED Requirements + +### Requirement: A voxel sculpt layer's creation and the edits inside it are steps +Creating a voxel sculpt layer SHALL be a history step: undoing it SHALL remove the layer from the stack and end any recording into it, and redoing it SHALL restore the layer, with its name and dither seed, without reopening the recording. + +An edit made while a voxel sculpt layer is recording SHALL be one step that carries both the cells it wrote and what it did to the layer's record of the pass. Undoing it SHALL restore the cells and the record together, so that the grid and its stack are byte-identical to their state before the edit and a later change to the layer's strength or visibility SHALL NOT re-apply the undone cells. + +A journal replayed onto a snapshot taken before a voxel sculpt layer existed SHALL rebuild the layer and its passes, and SHALL NOT be refused at an operation that names it. + +An edit inside a voxel sculpt layer that changed no cell SHALL NOT be a step, and SHALL leave the layer's record as it found it. + +#### Scenario: Undoing an edit takes it out of the layer +- **GIVEN** a document with undo enabled and a voxel sculpt layer recording +- **WHEN** an inflate is made inside the layer and the history is undone once +- **THEN** the layer's cell count is zero and the document is byte-identical to its state before the inflate +- **AND** setting the layer's strength to 0.5 afterwards leaves the occupied cell count unchanged + +#### Scenario: Undoing a creation removes the layer +- **GIVEN** a document with undo enabled +- **WHEN** a voxel sculpt layer is created, a pass is made inside it, and the history is undone twice +- **THEN** the grid has no sculpt layers and the document is byte-identical to its state before the creation +- **AND** redoing twice returns the layer and its pass byte-identically + +#### Scenario: A journal rebuilds a layer the snapshot lacks +- **GIVEN** a snapshot taken before any voxel sculpt layer existed +- **WHEN** two layers are created with a pass each, one is dialled, and the journal since the snapshot is replayed onto it +- **THEN** every event applies and the recovered document is byte-identical to the live one + +#### Scenario: A no-op edit inside a layer leaves no trace +- **GIVEN** a voxel sculpt layer recording, with undo enabled +- **WHEN** an edit that changes no cell is made inside it +- **THEN** the undo depth does not change and the grid and its stack are byte-identical to their state before the edit diff --git a/openspec/changes/make-voxel-sculpt-layer-creation-a-step/tasks.md b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/tasks.md new file mode 100644 index 00000000..ff93c916 --- /dev/null +++ b/openspec/changes/make-voxel-sculpt-layer-creation-a-step/tasks.md @@ -0,0 +1,34 @@ +# Tasks: make-voxel-sculpt-layer-creation-a-step + +## 1. Reproduce + +- [x] 1.1 On `main`, through the C ABI: undo a dab inside a layer and read the + layer's cell count (110, not 0); dial the layer and count occupied cells + (54 undone cells put back); replay a journal onto a snapshot older than + the layer (refused after 2 of 3 events) + +## 2. Build + +- [x] 2.1 `SculptLayerOp::Kind::Begin` and `Kind::Pass`, appended; `pass_afters` + in the struct and the journal encoding; `op_fits` checks both +- [x] 2.2 `begin_sculpt_layer(name, record)`; the C and Python bindings record it +- [x] 2.3 `VoxelGrid::begin_pass_capture` / `end_pass_capture`, fed by the + recording hook's rewrite branch +- [x] 2.4 `History::begin_voxel_step` / `end_voxel_step` open and close the + capture; a changed record makes the step a `VoxelLayerProperty` Pass; a + step with no changed cell rolls its record entries back +- [x] 2.5 The pinned journal test now snapshots BEFORE the layers exist + +## 3. Verify + +- [x] 3.1 Regression tests, failing on `main` and passing here: + `test_c_voxel_layer_history.cpp` (three cases) and + `test_voxel_layer_history.cpp` (five cases plus the Pass encoding) and + `bindings/python/tests/test_voxel_layer_history.py` (one case) +- [x] 3.2 The rollback's own regression fails with the rollback removed +- [x] 3.3 Full unit suite; `release_check.py --skip-slow` + +## 4. Document + +- [x] 4.1 `bindings/c/clay.h` undo notes; `docs/05` "What is NOT covered" +- [x] 4.2 No ABI version move: no entry point added, no signature changed diff --git a/src/session/history.cpp b/src/session/history.cpp index de9fbdbc..74b06665 100644 --- a/src/session/history.cpp +++ b/src/session/history.cpp @@ -182,18 +182,42 @@ bool History::begin_voxel_step(scene::LayerId layer, voxel::VoxelGrid& grid) { open_layer_ = layer; voxel_open_ = true; grid.set_change_sink(&open_cells_); + // And, while a sculpt layer is recording, what the edit does to its record + // (#642). Left empty when none is. + grid.begin_pass_capture(&open_pass_); return true; } void History::end_voxel_step(voxel::VoxelGrid& grid) { if (!voxel_open_) return; grid.set_change_sink(nullptr); + grid.end_pass_capture(); voxel_open_ = false; // A step that changed no cell is dropped. A dab that misses every cell is // ordinary here — a sub-cell grab, a flatten meeting a flat region, a // dithered stamp — and recording one would be an undo that does nothing, // which is the thing this whole change exists to remove. - if (open_cells_.empty()) return; + // + // Inside a sculpt layer such a dab still lists the cells it touched in the + // layer's record, and dropping the step would leave those entries where no + // later step expects them: every pass record after it would name a length + // the record no longer has, and its undo, redo and journal replay would + // all be refused. So the record is put back as the dab found it. A dab + // that changed nothing is not part of the pass either. + if (open_cells_.empty()) { + if (!open_pass_.empty()) grid.apply_sculpt_layer_op(open_pass_, /*forward=*/false); + open_pass_ = {}; + return; + } + // Inside a layer the edit is a pass: its cells AND its record, as one + // layer operation, so undoing it takes the cells out of the layer too. + if (!open_pass_.empty()) { + open_pass_.cells = std::move(open_cells_); + open_cells_.clear(); + record_voxel_layer_property(open_layer_, std::move(open_pass_)); + open_pass_ = {}; + return; + } Step step; step.kind = Step::Kind::Voxel; step.layer = open_layer_; diff --git a/src/voxel/grid.cpp b/src/voxel/grid.cpp index 5c5fb389..eedbafed 100644 --- a/src/voxel/grid.cpp +++ b/src/voxel/grid.cpp @@ -289,6 +289,12 @@ void VoxelGrid::set(VoxelCoord c, std::uint8_t index) { rec.index.emplace(c, rec.changes.size()); rec.changes.push_back({c, get(c), index}); } else { + // A rewrite of an entry older than the open pass capture is what + // an undo of this edit has to put back (#642). Entries the edit + // itself appended are not noted: truncating removes them whole. + if (pass_capture_ && it->second < pass_capture_->lower_count) + pass_capture_->lower_afters.emplace_back(static_cast(it->second), + rec.changes[it->second].after); rec.changes[it->second].after = index; } } @@ -321,11 +327,20 @@ void VoxelGrid::set(VoxelCoord c, std::uint8_t index) { // -- sculpt layers ----------------------------------------------------------- -std::size_t VoxelGrid::begin_sculpt_layer(std::string name) { +std::size_t VoxelGrid::begin_sculpt_layer(std::string name, SculptLayerOp* record) { end_sculpt_layer(); + // A capture names the layer it opened on; a new top layer is not it. + end_pass_capture(); SculptLayerRecord rec; rec.name = std::move(name); rec.seed = next_sculpt_seed_++; + if (record) { + *record = SculptLayerOp{}; + record->kind = SculptLayerOp::Kind::Begin; + record->layer = sculpt_layers_.size(); + record->held.name = rec.name; + record->held.seed = rec.seed; + } sculpt_layers_.push_back(std::move(rec)); recording_ = true; return sculpt_layers_.size() - 1; diff --git a/src/voxel/sculpt_layer_op.cpp b/src/voxel/sculpt_layer_op.cpp index 2fdc4a37..96e9fa21 100644 --- a/src/voxel/sculpt_layer_op.cpp +++ b/src/voxel/sculpt_layer_op.cpp @@ -1,10 +1,13 @@ -// A sculpt-layer operation as an undo step (unify-the-undo-history 3.3, 3.4). +// A sculpt-layer operation as an undo step (unify-the-undo-history 3.3, 3.4), +// including a layer's creation and each edit made inside it (#642). // // The operations themselves live beside the rest of the stack in grid.cpp; // this file holds what makes one REPLAYABLE — the apply in either direction, // the data a removal or a merge-down has to keep, and the journal encoding. +#include #include +#include #include #include "clay/bytes.h" @@ -37,13 +40,17 @@ VoxelGrid::SculptLayerRecord VoxelGrid::from_data(SculptLayerData data) { return rec; } +// Drop a record's entries from `count` on, and the lookup entries naming them. +void VoxelGrid::truncate_record(SculptLayerRecord& rec, std::size_t count) { + for (std::size_t i = count; i < rec.changes.size(); ++i) rec.index.erase(rec.changes[i].cell); + rec.changes.resize(count); +} + // The inverse of fold_down: truncate the lower layer to what it held, put back // the `after` values the fold overwrote, and reinsert the upper layer. void VoxelGrid::unfold_down(const SculptLayerOp& op) { SculptLayerRecord& lower = sculpt_layers_[op.layer - 1]; - for (std::size_t i = op.lower_count; i < lower.changes.size(); ++i) - lower.index.erase(lower.changes[i].cell); - lower.changes.resize(op.lower_count); + truncate_record(lower, op.lower_count); for (std::size_t k = op.lower_afters.size(); k > 0; --k) { const auto& [at, after] = op.lower_afters[k - 1]; lower.changes[at].after = after; @@ -52,6 +59,70 @@ void VoxelGrid::unfold_down(const SculptLayerOp& op) { from_data(op.held)); } +// -- a creation and a pass (#642) -------------------------------------------- + +void VoxelGrid::begin_pass_capture(SculptLayerOp* record) { + pass_capture_ = nullptr; + if (!record) return; + *record = SculptLayerOp{}; + if (!recording_) return; + record->layer = sculpt_layers_.size() - 1; + record->lower_count = sculpt_layers_.back().changes.size(); + pass_capture_ = record; +} + +void VoxelGrid::end_pass_capture() { + SculptLayerOp* op = pass_capture_; + pass_capture_ = nullptr; + // The layer shrank or vanished under the capture — nothing a binding does + // inside one edit — so there is no record left to describe. + if (!op || op->layer >= sculpt_layers_.size() || + sculpt_layers_[op->layer].changes.size() < op->lower_count) + return; + const SculptLayerRecord& rec = sculpt_layers_[op->layer]; + // A cell rewritten twice was noted twice; the FIRST note holds the value + // the edit found, which is the one an undo restores. And a rewrite that + // ended where it started is not part of the record's change at all. + std::vector> noted = std::move(op->lower_afters); + op->lower_afters.clear(); + std::unordered_set seen; + for (const auto& [at, was] : noted) { + if (!seen.insert(at).second || rec.changes[at].after == was) continue; + op->lower_afters.emplace_back(at, was); + op->pass_afters.emplace_back(at, rec.changes[at].after); + } + op->held.changes.assign(rec.changes.begin() + static_cast(op->lower_count), + rec.changes.end()); + if (!op->held.changes.empty() || !op->lower_afters.empty()) op->kind = SculptLayerOp::Kind::Pass; +} + +void VoxelGrid::redo_begin(const SculptLayerOp& op) { + sculpt_layers_.push_back(from_data(op.held)); + // A replayed journal rebuilds layers this grid never made, and the next + // layer it does make must not reuse one of their seeds. + next_sculpt_seed_ = std::max(next_sculpt_seed_, op.held.seed + 1); +} + +void VoxelGrid::undo_begin() { + sculpt_layers_.pop_back(); + recording_ = false; // what it was recording into is gone +} + +void VoxelGrid::redo_pass(const SculptLayerOp& op) { + SculptLayerRecord& rec = sculpt_layers_[op.layer]; + for (const auto& [at, after] : op.pass_afters) rec.changes[at].after = after; + for (const SculptChange& ch : op.held.changes) { + rec.index.emplace(ch.cell, rec.changes.size()); + rec.changes.push_back(ch); + } +} + +void VoxelGrid::undo_pass(const SculptLayerOp& op) { + SculptLayerRecord& rec = sculpt_layers_[op.layer]; + truncate_record(rec, op.lower_count); + for (const auto& [at, was] : op.lower_afters) rec.changes[at].after = was; +} + namespace { // Undoing a merge restores the lower layer from its recorded length and @@ -65,12 +136,35 @@ bool merge_undo_fits(const VoxelGrid::SculptLayerOp& op, std::size_t count, return true; } +// A pass replays onto a record of EXACTLY the length it left or found — not +// merely a long enough one, because a record that grew or shrank outside the +// history is one whose positions no longer mean what this record says. +bool pass_fits(const VoxelGrid::SculptLayerOp& op, bool forward, std::size_t count, + std::size_t own_size) { + if (op.layer >= count || op.lower_afters.size() != op.pass_afters.size()) return false; + const std::size_t expected = forward ? op.lower_count : op.lower_count + op.held.changes.size(); + if (own_size != expected) return false; + for (const auto& entry : op.lower_afters) + if (entry.first >= op.lower_count) return false; + for (const auto& entry : op.pass_afters) + if (entry.first >= op.lower_count) return false; + return true; +} + +// The stack's shape as op_fits reads it: its length, and the lengths of the +// record an operation names and of the one below it (zero where absent). +struct StackShape { + std::size_t count = 0; + std::size_t lower_size = 0; + std::size_t own_size = 0; +}; + // Whether the stack has the shape `op` names, in the direction it is about to // run. Checked before anything is written, so a refusal leaves the grid as it // was rather than half-replayed. -bool op_fits(const VoxelGrid::SculptLayerOp& op, bool forward, std::size_t count, - std::size_t lower_size) { +bool op_fits(const VoxelGrid::SculptLayerOp& op, bool forward, const StackShape& shape) { using Kind = VoxelGrid::SculptLayerOp::Kind; + const std::size_t count = shape.count; switch (op.kind) { case Kind::Strength: case Kind::Visible: @@ -82,7 +176,15 @@ bool op_fits(const VoxelGrid::SculptLayerOp& op, bool forward, std::size_t count return forward ? op.layer < count : op.layer <= count; case Kind::Merge: if (op.layer == 0) return false; - return forward ? op.layer < count : merge_undo_fits(op, count, lower_size); + return forward ? op.layer < count : merge_undo_fits(op, count, shape.lower_size); + case Kind::Begin: + // Always the top: a redo appends one, and an undo removes the one + // it appended, which must have nothing left in its record — every + // pass inside it is a later step and is undone first. + return forward ? op.layer == count + : count > 0 && op.layer == count - 1 && shape.own_size == 0; + case Kind::Pass: + return pass_fits(op, forward, count, shape.own_size); case Kind::None: return false; } @@ -109,6 +211,12 @@ void VoxelGrid::redo_layer_property(const SculptLayerOp& op) { case SculptLayerOp::Kind::Merge: fold_down(op.layer, nullptr); break; + case SculptLayerOp::Kind::Begin: + redo_begin(op); + break; + case SculptLayerOp::Kind::Pass: + redo_pass(op); + break; case SculptLayerOp::Kind::None: break; } @@ -133,16 +241,24 @@ void VoxelGrid::undo_layer_property(const SculptLayerOp& op) { case SculptLayerOp::Kind::Merge: unfold_down(op); break; + case SculptLayerOp::Kind::Begin: + undo_begin(); + break; + case SculptLayerOp::Kind::Pass: + undo_pass(op); + break; case SculptLayerOp::Kind::None: break; } } bool VoxelGrid::apply_sculpt_layer_op(const SculptLayerOp& op, bool forward) { - const std::size_t count = sculpt_layers_.size(); - const bool has_lower = op.layer >= 1 && op.layer - 1 < count; - const std::size_t lower_size = has_lower ? sculpt_layers_[op.layer - 1].changes.size() : 0; - if (!op_fits(op, forward, count, lower_size)) return false; + StackShape shape; + shape.count = sculpt_layers_.size(); + if (op.layer >= 1 && op.layer - 1 < shape.count) + shape.lower_size = sculpt_layers_[op.layer - 1].changes.size(); + if (op.layer < shape.count) shape.own_size = sculpt_layers_[op.layer].changes.size(); + if (!op_fits(op, forward, shape)) return false; // Undo restores the cells FIRST, while the stack still has the shape the // operation left it in, and then the property; redo runs the other way. // Neither order matters to the cells — a replay writes by coordinate — but @@ -159,7 +275,7 @@ bool VoxelGrid::apply_sculpt_layer_op(const SculptLayerOp& op, bool forward) { std::size_t VoxelGrid::SculptLayerOp::bytes() const { return sizeof(SculptLayerOp) + held.name.capacity() + vector_bytes(held.changes) + - vector_bytes(lower_afters) + vector_bytes(cells); + vector_bytes(lower_afters) + vector_bytes(pass_afters) + vector_bytes(cells); } // -- the journal encoding ---------------------------------------------------- @@ -171,6 +287,7 @@ std::size_t VoxelGrid::SculptLayerOp::bytes() const { // f32 strength_before f32 strength_after u8 visible_before u8 visible_after // held: u32 name_len name f32 strength u8 visible u32 seed changes // u32 lower_count u32 n n x (u32 index, u8 after) +// u32 n n x (u32 index, u8 after) pass_afters // cells // where `changes` and `cells` are u32 n n x (i32 x, i32 y, i32 z, u8 before, u8 after). namespace { @@ -197,6 +314,15 @@ void put_changes(std::vector& out, } } +void put_afters(std::vector& out, + const std::vector>& afters) { + put_u32(out, static_cast(afters.size())); + for (const auto& [at, after] : afters) { + put_u32(out, at); + out.push_back(after); + } +} + struct Reader { const std::uint8_t* data; std::size_t size; @@ -243,6 +369,16 @@ struct Reader { } return ok; } + bool afters(std::vector>* out) { + std::uint32_t n = 0; + if (!count(5, &n)) return false; + out->resize(n); + for (auto& [at, after] : *out) { + at = u32(); + after = u8(); + } + return ok; + } }; } // namespace @@ -264,11 +400,8 @@ std::vector VoxelGrid::SculptLayerOp::encode() const { put_u32(out, held.seed); put_changes(out, held.changes); put_u32(out, static_cast(lower_count)); - put_u32(out, static_cast(lower_afters.size())); - for (const auto& [at, after] : lower_afters) { - put_u32(out, at); - out.push_back(after); - } + put_afters(out, lower_afters); + put_afters(out, pass_afters); put_changes(out, cells); return out; } @@ -280,7 +413,7 @@ bool VoxelGrid::SculptLayerOp::decode(const std::uint8_t* data, std::size_t size if (r.u8() != kOpVersion) return false; SculptLayerOp op; const std::uint8_t kind = r.u8(); - if (kind == 0 || kind > static_cast(Kind::Merge)) return false; + if (kind == 0 || kind > static_cast(Kind::Pass)) return false; op.kind = static_cast(kind); op.layer = r.u32(); op.to = r.u32(); @@ -297,13 +430,7 @@ bool VoxelGrid::SculptLayerOp::decode(const std::uint8_t* data, std::size_t size op.held.seed = r.u32(); if (!r.changes(&op.held.changes)) return false; op.lower_count = r.u32(); - std::uint32_t afters = 0; - if (!r.count(5, &afters)) return false; - op.lower_afters.resize(afters); - for (auto& [at, after] : op.lower_afters) { - at = r.u32(); - after = r.u8(); - } + if (!r.afters(&op.lower_afters) || !r.afters(&op.pass_afters)) return false; if (!r.changes(&op.cells)) return false; // Trailing bytes are a record this build did not write. if (!r.ok || r.pos != size) return false; diff --git a/tests/unit/test_c_voxel_layer_history.cpp b/tests/unit/test_c_voxel_layer_history.cpp index 229b38cb..c65b9f9a 100644 --- a/tests/unit/test_c_voxel_layer_history.cpp +++ b/tests/unit/test_c_voxel_layer_history.cpp @@ -97,8 +97,34 @@ struct Doc { REQUIRE(clay_voxel_sculpt_layer_cell_count(grid(), layer, &n) == CLAY_OK); return n; } + size_t occupied() { + size_t n = 0; + REQUIRE(clay_voxel_occupied_count(grid(), &n) == CLAY_OK); + return n; + } + size_t journal_next() const { + size_t next = 0; + REQUIRE(clay_document_journal_range(d, nullptr, &next) == CLAY_OK); + return next; + } + Bytes journal_since(size_t from) const { + clay_blob* blob = nullptr; + REQUIRE(clay_document_journal_since(d, from, &blob, nullptr) == CLAY_OK); + Bytes out(clay_blob_data(blob), clay_blob_data(blob) + clay_blob_size(blob)); + clay_blob_destroy(blob); + return out; + } }; +// A document's whole bytes, read the way Doc::bytes reads them. +Bytes bytes_of(const clay_document* d) { + clay_blob* blob = nullptr; + REQUIRE(clay_document_save_memory(d, &blob) == CLAY_OK); + Bytes out(clay_blob_data(blob), clay_blob_data(blob) + clay_blob_size(blob)); + clay_blob_destroy(blob); + return out; +} + } // namespace TEST_CASE("c abi: undoing a sculpt-layer dial restores the dial, not the pass") { @@ -109,11 +135,11 @@ TEST_CASE("c abi: undoing a sculpt-layer dial restores the dial, not the pass") doc.pass("wrinkles", 6, 2); const Bytes full = doc.bytes(); const std::size_t cells = doc.layer_cells(0); - REQUIRE(doc.depth() == 1); + REQUIRE(doc.depth() == 2); // the layer's creation, and the pass inside it REQUIRE(clay_voxel_set_sculpt_layer_strength(doc.grid(), 0, 0.4f) == CLAY_OK); const Bytes dialled = doc.bytes(); - CHECK(doc.depth() == 2); + CHECK(doc.depth() == 3); CHECK(doc.undo()); CHECK(doc.strength(0) == 1.0f); @@ -161,6 +187,89 @@ TEST_CASE("c abi: visibility, reorder and removal are each one step") { CHECK(doc.depth() == base + 3); } +// -- creating a sculpt layer is a step (#642) -------------------------------- + +TEST_CASE("c abi: undoing a pass inside a sculpt layer takes it out of the layer") { + // Regression (#642). The pass's cells were a step and the layer's record of + // them was not, so the undo reverted the cells and left the record listing + // them — and the next dial recomposed the layer and put the undone cells + // back, at the dithered fraction. + Doc doc(true); + size_t index = 0; + REQUIRE(clay_voxel_begin_sculpt_layer(doc.grid(), "wrinkles", &index) == CLAY_OK); + const Bytes opened = doc.bytes(); + const std::size_t cells = doc.occupied(); + clay_brush_params brush{}; + brush.struct_size = sizeof(brush); + brush.size = 9; + brush.shape = 1; /* sphere */ + brush.strength = 1.0f; + const std::int32_t at[3] = {0, 6, 0}; + REQUIRE(clay_voxel_sculpt_inflate(doc.grid(), at, &brush, 2) == CLAY_OK); + REQUIRE(doc.layer_cells(0) > 0); + REQUIRE(doc.occupied() > cells); + + CHECK(doc.undo()); + CHECK(doc.layer_cells(0) == 0); + CHECK(doc.bytes() == opened); + // The half a host could see: dialling the now-empty layer moves nothing. + REQUIRE(clay_voxel_set_sculpt_layer_strength(doc.grid(), 0, 0.5f) == CLAY_OK); + CHECK(doc.occupied() == cells); + // Still recording: undoing a dab mid-pass does not end the pass. + int32_t recording = 0; + REQUIRE(clay_voxel_recording_sculpt_layer(doc.grid(), &recording) == CLAY_OK); + CHECK(recording == 1); +} + +TEST_CASE("c abi: undoing a sculpt layer's creation removes it, and redo brings the pass back") { + Doc doc(true); + const Bytes start = doc.bytes(); + doc.pass("wrinkles", 6, 2); + const Bytes made = doc.bytes(); + const std::size_t cells = doc.layer_cells(0); + REQUIRE(doc.depth() == 2); + + CHECK(doc.undo()); // the pass + CHECK(doc.undo()); // the layer + CHECK(doc.layers() == 0); + CHECK(doc.bytes() == start); + CHECK_FALSE(doc.undo()); + + CHECK(doc.redo()); + CHECK(doc.layers() == 1); + CHECK(doc.layer_cells(0) == 0); + CHECK(doc.redo()); + CHECK(doc.layer_cells(0) == cells); + CHECK(doc.bytes() == made); +} + +TEST_CASE("c abi: a journal replayed onto a snapshot older than the layer rebuilds the stack") { + // Regression (#642). Creation was not an event, so a snapshot taken before + // the layer existed rebuilt the pass's cells and not the layer, and the + // replay was refused at the first dial naming it. + Doc doc(true); + const Bytes snapshot = doc.bytes(); + const std::size_t from = doc.journal_next(); + doc.pass("lower", 6, 2); + doc.pass("upper", 8, -1); + REQUIRE(clay_voxel_set_sculpt_layer_strength(doc.grid(), 1, 0.3f) == CLAY_OK); + const Bytes journal = doc.journal_since(from); + const std::size_t next = doc.journal_next(); + REQUIRE(next - from == 5); // two creations, two passes, the dial + + clay_document* recovered = nullptr; + REQUIRE(clay_document_load_memory(snapshot.data(), snapshot.size(), &recovered) == CLAY_OK); + REQUIRE(clay_document_enable_undo(recovered) == CLAY_OK); + std::size_t applied = 0; + int32_t stopped = -1; + CHECK(clay_document_replay_journal(recovered, journal.data(), journal.size(), &applied, + &stopped) == CLAY_OK); + CHECK(applied == next - from); + CHECK(stopped == 0); + CHECK(bytes_of(recovered) == doc.bytes()); + clay_document_destroy(recovered); +} + // -- enabling undo mid-session (2.4) ----------------------------------------- TEST_CASE("c abi: enabling undo mid-session starts an empty history") { diff --git a/tests/unit/test_voxel_layer_history.cpp b/tests/unit/test_voxel_layer_history.cpp index 0b0a9cb2..c81146d3 100644 --- a/tests/unit/test_voxel_layer_history.cpp +++ b/tests/unit/test_voxel_layer_history.cpp @@ -1,5 +1,6 @@ // A voxel sculpt-layer operation is an undo step (unify-the-undo-history 3.3, -// 3.4, 4.4), and a bracket only folds what it opened (2.4). +// 3.4, 4.4), and so are a layer's creation and each edit made inside it +// (#642); a bracket only folds what it opened (2.4). // // The claim is EXACT, not close. A layer operation's step carries the property // it changed AND every cell its recompose wrote, so an undo restores the grid @@ -71,17 +72,27 @@ struct World { bool undo() { return h.undo(doc, grid_for(), mesh_for()); } bool redo() { return h.redo(doc, grid_for(), mesh_for()); } - // One sculpt-layer pass, recorded the way both bindings record a verb: the - // pass joins the artist's layer AND becomes one undo step. - // `also`, when given, is a second inflate inside the same pass. + // One sculpt-layer pass, recorded the way both bindings record it: the + // layer's creation is one undo step, and the edit inside it another. + // `also`, when given, is a second inflate inside the same edit. void pass(const char* name, voxel::VoxelCoord at, int amount, std::optional also = std::nullopt) { - grid.begin_sculpt_layer(name); + open(name); + inflate(at, amount, also); + grid.end_sculpt_layer(); + } + void open(const char* name) { + VoxelGrid::SculptLayerOp begin; + grid.begin_sculpt_layer(name, &begin); + record(begin); + } + // One bracketed edit, the way both bindings bracket a verb. + void inflate(voxel::VoxelCoord at, int amount, + std::optional also = std::nullopt) { REQUIRE(h.begin_voxel_step(voxel_layer, grid)); grid.sculpt_inflate(at, dab(9), amount); if (also) grid.sculpt_inflate(*also, dab(9), 2); h.end_voxel_step(grid); - grid.end_sculpt_layer(); } void record(const VoxelGrid::SculptLayerOp& op) { h.record_voxel_layer_property(voxel_layer, op); } }; @@ -107,7 +118,7 @@ TEST_CASE("voxel layer history: a strength change is its own step, and undoes bi World w; w.pass("wrinkles", {0, 6, 0}, 2); const Bytes full = w.grid.serialize(); - REQUIRE(w.h.undo_depth() == 1); + REQUIRE(w.h.undo_depth() == 2); // the layer's creation, and the pass VoxelGrid::SculptLayerOp op; REQUIRE(w.grid.set_sculpt_layer_strength(0, 0.4f, &op)); @@ -115,7 +126,7 @@ TEST_CASE("voxel layer history: a strength change is its own step, and undoes bi CHECK_FALSE(op.cells.empty()); // the dial really moved cells w.record(op); const Bytes dialled = w.grid.serialize(); - CHECK(w.h.undo_depth() == 2); // the pass, and the dial + CHECK(w.h.undo_depth() == 3); // the creation, the pass, and the dial round_trip(w, full, dialled); // The undo took the DIAL, not the pass: one undo leaves the layer present, @@ -219,7 +230,7 @@ TEST_CASE("voxel layer history: an operation that changed nothing is not a step" CHECK(onto_itself.empty()); w.record(same); w.record(onto_itself); - CHECK(w.h.step_count() == 1); + CHECK(w.h.step_count() == 2); // the creation and the pass, nothing more } TEST_CASE("voxel layer history: a step whose stack changed shape is refused, not misapplied") { @@ -236,16 +247,15 @@ TEST_CASE("voxel layer history: a step whose stack changed shape is refused, not } TEST_CASE("voxel layer history: a dialled pass replays from the journal") { - // The snapshot already holds both layers. Creating a sculpt layer is not a - // history event — a journal replayed onto a snapshot taken BEFORE the - // layers existed rebuilds their cells and not the stack, and refuses the - // first operation that names a layer it does not have — so the claim here - // is about the operations, replayed onto the stack they were made on. + // The snapshot is taken BEFORE either layer exists. Until creation was a + // history event (#642) this replay rebuilt the passes' cells and not the + // stack, and refused the dial — the first event naming a layer — so this + // test used to snapshot after the layers and claim only the operations. World w; - w.pass("lower", {0, 6, 0}, 2); - w.pass("upper", {0, 7, 0}, -1); const Bytes snapshot = w.grid.serialize(); const std::size_t from = w.h.journal_next(); + w.pass("lower", {0, 6, 0}, 2); + w.pass("upper", {0, 7, 0}, -1); VoxelGrid::SculptLayerOp dial, merge; REQUIRE(w.grid.set_sculpt_layer_strength(1, 0.3f, &dial)); w.record(dial); @@ -259,12 +269,128 @@ TEST_CASE("voxel layer history: a dialled pass replays from the journal") { r.grid = std::move(*restored); session::History::ReplayResult result; REQUIRE(r.h.replay(journal.data(), journal.size(), r.doc, r.grid_for(), r.mesh_for(), &result)); - CHECK(result.applied == 2); + CHECK(result.applied == 6); // two creations, two passes, the dial, the merge CHECK(r.grid.serialize() == w.grid.serialize()); // And the rebuilt steps undo like the live ones: the merge, then the dial. REQUIRE(r.undo()); REQUIRE(r.undo()); CHECK(r.grid.sculpt_layer_strength(1) == 1.0f); + // ...and on, back through both passes and both creations to the snapshot. + for (int i = 0; i < 4; ++i) REQUIRE(r.undo()); + CHECK(r.grid.sculpt_layer_count() == 0); + CHECK(r.grid.serialize() == snapshot); +} + +// -- a layer's creation, and the edits inside it (#642) ----------------------- + +TEST_CASE("voxel layer history: undoing an edit inside a layer takes it out of the record") { + // Regression (#642). The edit's cells were a step and the layer's record of + // them was not: the undo reverted the cells and left the record listing + // them, so the next dial recomposed the layer and put them back. + World w; + w.open("wrinkles"); + const Bytes opened = w.grid.serialize(); + const std::size_t occupied = w.grid.occupied_count(); + w.inflate({0, 6, 0}, 2); + REQUIRE(w.grid.sculpt_layer_cell_count(0) > 0); + const Bytes inflated = w.grid.serialize(); + + REQUIRE(w.undo()); + CHECK(w.grid.sculpt_layer_cell_count(0) == 0); + CHECK(w.grid.serialize() == opened); + CHECK(w.grid.recording_sculpt_layer()); // an undone dab does not end the pass + REQUIRE(w.redo()); + CHECK(w.grid.serialize() == inflated); + + REQUIRE(w.undo()); + REQUIRE(w.grid.set_sculpt_layer_strength(0, 0.5f)); + CHECK(w.grid.occupied_count() == occupied); +} + +TEST_CASE("voxel layer history: an edit that rewrites a pass's own cells undoes bit-exact") { + // The second edit lands on cells the first already put in the record, so + // its undo has to restore the `after` those entries held, not only + // truncate what it appended. + World w; + w.open("pass"); + w.inflate({0, 6, 0}, 2); + const Bytes first = w.grid.serialize(); + const std::size_t cells = w.grid.sculpt_layer_cell_count(0); + w.inflate({0, 7, 0}, -1, voxel::VoxelCoord{5, 3, 0}); + const Bytes second = w.grid.serialize(); + REQUIRE(second != first); + + round_trip(w, first, second); + REQUIRE(w.undo()); + CHECK(w.grid.sculpt_layer_cell_count(0) == cells); + CHECK(w.grid.serialize() == first); +} + +TEST_CASE("voxel layer history: an edit inside a layer that changed nothing leaves no trace") { + // Regression for the fix's own first cut. A dab that changes no cell is + // dropped rather than recorded, but inside a layer it still lists the cells + // it touched; left there, every later edit's record named a length the + // record no longer had once an earlier step was undone, and its redo — and + // its journal replay, which never saw the dropped dab — was refused. + World w; + const Bytes snapshot = w.grid.serialize(); + const std::size_t from = w.h.journal_next(); + w.open("pass"); + w.inflate({0, 6, 0}, 2); + const Bytes first = w.grid.serialize(); + const std::size_t depth = w.h.undo_depth(); + + // Painting a cell the index it already holds changes nothing. + REQUIRE(w.h.begin_voxel_step(w.voxel_layer, w.grid)); + w.grid.set({0, 0, 0}, w.grid.get({0, 0, 0})); + w.h.end_voxel_step(w.grid); + CHECK(w.h.undo_depth() == depth); + CHECK(w.grid.serialize() == first); + + w.inflate({6, 0, 0}, 2); + const Bytes last = w.grid.serialize(); + REQUIRE(w.undo()); + REQUIRE(w.undo()); + REQUIRE(w.redo()); + REQUIRE(w.redo()); + CHECK(w.grid.serialize() == last); + + World r; + std::optional restored = VoxelGrid::deserialize(snapshot.data(), snapshot.size()); + REQUIRE(restored); + r.grid = std::move(*restored); + const Bytes journal = w.h.journal_since(from, nullptr); + session::History::ReplayResult result; + REQUIRE(r.h.replay(journal.data(), journal.size(), r.doc, r.grid_for(), r.mesh_for(), &result)); + CHECK(r.grid.serialize() == last); +} + +TEST_CASE("voxel layer history: undoing a creation removes the layer and ends its recording") { + World w; + const Bytes start = w.grid.serialize(); + w.open("wrinkles"); + REQUIRE(w.grid.recording_sculpt_layer()); + REQUIRE(w.undo()); + CHECK(w.grid.sculpt_layer_count() == 0); + CHECK_FALSE(w.grid.recording_sculpt_layer()); + CHECK(w.grid.serialize() == start); + // Redo brings the layer back CLOSED: recording is a gesture's state, and + // reopening it would refuse a host's next begin. + REQUIRE(w.redo()); + CHECK(w.grid.sculpt_layer_count() == 1); + CHECK(w.grid.sculpt_layer_name(0) == "wrinkles"); + CHECK_FALSE(w.grid.recording_sculpt_layer()); +} + +TEST_CASE("voxel layer history: a creation with a pass still in it is refused, not misapplied") { + World w; + w.pass("a", {0, 6, 0}, 2); + VoxelGrid::SculptLayerOp begin; + begin.kind = VoxelGrid::SculptLayerOp::Kind::Begin; + begin.layer = 0; + const Bytes now = w.grid.serialize(); + CHECK_FALSE(w.grid.apply_sculpt_layer_op(begin, /*forward=*/false)); + CHECK(w.grid.serialize() == now); } TEST_CASE("voxel layer history: the op survives its own encoding") { @@ -281,6 +407,22 @@ TEST_CASE("voxel layer history: the op survives its own encoding") { CHECK_FALSE(VoxelGrid::SculptLayerOp::decode(bytes.data(), bytes.size() - 1, &back)); } +TEST_CASE("voxel layer history: a pass op survives its own encoding") { + World w; + w.grid.begin_sculpt_layer("pass"); + w.grid.sculpt_inflate({0, 6, 0}, dab(9), 2); + VoxelGrid::SculptLayerOp op; + w.grid.begin_pass_capture(&op); + w.grid.sculpt_inflate({0, 7, 0}, dab(9), -1); + w.grid.end_pass_capture(); + REQUIRE(op.kind == VoxelGrid::SculptLayerOp::Kind::Pass); + REQUIRE_FALSE(op.pass_afters.empty()); // the rewrite half is exercised + const Bytes bytes = op.encode(); + VoxelGrid::SculptLayerOp back; + REQUIRE(VoxelGrid::SculptLayerOp::decode(bytes.data(), bytes.size(), &back)); + CHECK(back.encode() == bytes); +} + // -- brackets (2.4) ---------------------------------------------------------- TEST_CASE("session: an end with no open bracket folds nothing") {