Make a voxel sculpt layer's creation, and each edit inside it, an undo step

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

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;
 }

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")

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)

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.

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<voxel::VoxelGrid::SculptChange> 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;

include/clay/voxel/grid.h

@@ -200,9 +200,16 @@ class VoxelGrid {
     void reapply_changes(const std::vector<SculptChange>& 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<std::pair<std::uint32_t, std::uint8_t>> lower_afters;
+        // Pass only: parallel to lower_afters, the value each entry was left at.
+        std::vector<std::pair<std::uint32_t, std::uint8_t>> pass_afters;
         // Every cell the recompose wrote, in the order it wrote them.
         std::vector<SculptChange> 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,13 +980,21 @@ 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);
 
     std::vector<SculptLayerRecord> sculpt_layers_;
     // Not owned. Null is off; see set_change_sink.
     std::vector<SculptChange>* 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;