Skip to content

Record a session-fed multires gesture into a clay_multires_delta - #689

Merged
leonardoaraujosantos merged 2 commits into
mainfrom
fix/multires-stroke-tx-record
Oct 4, 2026
Merged

leonardoaraujosantos merged 2 commits into
mainfrom
fix/multires-stroke-tx-record

Conversation

@leonardoaraujosantos

@leonardoaraujosantos leonardoaraujosantos commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

clay_multires_sculptor_apply_stroke_tx (#688, ABI 0.126.0) took no clay_multires_delta. Every other way into a sculptor takes the record its whole-path call takes: clay_mesh_sculptor_apply_stroke_tx takes clay_mesh_deltas*, clay_dynamic_sculptor_apply_stroke_tx takes clay_dynamic_delta* record, and clay_multires_sculptor_apply_stroke_recorded (#687) takes clay_multires_delta* record. So a multires gesture fed through a session could not go onto a host's undo stack. A host that fed one pen gesture in pieces had to fall back to a clay_multires_serialize snapshot of the whole hierarchy, or to the whole-path call, which is what the session was built to replace. #688's body flagged this: adding the parameter costs nothing before 0.126.0 ships, and after that it needs a new entry point.

The signature change, in place

0.126.0 is not tagged (the latest tag is v0.120.1), so the existing entry point changes instead of gaining a _recorded twin:

clay_result clay_multires_sculptor_apply_stroke_tx(clay_multires_sculptor* sculptor,
                                                   clay_stroke_tx* tx,
                                                   const clay_mesh_brush_desc* brush,
                                                   const clay_mask* mask,
                                                   const clay_mesh_frame* mesh_to_world,
                                                   int32_t defer_normals,
                                                   clay_multires_delta* record,
                                                   size_t* out_applied,
                                                   clay_multires_stamp_report* out_report);

record sits after defer_normals, where _apply_stroke_recorded has it and where the mesh and adaptive consumers put theirs (just before out_applied). It may be NULL, which means no recording and the stamps are the same as before.

What the consumer now does with it, mirroring the adaptive consumer:

  • The record is part of the binding (bind_key(record), as clay_dynamic_sculptor_apply_stroke_tx has it). A later call that names another record, or NULL after a record, is CLAY_ERROR_INVALID_ARGUMENT. Otherwise half a gesture would be in one record and half in another, giving two undo steps that each restore a torn stroke.

  • The record is continued across calls. Each call hands both halves (base_half / layer_half) to MultiresStrokeGesture::apply and then binds the record, so one record across the gesture is one undo step. It holds the active pass's half too.

  • Refusals come before the session's stamps are taken. accept_multires_record runs before next_batch (skipped once the sink is closed, as on the adaptive consumer), so a CLAY_ERROR_SNAPSHOT_MISMATCH loses no stamps. A multires record does have a mismatch case mid-gesture: the host makes a different sculpt pass active while the record holds one (and a level change, which can't be undone in place). The report is also probed up front now (probe_multires_report), so a malformed report is refused before stamps are taken instead of after they were applied. This is the order _apply_stroke_recorded already promises.

  • Only a call that took stamps binds the record (review fix, second commit). The first version called bind on every call. A call after the close skips the record check, applies nothing and returns CLAY_OK, and it still re-stamped the record with the hierarchy's current structure_revision. Calling after every append is the intended rhythm, so a host could do this: close a recorded gesture, revert it, remove the top level and add it back, make one lagging call, then redo. The redo was accepted (CLAY_OK) instead of being refused with SNAPSHOT_MISMATCH, and replayed the record onto a hierarchy it does not describe. Now bind runs only when batch.stamps is non-empty, which is always after the check accepted the record. A call that takes no stamps needs no bind: an empty record has nothing to bind, and a non-empty one was bound by the call that captured into it.

  • A pass activated mid-gesture joins a base-only record. The two halves are separate storage, so the record then holds both and one revert takes both back. This is now stated in the header and tested.

clay.h declares typedef struct clay_multires_delta clay_multires_delta; above the consumer, which is now the first call that names the type. The later typedef is turned into a comment pointing back to it, so the header is still valid C99 (checked with gcc-16 -std=c99 -pedantic -Werror).

Tests, and proof they failed before

New cases in tests/unit/test_c_multires_delta.cpp, through the C ABI only:

  • A session-fed stroke records the whole-path stroke's record. A 24-sample drag fed in batches of 4, with one record, on a hierarchy with an active sculpt pass and on one without. Several calls write into the record (asserted). Positions on every level, the detail checksum and the sculpt-layer checksum equal clay_multires_sculptor_apply_stroke_recorded's. The record's entry counts, encoded_bytes, levels and sculpt_layer equal that call's record. With a pass, layer_detail_entries > 0. Then one revert restores every level bit for bit, and revert / revert / apply / revert all land where they should.

  • A NULL record stamps the same and records nothing. Unrecorded session == recorded session == whole-path clay_multires_sculptor_apply_stroke, snapshot for snapshot.

  • The record is part of the binding. Another record and NULL are each INVALID_ARGUMENT and apply nothing, the other record stays empty, and the held stamps apply on the next matching call.

  • A refused call loses no stamps. Two passes, the first active. After one batch the host activates the second, and the next call is SNAPSHOT_MISMATCH with n == 0 and nothing written. The host reactivates the first. A malformed report is INVALID_ARGUMENT with nothing written. The next call applies the held stamps. The gesture ends where the whole-path recorded stroke ends, with the same record, and one revert restores it bit for bit.

  • A call after a session closes does not re-bind its record (c multires delta: a call after a session closes does not re-bind its record). This has the review repro's exact shape: a recorded gesture closes, the record is reverted, the top level is removed and added back, and there is one lagging _apply_stroke_tx (CLAY_OK, n == 0). Then clay_multires_delta_apply and _revert must both return SNAPSHOT_MISMATCH and write nothing. The case runs with and without the lagging call. Before the fix it failed: with the lagging call, _apply and _revert both returned CLAY_OK (2 assertions).

  • A pass made active mid-session joins a base-only record. The record holds both halves, and revert / apply restore the base and every level's positions.

  • Adaptive consumer, checked: it did not have this bug. DynamicStrokeGesture::apply_recorded returns before begin_capture / end_capture when it has no stamps, so a call after the close leaves the record's marks alone. A new subcase in test_c_stroke_session.cpp pins it: close, an unrecorded stamp, a post-close call, and clay_dynamic_delta_revert is still SNAPSHOT_MISMATCH. Injecting a re-mark on empty batches into the adaptive consumer makes that subcase fail, so it can catch the defect.

The existing multires calls in test_c_stroke_session.cpp pass NULL for the new parameter.

Mutation check. I rebuilt with each defect injected and ran test_c_multires_delta.cpp:

injected defect result
record ignored inside the consumer (apply(stamps, nullptr, nullptr)), the state before this PR 2 cases, 16 assertions fail
record check removed 1 case, 10 assertions fail
record checked after the stamps are taken 1 case, 4 assertions fail
record dropped from the binding 1 case, 6 assertions fail
report probed only after stamping 1 case, 3 assertions fail
record re-bound on a call that took no stamps (the first commit of this PR) 1 case, 2 assertions fail
adaptive record re-marked on an empty batch 1 subcase, 1 assertion fails

Verification

All exit codes were read directly (no $? after a pipe).

  • cmake --preset cpu-only -DCLAY_BUILD_TESTS=ON -DCLAY_BUILD_PYTHON=ON and a full build: rc 0.
  • ctest --preset cpu-only: rc 0, 11/11 (unit shards prefix/cull/heavy/rest, pyclay pytest, shard partition, ...), re-run after the review fix. test_c_multires_delta.cpp 21 cases / 1280 assertions; test_c_stroke_session.cpp 11 / 5418.
  • check_layering 0, check_kernel_dialect 0, check_licenses 0.
  • check_c_abi.py build/cpu-only/libclay_shared.dylib 0 (hygiene + ctypes FFI, against a freshly built dylib).
  • check_test_shards.py --binary 0 (3075 cases, 4 shards), check_doc_latency 0.
  • check_binding_parity.py --pyclay build/cpu-only/bindings/python --require-import 0, imported .../pyclay.cpython-311-darwin.so (773 capabilities). pyclay does not wrap the consumers; parity already lists them as C-only.
  • openspec validate --all --strict: 80/80 under both 1.12.0 (CI's pin) and 1.8.0.
  • GCC 16 -Wall -Wextra -Wpedantic -Wshadow -Werror -fsyntax-only on bindings/c/clay_c.cpp, test_c_multires_delta.cpp and test_c_stroke_session.cpp: rc 0. clay.h as C99: rc 0.

Docs and spec

  • clay.h: the consumer's comment block (nullability, binding, continuation, refusal order) and the "THREE WAYS IN" list on clay_multires_delta.
  • docs/07 § 5 (the sculptor consumers' records) and § 8d, plus docs/05's multires undo entry.
  • openspec/changes/add-stroke-session (not yet archived): the c-abi requirement now states the multires record. Two new scenarios (batches give one record, and a refused call loses no stamps), the binding scenario names the record, and tasks.md has a follow-up section.

ABI 0.126.0 (unchanged; amends an untagged entry point). Anyone building against an unreleased main between #688 and this PR has to add the argument. No tagged release ever had the old signature.

Follow-up to #670 / #688. Once this lands, the release/v0.126.0 notes need updating: docs/release-notes/v0.126.0.md still says the multires session consumer takes no clay_multires_delta.

clay_multires_sculptor_apply_stroke_tx (ABI 0.126.0, #688) took no
record, so a multires gesture fed through a stroke session could not
enter a host's undo stack, while its mesh and adaptive siblings and the
whole-path _apply_stroke_recorded all take one. 0.126.0 is untagged, so
the parameter is added in place: a nullable clay_multires_delta* record
after defer_normals, as in _apply_stroke_recorded.

The record is part of the session's binding, is continued across every
call of the gesture (base and active-pass halves), and is checked before
the session's stamps are taken, as is the report, so a
CLAY_ERROR_SNAPSHOT_MISMATCH or a malformed report loses no stamps.
The consumer re-bound its record on every call, including a no-op call
after the close, which skips the record check. A host calling after
every append could therefore stamp a record from before a level change
with the hierarchy's current structure_revision, and a redo after a
level was removed and added back was accepted instead of refused with
SNAPSHOT_MISMATCH. Bind only when stamps were taken, which is always
after the check accepted the record.

The adaptive consumer does not re-mark on an empty batch
(DynamicStrokeGesture::apply_recorded returns before begin_capture);
a subcase now pins that. Also states in clay.h that a pass activated
mid-gesture joins a base-only record, with a test, and rewords the
docs/05 capture sentence.
@leonardoaraujosantos
leonardoaraujosantos merged commit a2f27aa into main Oct 4, 2026
16 checks passed
@leonardoaraujosantos
leonardoaraujosantos deleted the fix/multires-stroke-tx-record branch October 4, 2026 15:48
leonardoaraujosantos added a commit that referenced this pull request Oct 4, 2026
The multires stroke-session consumer now takes a clay_multires_delta,
so its known limit is gone; the at-a-glance table, the session section,
Upgrading and the RELEASE.md entry say so.
leonardoaraujosantos added a commit that referenced this pull request Oct 5, 2026
The multires stroke-session consumer now takes a clay_multires_delta,
so its known limit is gone; the at-a-glance table, the session section,
Upgrading and the RELEASE.md entry say so.
leonardoaraujosantos added a commit that referenced this pull request Oct 6, 2026
The multires stroke-session consumer now takes a clay_multires_delta,
so its known limit is gone; the at-a-glance table, the session section,
Upgrading and the RELEASE.md entry say so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant