feat(core): name every preset node, and add a multi-band EQ over existing filters - #3178
feat(core): name every preset node, and add a multi-band EQ over existing filters#3178vanceingalls wants to merge 17 commits into
Conversation
68263fa to
5fb2c91
Compare
dd80e8d to
7ddc505
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(core): name every preset node, and add a multi-band EQ — #3178
Verdict: LGTM
This is the fix half of the #3177+#3178 load-bearing pair, and it delivers. All three new HfAudioFxNode fields round-trip correctly:
- Parse side (defensive):
typeof node.X === "string" && node.X— guards against non-string JSON values, drops empty strings. - Serialize side (trusted):
node.X ? { X: node.X } : {}— the typed interface guarantees string-or-undefined, so truthiness is sufficient.
Both sides agree on the empty-string boundary (both treat it as absent). The pattern mirrors the existing fromCarve handling exactly.
The upgraded round-trip test is the real deliverable. The old test compared only n.type; the new one compares { type, id, fromPreset, label }. The PR body's comment — "Comparing only types is what let fromPreset be silently dropped by the parser" — is exactly right, and the new test is the regression gate.
Named-job labels flow through node.label ?? def.label in the UI — no hardcoded label map. The "names every node for the job it is doing" test enforces this across the entire preset catalog, with a thoughtful exemption for stacked pairs (consecutive identical nodes with matching params = one stage built from two biquads, like steep()).
EQ composite is well-designed. fromEq tagging follows the fromCarve pattern. Per-band ids are minted with the chain visible, so automation lanes address the correct biquad. readAudioEqBands reads from chain nodes (source of truth), not a cached list. The round-trip test verifies audioEqIds, band names, and gain values all survive serialization.
One observation (non-blocking): setAudioEqBandGain identifies bands by node.label === bandName. If custom bands with duplicate names were passed to addAudioEq, the fader would move both simultaneously. Not exploitable today (preset band names are unique), but a doc note making this explicit would be good hygiene.
No blocking issues.
Review by Miga
🤖 Generated with Claude Code
7ddc505 to
bff8f50
Compare
5fb2c91 to
def70d3
Compare
bff8f50 to
19cbbeb
Compare
def70d3 to
df2cc3e
Compare
19cbbeb to
7696263
Compare
df2cc3e to
6a84ad0
Compare
A peaking filter is "Peaking EQ" wherever it appears, so Clean Voice showed the same two words at node 02 (cutting mud at 250 Hz) and node 04 (lifting clarity at 3 kHz) with nothing to tell them apart. Reading down the rack, an author could not follow what had been done for them. `HfAudioFxNode.label` carries the job name, presets set it per node, and the rack shows it in place of the effect's own name. Clean Voice now reads: Remove Rumble, Reduce Mud, Even Out Loudness, Add Clarity, Peak Ceiling. FIXES A BUG IN a533d16: `parseAudioFxChain` and `serializeAudioFxChain` were dropping `fromPreset` entirely — only `fromCarve` survived the attribute. So a preset could not find its own nodes after a reload: re-applying would stack a second copy instead of replacing, and the rack would lose the grouping. The round-trip test compared only node TYPES, so it passed the whole time. It now compares type, id, fromPreset and label, and fails if any is dropped. Three tests, all falsified. Re-introducing the fromPreset drop fails the round-trip; dropping label from the serializer fails it too; and giving two nodes in one preset the same name fails the no-repeats check, which is the invariant this whole change exists to hold. Identical CONSECUTIVE nodes are exempt from that check — a stacked 24 dB/oct pair is one stage built from two biquads, not two jobs. Core 1688 -> 1690, studio 3664 -> 3665. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bass, middle and treble is the most widely understood audio control there is, which makes it the right answer for an author who would never reach for a parametric filter. It also removes a real failure: without it, a chain shaping two ranges holds two peaking filters that look identical in the rack. Built the way the carve is — one module owning several tagged nodes rather than a new effect type. Three bands ARE a low shelf, a peaking and a high shelf, so there is nothing new in the graph, nothing new in the render, and an author who opens the details finds exactly the filters they could have added by hand. That shape is also forced: the registry's parameters are a flat key/value record, so an `eq` effect TYPE carrying N bands would need array-shaped params it has no way to express. Two band sets. Three is Bass/Middle/Treble. Five opens to Bass/Warmth/Middle/Clarity/Air, named from the shared vocabulary the rest of the rack uses, so reaching for the EQ is also how the words get learned. The chain is authoritative, not a cached band list: an author can open the details and move a frequency by hand, and the faders read it back rather than overwriting it on the next drag. The fader is held to ±12 dB while the filters themselves allow ±40 — a tone control that can bury a track under 40 dB of bass is not a tone control. `fromEq` joins `fromCarve`, `fromPreset` and `label` through the parser and serializer, so an EQ survives the attribute round trip. Without that the module cannot find its own bands after a reload and silently becomes loose filters. Fourteen tests, falsified against four mutations: the parser dropping fromEq, the fader re-seeding the whole band (which would lose a hand-set frequency), the ±12 clamp removed, and two EQs sharing an id. Core 1690 -> 1704. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7696263 to
072040b
Compare
6a84ad0 to
804bc1e
Compare
Every preset node is named for the job it does —
"Remove Rumble", nothighpass. A chain that cuts mud and then lifts clarity must not show the same name twice.Not hypothetical — the first commit added
fromPresetto the type and the writer but not the parser, so the tag was silently dropped on every reload and a preset could no longer find its own nodes. The round-trip test passed the whole time because it compared only node types.Tone (EQ) is built as a composite over existing filters rather than a new effect type: users know what an EQ is, they don't know they want five peaking filters — and the registry's params are flat key/value and cannot express a band array.
🤖 Generated with Claude Code