feat: one knob for the five effects that cannot honestly have one - #3188
Conversation
e47c566 to
b2b1d5f
Compare
e015f83 to
96e3dd8
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM. Preflight red is stack-wide pre-existing drift (verified below), not this PR.
Verified claims (head 96e3dd85).
- Five effects with derived knobs = compressor, gate, saturate, reverb, bitcrush —
audioFxProfiles.ts:229-340. Coverage pinned by test toEFFECT_COPY[id].primary === "strength"(audioFxProfiles.test.ts:63-73). - "Compressor makeup = s²·9.5, not linear 1/3/7" —
audioFxProfiles.ts:256. - "Saturation reversed to s²·2.8 up" —
audioFxProfiles.ts:301. - "Gate
releaseadded to profile" —audioFxProfiles.ts:280(120 − s·105), covered byderives: [..., "release"]at line 264. - "Registry defaults ≠ point on curve; compressor arrived at Evenness 0.67 / makeup 0 dB." Fix:
propertyPanelFxSection.tsx:702-704seeds throughapplyAudioFxProfile(type, 0.5, defaultAudioFxParams(type))when profile exists; test atpropertyPanelFxSection.test.tsx:596-611. - NaN → 0.5 (never silence) —
audioFxProfiles.ts:209-211clamp01; test ataudioFxProfiles.test.ts:164-172.
OLD-assumption counterfactuals.
- covers exactly... fails if a
primary: "strength"effect ships without a profile (five-set changes) or a profile is added for an effect the copy hasn't nominated. - derives only params effect has fails if
derivesnames a key not in the registrydef.params(typo migration guard). - stays inside declared range fails if sweep of
at(s)ever exceedsparam.min/max(clamping happens on write; the knob would silently go inert past the boundary). - moves monotonically fails on the exact old three-point-table shape the design proposed — a hand-tuned middle can turn around.
- keeps knee/mix under Details fails if
applyAudioFxProfilereplaces instead of{...params, ...profile.at(s)}(verified merge ataudioFxProfiles.ts:360). - reads strength back fails if
audioFxProfileStrengthmaps low↔high wrong on the first derived key (regression against invertingcarveProfile-style contract). - passes through anchors fails on the exact numeric drift the make-up/trim/gate corrections introduced (
-12, 4, 90, -55, -18, 0.25/0.55/0.9, 6). - seeded on curve fails if
withEffectreverts todefaultAudioFxParams(type)for profiled effects — the Evenness-0.67-with-0dB-makeup regression.
Cross-PR coupling verdict.
- No new
HfAudioFxNodefield. Profile is a derivation over storedparams;parseAudioFxChain/serializeAudioFxChain(audioFx.ts:862-949) unchanged and correctly untouched — the load-bearing-pair pattern (#3177+#3178, #3212+#3213) does not apply. - Coverage table analog to #3184's
HF_AUDIO_FXenumeration: yes —audioFxProfiles.test.ts:63-73enumeratesEFFECT_COPYand demands exact set equality. - Curve mapping: per-effect specific (linear anchors for threshold/ratio/attack/release/range; s² for makeup/output; piecewise-linear for reverb size to hit the 0.25/0.55/0.9 anchors — verified
at(0)/at(0.5)/at(1)in the anchors test). Monotonic per param tested at 21 samples. - Rack UI: derived knob replaces the primary control on the open face and Details reveals the real 5-7 params (
propertyPanelFxNodeRow.tsx:482-508, 517, 526). When no profile is present (derived === null), the existing primary-param path is unchanged;oneKnob = primary !== null || derived !== nullgates the disclosure. Automation correctly stays on the real params under Details — the derived knob has no AudioParam (propertyPanelFxSection.test.tsx:1195-1200).
Nits (non-blocking).
audioFxProfileStrengthinverts onderives[0]only. For compressor that'sthreshold; hand-edited chains where the author setsthreshold=-30but leaves ratio/makeup at defaults will read a strength inconsistent with the mechanism. Documented ataudioFxProfiles.ts:363-376as the honest answer — noting for reviewer awareness, not blocking.to2rounding onrange: -10 - s*20ats=0yields-10(inside[-40, -3]presumably); range checked by test, so safe.
CI. Preflight (lint + format) FAILURE ×3 = the recurring stack-wide oxfmt --check drift: identical 9-file list (README.md, 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, skills/hyperframes-audio/SKILL.md, .../attributes.md, .../fx-registry.md) fails on parent PR #3187 (wa-20d3-schematic) with byte-identical output — pre-existing, not introduced here. Downstream player-perf / preview-regression / regression are the rollup-of-skipped shards fed by the Preflight failure. Not this PR.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat: one knob for the five effects that cannot honestly have one — #3188
Verdict: LGTM
All five profile mappings (compressor, gate, saturate, reverb, bitcrush) are DSP-correct with measurement-backed corrections to the original design proposal:
- Compressor — quadratic makeup gain (
s*s*9.5) compensates for accelerating gain reduction as threshold drops AND ratio rises simultaneously. The linear 1/3/7 dB proposal left the track -2.5 dB at full strength. - Gate — release was missing from the original proposal. Measured: "swept against a fixed threshold and range, the gaps move 0.2 dB at 140 ms and 13.4 dB at 10 ms." Adding release to the profile is the correct fix.
- Saturate — output goes UP, reversing the design's figure. A soft clipper at -18 dB IS a limiter; peak fell from 0.496 to 0.089, making "warmer" mean "much quieter."
- Reverb — piecewise linear for size to hit design anchors (0.55 ≠ midpoint of 0.25/0.90). Wet/dry don't sum equally because matching them makes a big room quieter.
- Bitcrush — bits go DOWN as strength goes UP.
Math.roundfor the integersamplesparameter. This is exactly why bits can't be the module's face (the knob would go backwards).
Test coverage is thorough: bidirectional structural invariant, range bounds (0-1 sweep in 0.05 steps, all within registry min/max), monotonicity (every derived param moves in one direction), non-clobbering (preserves knee/mix), round-trip (apply → read back ≈ original), NaN handling (clamp01 returns 0.5).
The derives[0] position is implicitly the inversion key for readback — a one-line comment naming this convention would protect against accidental reordering.
Review by Miga
🤖 Generated with Claude Code
96e3dd8 to
88d7d0b
Compare
b2b1d5f to
95772df
Compare
88d7d0b to
dcc84d3
Compare
95772df to
94c0291
Compare
dcc84d3 to
212ddf3
Compare
94c0291 to
a7e8e7f
Compare
212ddf3 to
ca02905
Compare
a7e8e7f to
ffbc329
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
A compressor has seven controls and an author wants one, but no single one can be its face: threshold means nothing without ratio. So the knob is derived, exactly as
carveProfilealready turns one number into six.Three of the five sets of figures were wrong, and only rendering showed it. Measured through the real engine path on 20 s of narration:
s² × 9.5: spread falls 19.1 → 8.8 dB with level unmoved.releasewas not in the profile. Swept, release dominates: 0.2 dB at 140 ms, 13.4 dB at 10 ms. Gaps now fall 29.6 dB with speech unmoved.The first measurement reported the gate as working when it was doing nothing at all — a speaking-window measure excludes exactly the windows a gate acts on.
Also fixes a profiled effect arriving at registry defaults, which put the knob at 0.67 with make-up at 0 dB.
🤖 Generated with Claude Code