Author corrections without baking them into motion
Constant, ramp, and return offsets now append ordinary channel layers to a lane or cel in explicit owner frames. The inspector exposes the commands as one undoable transaction, shows conflicts, and offers removal or retry while preserving generated bases through regeneration. Ordered-stack compatibility is shared by validation, conflict reporting, and regeneration, including adjacent replacement coverage. Cel-sheet gaps and headers select their lane, so commands cannot fall through to another column's stale selection. 437 tests, 5,804 assertions; both browser flows; 56 Django tests; optimized frontend build.
This commit is contained in:
parent
3dbbe285fc
commit
815ce449ea
14 changed files with 806 additions and 60 deletions
232
docs/correction-authoring-plan.md
Normal file
232
docs/correction-authoring-plan.md
Normal file
|
|
@ -0,0 +1,232 @@
|
|||
# Correction authoring implementation plan
|
||||
|
||||
Written against `2f1c9b9` (2026-09-30), following the lane handoff in
|
||||
`7a54bfc`. Implemented on `codex/correction-authoring`; this now records the
|
||||
scope and acceptance criteria of that implementation.
|
||||
Read [lane-handoff.md](lane-handoff.md) and the correction section of
|
||||
[lane-model.md](lane-model.md) first. Their ownership and document rules remain
|
||||
the foundation. The choices below settle the first implementation's scope.
|
||||
|
||||
## Outcome
|
||||
|
||||
A person can select a lane or cel, specify a range, and apply Constant
|
||||
adjustment, Ramp, or Return motion to rotation or position. The result is one
|
||||
correction layer and one undo step. It works from either timing view. A lane
|
||||
correction crosses drawing boundaries; a cel correction travels with its cel.
|
||||
Regeneration preserves the hand work and presents incompatible layers for an
|
||||
explicit decision. Frames outside the support evaluate exactly as before.
|
||||
|
||||
Finish this vertical slice before adding more property types or gestures.
|
||||
Numeric range fields and an Apply button are sufficient for this pass. Dragging
|
||||
a range or manipulating a peak on the stage can later issue the same command.
|
||||
|
||||
## 1. Fix sheet targeting first
|
||||
|
||||
In `ui/timeline.cljs`, `cel-sheet` currently drops each lane row's `:select`.
|
||||
An occupied cell selects its cel; a gap only seeks, leaving the previous target
|
||||
selected. Thus clicking lane B's gap after selecting lane A can send an insert
|
||||
or overwrite to A.
|
||||
|
||||
Carry the row's complete selection address into its column and gap cells.
|
||||
An occupied cell selects its cel; a gap selects its lane. Make the column header
|
||||
select the lane too: this is the explicit way to author across drawings.
|
||||
Preserve full paths, not just `(peek path)`, as view identity. Keep seek and
|
||||
selection dispatch order deterministic.
|
||||
|
||||
Add a two-lane browser case: select A, click a gap in B, overwrite, assert that
|
||||
only B changes, and undo once. Add a header-selection assertion. Retain the
|
||||
existing occupied-cell hold test. Do not redesign sheet rendering in this step.
|
||||
|
||||
## 2. Make stack compatibility consistent
|
||||
|
||||
There is a concrete discrepancy at this HEAD:
|
||||
|
||||
- `channel/problems` uses `stack-conflict`, accounting for prior replacements.
|
||||
- `channel/conflicts` and `flow/regenerate.cljs`'s `rebased` use
|
||||
`conflict-with`, comparing an offset directly with the base.
|
||||
|
||||
A two-component base, a covering three-component replacement, then a
|
||||
three-component offset is valid and evaluates correctly, but the latter paths
|
||||
can report or mark that offset incompatible. Conversely a replacement can make
|
||||
an offset incompatible even when it fits the original base.
|
||||
|
||||
Extract one ordered-stack compatibility operation and use it for validation,
|
||||
conflict discovery, regeneration, and resolution. Keep `conflict-with` if useful
|
||||
for the narrower question its name/docstring describe. Do not use it alone to
|
||||
decide whether a stacked layer is applicable.
|
||||
|
||||
Compute compatibility against the values that can actually reach a layer over
|
||||
its support. Partition at overlapping support boundaries if needed: two adjacent
|
||||
replacements can jointly cover an offset even though neither covers it alone.
|
||||
An empty replacement channel does not supply a value and must not erase the
|
||||
possible input shape. Preserve the evaluator's absence behavior. Explicitly
|
||||
marked conflicts are skipped, so later layers must be checked against the stack
|
||||
that actually runs. During regeneration, recompute compatibility in order, using
|
||||
each preceding layer's resulting active/conflicted state. Preserve IDs, values,
|
||||
support, and order; update compatibility reasons without dropping hand work.
|
||||
|
||||
Use structural shape reasoning for dense data rather than requiring every block
|
||||
to be sampled. If unknown shape or missing samples limit what can be proven,
|
||||
retain the current absence contract and document that limit; do not claim an
|
||||
unconditional proof of runtime safety from incomplete metadata.
|
||||
|
||||
Tests: covering replacement of a different shape; partial coverage; adjacent
|
||||
covering replacements; empty replacement; inactive conflicted replacement;
|
||||
regeneration changing base shape; and the same cases through cursor evaluation.
|
||||
Assert that an accepted compatible stack is not listed as a conflict, and that
|
||||
an incompatible regenerated layer remains persisted but is skipped.
|
||||
|
||||
## 3. Pure correction commands
|
||||
|
||||
Add `frontend/src/arthur/domain/correction.cljs`. It owns authoring and resolving
|
||||
corrections; `channel.cljs` continues to own evaluation and compatibility.
|
||||
Suggested API (names may follow repository conventions):
|
||||
|
||||
```clojure
|
||||
(add clip sid node-id channel-path
|
||||
{:id layer-id :support [a b] :motion :return
|
||||
:start 0 :peak angle :peak-frame p})
|
||||
(remove-layer clip sid node-id channel-path layer-id)
|
||||
(retry-layer clip sid node-id channel-path layer-id)
|
||||
```
|
||||
|
||||
Return `{:clip updated :selection node-id}` or `{:refused reason}`. IDs come
|
||||
from the event caller (`random-uuid`), never from the pure command. Reject a nil
|
||||
ID or one already used within that channel stack. Address layers by the full
|
||||
symbol/node/channel/layer tuple; no global layer registry is needed.
|
||||
|
||||
Resolve the base via `node/channels`, which supplies defaults. A lane with no
|
||||
explicit rotation channel already has a zero rotation; materialize that channel
|
||||
with its new `:over`. Preserve every existing base field, generated provenance,
|
||||
and previous layer. Never route this through a setter that bakes the correction
|
||||
into base keys. Append to the ordered stack and validate the resulting document.
|
||||
Do not run correction edits through `lane/finish`, whose extent policy belongs
|
||||
to cel arrangement. Use `clip/problems` for the candidate document instead.
|
||||
|
||||
First authoring properties: `[:xform :rot]` (scalar radians) and `[:xform :pos]`
|
||||
(two numeric components). First blend operation: `:offset`. UI labels must say
|
||||
offset/delta, since a target offset of 20 degrees does not mean an absolute
|
||||
rotation of 20 degrees. Keep existing `:replace` evaluation and loaded stacks;
|
||||
there is no new replacement-authoring UI in this slice.
|
||||
|
||||
All command support endpoints are finite integer OWNER frames, `[a b)`, with
|
||||
`a < b`. Do not ban negative owner frames merely because displayed shot frames
|
||||
start at zero. Validate all supplied values for finite numbers and exact shape.
|
||||
Refuse unknown targets, unsupported properties/motions, malformed ranges, and
|
||||
incompatible stacks with useful messages. Refusal must not mutate store/history.
|
||||
|
||||
Motion construction uses existing channels only:
|
||||
|
||||
| Command | Values | Minimum samples |
|
||||
| --- | --- | --- |
|
||||
| Constant adjustment | `(ch/framed delta)` | 1 |
|
||||
| Ramp | `(ch/keyed {a start, (dec b) end} :linear)` | 2 |
|
||||
| Return motion | `(ch/keyed {a start, p peak, (dec b) start} :linear)` | 3 |
|
||||
|
||||
For Return, require integer `a < p < b-1`. Default the UI peak to
|
||||
`a + floor((b-a-1)/2)`; on an even-length range the earlier middle sample wins.
|
||||
Expose the peak frame so this is visible and adjustable. Never place an endpoint
|
||||
at `b`: it is outside the selected samples. `[10 13)` with start 0 and peak 0.5
|
||||
must yield offsets `0, 0.5, 0` at 10, 11, 12. Support controls the boundary;
|
||||
there is no need to insert zero keys into the base before/after it.
|
||||
|
||||
## 4. Owner and range UI
|
||||
|
||||
Add a Corrections section to the right pane (`ui/params.cljs`), extracting a
|
||||
`ui/corrections.cljs` component if that keeps the pane readable. Provide an
|
||||
explicit target readout (symbol, lane or cel), Rotation/Position, motion choice,
|
||||
From/Through fields, relevant value fields, peak frame for Return, and Apply.
|
||||
Offer a selected cel's owning lane as an explicit target choice. Do not silently
|
||||
promote a cel edit to a lane edit. Shared drawing content is outside this first
|
||||
UI; it has different sharing consequences.
|
||||
|
||||
For this first pass the range fields explicitly read **owner frames**, with
|
||||
inclusive From/Through converted to `[from, through+1)`. This is a deliberate
|
||||
UI scope choice, not a claim that a displayed shot range and owner range are
|
||||
interchangeable. It lets nested and retimed owners be addressed without an
|
||||
unproven range conversion. Match the existing zero-based numbering and show the
|
||||
owner beside the range. The handoff must record that displayed-range dragging
|
||||
is still outstanding.
|
||||
|
||||
Use an explicit three-sample initial draft in owner coordinates; for a cel,
|
||||
prefer its span start where it is integral. Show the range, allow adjustment,
|
||||
and do not extend a cel or shot to make the correction visible. Reset the draft
|
||||
when the target changes. Rotation is shown in degrees and converted to radians
|
||||
at the event boundary, following the existing inspector convention. Position
|
||||
uses x/y inputs in the owner's transform coordinates.
|
||||
|
||||
Draft inputs must not write document state, start history groups, or invoke the
|
||||
existing inspector `number-input`'s hold/settle behavior. Apply dispatches one
|
||||
event; success uses `edit/transaction` once and preserves the selected node's
|
||||
full address. Do not use a layer ID as node selection. Validate again at Apply,
|
||||
since the target/document may have changed since the draft was opened.
|
||||
|
||||
If adding a “use playhead” convenience, prove its mapping separately. The clock
|
||||
of a cel's transform is its own node clock, not the drawing source clock selected
|
||||
by `:playback`. `nest/inside` on the complete cel path enters the source and is
|
||||
therefore the wrong shortcut. The existing `selection-frame` resolves only the
|
||||
owning symbol; node/ancestor time conversion remains necessary. Floors, loops,
|
||||
and nonintegral mappings must never silently snap an authored range. Omit this
|
||||
convenience rather than expanding the first pass into a new timing system.
|
||||
|
||||
## 5. Conflict actions and regeneration proof
|
||||
|
||||
Show a document-wide list from `clip/conflicts` in the pane, including symbol,
|
||||
node, property, layer ID, and reason. Keep it accessible even when a different
|
||||
node is selected. Also list the selected target's layers in stack order with
|
||||
support, motion values, and status; do not require a new persisted motion label.
|
||||
|
||||
Provide Remove correction and Retry compatibility. Remove is explicit and
|
||||
undoable; Retry rechecks the complete candidate stack and clears a conflict only
|
||||
when it is valid. If retry would invalidate a downstream offset, refuse and say
|
||||
why. Similarly, removing a replacement that makes a later offset invalid must
|
||||
refuse, rather than commit an invalid document or silently remove more layers.
|
||||
A retry that changes nothing must not manufacture an undo step.
|
||||
|
||||
These are minimal resolution actions, not topology remapping. Automatic geometry
|
||||
remapping, reordering layers, editing arbitrary stored vector values, and resolving
|
||||
removed targets are separate work. Preserve all existing generated-data behavior.
|
||||
|
||||
Exercise the actual `flow/regenerate.cljs` entry points in integration tests:
|
||||
author a correction, regenerate compatible base data, and confirm the correction
|
||||
survives with its ID/support/values intact and affects the new base. Then change
|
||||
topology on a geometry fixture and assert persisted actionable conflicts. The
|
||||
geometry fixture can use an existing layer directly; geometry authoring is not
|
||||
required to expose and resolve a conflict already present in a document.
|
||||
|
||||
## 6. Verification and completion
|
||||
|
||||
Use focused tests that establish observable promises:
|
||||
|
||||
- Domain: each motion's sample values; refusal on short ranges/nonfinite values;
|
||||
default channel materialization; unchanged base and prior layers; duplicate ID.
|
||||
- Evaluation: compare before/after on every frame outside support, including
|
||||
neighboring interpolated frames. Cursor/spec agreement in nonmonotonic order.
|
||||
- Ownership: a lane Return crosses a drawing boundary; a cel correction moves
|
||||
with the cel and survives split/trim. Neither alters another use of its drawing.
|
||||
- Sampling: picture-rate/pose selection changes the generated base frame while
|
||||
the authored correction still reads owner time. Keep HEAD's regression tests.
|
||||
- Events: one Apply is one undo step; undo/redo restores complete layer data;
|
||||
refusal leaves clip/history unchanged; stale target refuses; selection survives.
|
||||
- Persistence: leaf and Transit round trips retain layers, order, IDs, conflicts.
|
||||
- Browser: both views can select a target and apply the same correction using
|
||||
actual controls. Assert evaluated results and history, not only a layer count.
|
||||
Include the two-lane gap-targeting case from step 1.
|
||||
- Regeneration and conflict actions: use the real flow and test undoable removal,
|
||||
valid retry, invalid retry, and removal that would break a downstream layer.
|
||||
|
||||
Run the suites documented in `lane-handoff.md`: CLJS tests, lane and take browser
|
||||
flows, Django tests, and optimized frontend build. Restore the dev app bundle
|
||||
after the release build. Note that `take.mjs` writes a local project. Report
|
||||
actual results and any unrun checks; do not copy previous test counts as evidence.
|
||||
|
||||
Suggested commit sequence: sheet targeting; consistent stack compatibility;
|
||||
pure correction commands; pane/events plus browser proof; updated handoff.
|
||||
Keep each commit coherent and tested. No schema version bump should be needed:
|
||||
the layers already have a persisted representation.
|
||||
|
||||
Update `lane-handoff.md` and `lane-model.md` with what shipped, the owner-frame
|
||||
range UI limitation, conflict actions available, and verified test counts. Done
|
||||
means a person can author and undo the correction, regenerate its base, and see
|
||||
either their preserved edit or a useful conflict. A constructor without reachable
|
||||
controls, or controls without that regeneration proof, does not finish this work.
|
||||
|
|
@ -1,10 +1,11 @@
|
|||
# Lane and cel handoff
|
||||
|
||||
Status (2026-09-30): the lane model is implemented through its commands and its
|
||||
first two views. Cels are ordinary nodes with their own playback clock, the
|
||||
timeline draws them as one row, the cel sheet draws frames down and lanes across,
|
||||
and both views issue the same commands. Correction layers evaluate and survive
|
||||
regeneration. What is missing is the commands that make a correction.
|
||||
Status (2026-09-30): the lane model is implemented through its commands, its
|
||||
first two views, and correction authoring. Cels are ordinary nodes with their
|
||||
own playback clock; the timeline draws them as one row and the cel sheet draws
|
||||
frames down and lanes across. Both views issue the same commands. Rotation and
|
||||
position corrections can be authored as Constant, Ramp, or Return motion on a
|
||||
lane or cel, survive regeneration, and expose conflicts for removal or retry.
|
||||
|
||||
The commits beginning at `3d3c1bb` are the argument for the model and are worth
|
||||
reading before touching what they did — they are the design record, more than
|
||||
|
|
@ -87,18 +88,18 @@ decision, not a cleanup.
|
|||
|
||||
## Next steps, in order
|
||||
|
||||
1. **The commands that make a correction** — Constant adjustment, Ramp, Return
|
||||
motion over a selected range, per `lane-model.md`. The evaluator is done and
|
||||
has no opinion about how a range or a motion shape is chosen, which is now a
|
||||
view question. A panel also needs to offer `clip/conflicts` for resolution.
|
||||
Note the one open question: a correction needs a stable `:id` from somewhere,
|
||||
and cel ids come from the caller because this namespace is pure.
|
||||
2. **Slip source and retime.** Both have real design questions open and the doc
|
||||
The implemented correction slice and its remaining UI limits are recorded in
|
||||
[Correction authoring](correction-authoring-plan.md).
|
||||
|
||||
1. **Slip source and retime.** Both have real design questions open and the doc
|
||||
says to refuse rather than approximate: retime needs a defined warp and
|
||||
interpolation behaviour, and is not moving keys whose numbers happen to fall
|
||||
inside a selection.
|
||||
3. **Deleting reused content.** Reference discovery exists (`node/sources`,
|
||||
2. **Deleting reused content.** Reference discovery exists (`node/sources`,
|
||||
`clip/places`, `clip/contains-symbol?`); the policy does not.
|
||||
3. **Displayed-range correction gestures.** The first correction panel asks for
|
||||
explicit owner frames. Dragging a range in a retimed/nested view still needs
|
||||
a proved mapping; do not make it snap through floors or loops.
|
||||
4. **Collaboration.** `lane-model.md` is explicit that one leaf per channel does
|
||||
NOT solve two people editing different keys of the same channel. No conflict
|
||||
policy exists for that.
|
||||
|
|
@ -126,6 +127,12 @@ decision, not a cleanup.
|
|||
- **Generated sampling applies to the base, not the hand correction.** Picture
|
||||
rate and pose selection may choose an earlier generated frame; correction
|
||||
support and values still read the node's current authored frame.
|
||||
- **Correction commands live in `domain/correction.cljs`.** IDs come from the
|
||||
event caller; the pure command materializes default transform channels,
|
||||
appends one layer, and validates the complete document. The inspector authors
|
||||
rotation and position offsets in explicit owner frames. One Apply is one undo
|
||||
step. `channel/reconcile` is the shared ordered-stack compatibility rule used
|
||||
by validation, conflict reporting, and regeneration.
|
||||
- **Two test patterns worth copying.** `the-cursor-agrees-with-the-specification-in-any-frame-order`
|
||||
holds the optimized cursor to `value-at` in forward, backward and random order
|
||||
— add a case to it for any new channel shape. And `drawn` in `lane_test`
|
||||
|
|
@ -165,7 +172,7 @@ decision, not a cleanup.
|
|||
|
||||
From `frontend/`:
|
||||
|
||||
npx shadow-cljs compile test && node out/node-tests.js # 429 tests, 5,767 assertions
|
||||
npx shadow-cljs compile test && node out/node-tests.js # 437 tests, 5,804 assertions
|
||||
npx shadow-cljs compile app # the bundle Django serves
|
||||
npx shadow-cljs release app # then `compile app` again — see above
|
||||
|
||||
|
|
|
|||
|
|
@ -2,8 +2,8 @@
|
|||
|
||||
Revised 2026-09-30. Target design. Cel ownership, source playback, the
|
||||
content and cel commands, placement anywhere in a lane, overwrite, a one-row cel
|
||||
strip, a frame-down cel sheet and the correction-layer evaluator are implemented;
|
||||
the commands that produce a correction and the retiming commands are not.
|
||||
strip, a frame-down cel sheet, correction evaluation, and correction authoring
|
||||
for rotation and position are implemented. Retiming commands are not.
|
||||
See the status note under
|
||||
[Proof obligations](#proof-obligations-and-implementation-order).
|
||||
|
||||
|
|
@ -536,17 +536,23 @@ retime, and deleting reused content. A lane cannot hold AUDIO cels — `lane-pro
|
|||
requires visual ones, though this document says a lane may hold either and
|
||||
should reject only a mixture.
|
||||
|
||||
What is NOT implemented is a command that produces a layer — the doc's Constant
|
||||
adjustment, Ramp and Return motion — and with it the question of how a view
|
||||
offers those three over a selected range, and how it offers a conflict for
|
||||
resolution. Slip source and retime are also not implemented; a refusal is the
|
||||
current behavior where the model demands an explicit choice nobody has made yet.
|
||||
`domain/correction.cljs` now produces Constant adjustment, Ramp, and Return
|
||||
motion layers for rotation and position. The inspector exposes them on a selected
|
||||
lane or cel using an explicit range in that owner's frames; this deliberately
|
||||
leaves displayed-range dragging through nested or retimed owners for later. One
|
||||
Apply is one undo step. Conflicted layers are listed, can be removed, and can be
|
||||
retried when the complete ordered stack is compatible again. Validation,
|
||||
conflict reporting, and regeneration share that ordered-stack rule, including
|
||||
coverage by adjacent replacement layers. Slip source and retime are still not
|
||||
implemented; a refusal is the current behavior where the model demands an
|
||||
explicit choice nobody has made yet.
|
||||
The cel sheet is the same projected cels and selection addresses with its axes
|
||||
turned: frames down and lanes across, so commands selected there and in the
|
||||
timeline have identical targets. The suite stands at 429 tests and 5,767
|
||||
timeline have identical targets; a gap selects its column's lane rather than
|
||||
retaining a stale selection from another column. The suite stands at 437 tests and 5,804
|
||||
assertions, with `frontend/test/browser/lane.mjs` driving the editor through
|
||||
create, hold, overflow, undo, reuse, make unique, duplicate, split, insert,
|
||||
trim, move and blank. Rewrite tests that encode superseded
|
||||
trim, move, blank, correction authoring, and two-lane sheet targeting. Rewrite tests that encode superseded
|
||||
behavior rather than preserving behavior to keep them green.
|
||||
|
||||
Build small adversarial documents and test their domain operations before
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue