feat: switch a preset off, or ramp it, as one thing - #3189
Conversation
e015f83 to
96e3dd8
Compare
373cdbb to
e021065
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM
Empirical claim verification (head SHA e021065)
presetAmount?: numberonHfAudioFxNode—packages/core/src/audioFx.ts:363-373.- Round-trip in BOTH halves: parse (clamped) at
audioFx.ts:381,391-393; serialize (omitted when1) ataudioFx.ts:403-405. Round-trip test ataudioFxPresets.test.ts:417-440; clamp test at:442-454. - Wet/dry graph wrap at
audio/audioFxGraph.ts:194-231;handle.presetsat:235-236; values-only update at:693-702; unwire on dispose at:710-717. - Consecutive-only grouping in
presetRunsataudioFxGraph.ts:149-167; amount clampMath.min(1, Math.max(0, amount))at:161. fx.preset.<id>lane target:parseAutomationTargetataudioAutomation.ts:299-302, factorypresetAutomationTargetat:322-324, orphan-lane drop viachain?.nodes.some((n) => n.fromPreset === parsed.presetId)at:348-350. Parsed BEFORE 3-part fx form, correct sincePRESET_TARGET_KEY = "preset"cannot collide with mintedn1/n2/…ids.- Lane scheduler consults
presets?.[parsed.presetId]ataudio/audioFxAutomation.ts:263-266; runtime plumbshandle.presetsatruntime/audioFx.ts:181-183and offline entry atstubs/audio-fx-runtime-entry.ts:487-493. - FE Off/On/× on the run head at
propertyPanelFxSection.tsx:582-627;setRunAmountwritespresetAmount(notenabled) at:359-370; run-remove sheds lanes viaonRemoveNodeAutomationat:378-386.automatedPresetsderivation atpropertyPanelAudioFxGroup.tsx:530-535.
OLD-assumption counterfactuals (every NEW test)
- graph
exposes one blend / blends at stored amount / fully applied by default / pushes amount / wraps nothing on own nodes / unwires on dispose(audioFxGraph.test.ts:65-112) — falsify: (a) nohandle.presetsat all; (b) hard-coded wet=1; (c) default 0 (silences legacy chains); (d)update()returning false forcing rebuild; (e) wrapping every node not justfromPresetruns; (f) dispose only iteratinghandles. - Round-trip (
audioFxPresets.test.ts:417-440) — falsifies missingpresetAmountin EITHER parse or serialize (samefromPresetfailure mode). - Clamp (
:442-454) — falsifies missing 0..1 clamp letting4through and drivingdry = -3. - FE
switches off in one gesture— falsifies OLD per-node-Off UX ANDenabled: falsemechanism. puts half in / seeded where it sits / driven not slider— falsifies missing run-head Amount row and missingfx.preset.<id>target.reads as on while any is applied— falsifies aggregating byenabledinstead ofpresetAmount > 0.back out whole with lanes— falsifies missing orphan-lane cleanup on run remove.
Cross-PR coupling
- New field on
HfAudioFxNode: parse + serialize allow-lists both updated with pinned round-trip. Old chains fall through to fully applied viatypeof node.presetAmount === "number" ? ... : 1(audioFxGraph.ts:158) — no silent-off on legacy load. - Ramp is a real automation lane (
fx.preset.<id>), not a snapshot; scheduler readshandle.presetstargets. - Off mechanism is
presetAmount: 0,enabledpreserved so settings survive (verified inswitches back on rather than deleting).
Nits (non-blocking)
- If a reorder splits a preset into two non-consecutive runs,
presetRunsemits two entries with the samepreset. InbuildFxChain,presetTargets[p.id] = mixTargets(...)at:235-236overwrites (lane drives second wrap), whilepresets.find((p) => p.id === run.preset)at:698matches the first wrap (static amount writes hit the first). Design comment ("no longer a unit") acknowledges this, but the two writes disagree rather than both no-op. Considerpresets.filter(...)on the update path so a torn preset at least stays consistent. automatedPresetsinpropertyPanelAudioFxGroup.tsx:530-535is recomputed each render — cheap but auseMemowould match sibling patterns.
CI
Preflight failure is the stack-wide oxfmt --check drift on 9 .md files (README, plans/, skills/hyperframes-audio/) — none touched by this PR. player-perf / preview-regression / regression FAILUREs are if: always() aggregators reporting FAILURE on SKIPPED shards after Detect changes skipped the matrix — stack-wide, no this-PR signal.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat: switch a preset off, or ramp it, as one thing — #3189
Verdict: LGTM
presetAmount in both parse AND serialize — confirmed. Parse uses type guard + finiteness check + 0..1 clamp. Serialize intentionally omits when amount === 1 (fully applied) so existing chains don't bloat. The wa-18 invariant is satisfied.
Wet/dry crossfade design is sound. A 0..1 value drives a dry/wet gain bracket around the entire preset run. Amount=1 is fully applied (wet only), amount=0 is fully bypassed (dry signal, untouched). The toggle and the automation lane are the same value — one mechanism for both "switch off" and "ramp it." This dissolves the class of bug where two separate controls for the same thing get out of sync.
No rebuild on amount change. update() pushes new gain values into the running graph (returns true), so sliding the amount knob doesn't restart the audio. The chain stores mechanism values (real params), not the amount — same contract as normalizeCarveSettings.
Round-trip tests verify amounts [0, 0.4, 1] survive serialize→parse, 1 round-trips to undefined (absent = fully applied), and out-of-range values (4) clamp on parse.
Orthogonal with profiles — preset amount controls overall wet/dry blend of the run; profile strength scales individual parameter values. A preset at amount=0.5 with strength=0.7 means 70% parameter intensity, then 50/50 blend with dry. No interference by design.
7 graph tests + 7 UI tests + 2 round-trip tests. Clean coverage.
Review by Miga
🤖 Generated with Claude Code
e021065 to
6a61908
Compare
96e3dd8 to
88d7d0b
Compare
6a61908 to
98fb1e0
Compare
88d7d0b to
dcc84d3
Compare
98fb1e0 to
f42d061
Compare
dcc84d3 to
212ddf3
Compare
f42d061 to
beb004b
Compare
212ddf3 to
ca02905
Compare
# Conflicts: # packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # skills-manifest.json # skills/hyperframes-audio/SKILL.md # skills/hyperframes-audio/references/attributes.md # skills/hyperframes-audio/scripts/carve.mjs
Fallow audit reportFound 10 findings. Duplication (2)
Health (8)
Generated by fallow. |
# Conflicts: # packages/core/src/audio/audioFxGraph.ts # packages/core/src/audioFx.ts # packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxPresetMenu.tsx # packages/studio/src/components/editor/propertyPanelFxSection.test.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # plans/audio-automation-lanes/SPEC.md # plans/audio-fx-presets.md # plans/audio-fx-ux/README.md # plans/automation-lanes-shared-rows.md # plans/automation-time-selection-design.md
A preset's nodes share no automatable parameter, and its worklet effects expose no AudioParams at all — Clean Voice could only ever have automated 3 of its 5 nodes. So the graph wraps each preset's run in a wet/dry pair, and
fx.preset.<id>drives the blend.Off is amount 0, not
enabled: false— the switch and the lane are the same value, with the switch at its two ends. Writingenabledwould take the nodes out of the graph, which a lane cannot do part-way.The bracket also gains On/Off and × for the whole preset: it said a preset was one thing the author added, then made them treat it as five.
The round-trip for the new
presetAmountfield had no test until a mutation survived one — the same invariant that bitfromPresetearlier in the stack.🤖 Generated with Claude Code