Repository navigation
Record a session-fed multires gesture into a clay_multires_delta - #689
Merged
Merged
Conversation
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
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.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
clay_multires_sculptor_apply_stroke_tx(#688, ABI 0.126.0) took noclay_multires_delta. Every other way into a sculptor takes the record its whole-path call takes:clay_mesh_sculptor_apply_stroke_txtakesclay_mesh_deltas*,clay_dynamic_sculptor_apply_stroke_txtakesclay_dynamic_delta* record, andclay_multires_sculptor_apply_stroke_recorded(#687) takesclay_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 aclay_multires_serializesnapshot 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
_recordedtwin:recordsits afterdefer_normals, where_apply_stroke_recordedhas it and where the mesh and adaptive consumers put theirs (just beforeout_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), asclay_dynamic_sculptor_apply_stroke_txhas it). A later call that names another record, or NULL after a record, isCLAY_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) toMultiresStrokeGesture::applyand thenbinds 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_recordruns beforenext_batch(skipped once the sink is closed, as on the adaptive consumer), so aCLAY_ERROR_SNAPSHOT_MISMATCHloses 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_recordedalready promises.Only a call that took stamps binds the record (review fix, second commit). The first version called
bindon every call. A call after the close skips the record check, applies nothing and returnsCLAY_OK, and it still re-stamped the record with the hierarchy's currentstructure_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 withSNAPSHOT_MISMATCH, and replayed the record onto a hierarchy it does not describe. Nowbindruns only whenbatch.stampsis 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.hdeclarestypedef 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 withgcc-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 andsculpt_layerequal 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_ARGUMENTand 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_MISMATCHwithn == 0and nothing written. The host reactivates the first. A malformed report isINVALID_ARGUMENTwith 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). Thenclay_multires_delta_applyand_revertmust both returnSNAPSHOT_MISMATCHand write nothing. The case runs with and without the lagging call. Before the fix it failed: with the lagging call,_applyand_revertboth returnedCLAY_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_recordedreturns beforebegin_capture/end_capturewhen it has no stamps, so a call after the close leaves the record's marks alone. A new subcase intest_c_stroke_session.cpppins it: close, an unrecorded stamp, a post-close call, andclay_dynamic_delta_revertis stillSNAPSHOT_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.cpppass NULL for the new parameter.Mutation check. I rebuilt with each defect injected and ran
test_c_multires_delta.cpp:apply(stamps, nullptr, nullptr)), the state before this PRVerification
All exit codes were read directly (no
$?after a pipe).cmake --preset cpu-only -DCLAY_BUILD_TESTS=ON -DCLAY_BUILD_PYTHON=ONand 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.cpp21 cases / 1280 assertions;test_c_stroke_session.cpp11 / 5418.check_layering0,check_kernel_dialect0,check_licenses0.check_c_abi.py build/cpu-only/libclay_shared.dylib0 (hygiene + ctypes FFI, against a freshly built dylib).check_test_shards.py --binary0 (3075 cases, 4 shards),check_doc_latency0.check_binding_parity.py --pyclay build/cpu-only/bindings/python --require-import0,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.-Wall -Wextra -Wpedantic -Wshadow -Werror -fsyntax-onlyonbindings/c/clay_c.cpp,test_c_multires_delta.cppandtest_c_stroke_session.cpp: rc 0.clay.has C99: rc 0.Docs and spec
clay.h: the consumer's comment block (nullability, binding, continuation, refusal order) and the "THREE WAYS IN" list onclay_multires_delta.docs/07§ 5 (the sculptor consumers' records) and § 8d, plusdocs/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, andtasks.mdhas a follow-up section.ABI 0.126.0 (unchanged; amends an untagged entry point). Anyone building against an unreleased
mainbetween #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.0notes need updating:docs/release-notes/v0.126.0.mdstill says the multires session consumer takes noclay_multires_delta.