feat(sloppak): core reader for source rigs (feedpak 1.18.0)#1040
Merged
Conversation
`sloppak_tone_changes` emitted `{t, name}` only, so a chart's declared
sound never reached the client: `base_rig` was never read and each
change's `rig` was dropped at the wire boundary. Both survive load
intact (`Arrangement.tones` is an opaque passthrough) — the strip
happened here, at the last step before send.
That left the rig model (feedpak-spec 1.18.0 §6.9/§7.9) unreachable
from core: a pack could declare which rig voices a part, and nothing
downstream could ever see it. First step of the core reader for source
rigs; the rig library itself and the manifest precedence cascade follow.
Return `(base, base_rig, changes)` and keep `rig` on each change. Both
ids are validated as non-blank strings and stripped — anything else is
dropped rather than forwarded, so presence of the key means the change
binds a rig. Resolution against `rigs.json` deliberately does NOT happen
here: this builder preserves the declared binding, while realization
selection and the `intent.gm` fallback belong to whatever voices the
part.
On the wire `base_rig` is omitted entirely when empty, so packs that
bind no rig produce the byte-identical `tone_changes` message they
always did.
Signed-off-by: gionnibgud <gionnibgud@gmail.com>
feedpak 1.18.0 lets a chart declare what a MIDI part should sound like by binding a rig id, but core had nothing to bind to: `rigs`, `base_rig` and `drum_tones` appeared nowhere in lib/, server.py or static/. The preceding commit carries the reference onto the wire; this adds the library it references. Read the manifest `rigs:` key into a new `LoadedSloppak.rigs`, alongside the other side-files rather than on Song — every side-file (drum_tab, song_timeline, keys, notation) hangs off the load result, and rigs is pack-level, not per-arrangement. Same permissive posture as its neighbours: missing, unreadable, malformed or traversing disables rigs with a warning and never fails the pack, which §7.9 requires outright. Rig objects pass through VERBATIM. §7.9 obliges a Reader to preserve unknown role/engine/kind values and `ext` namespaces, so validating block structure here would be wrong as well as premature — realization selection and the `intent.gm` floor belong to whatever voices the part. The only entries dropped are ones unreachable by construction: a rig is addressable solely by `id`, so a non-dict entry or one without a usable string id can never be referenced. Ids are stripped to match the reference side, and a duplicate id resolves first-wins with a warning, since ambiguity there would surface as the wrong sound rather than an error. Signed-off-by: gionnibgud <gionnibgud@gmail.com>
feedpak 1.18.0 lets a sound binding arrive from three places, and core
honoured none of them: the manifest arrangement entry, the arrangement
JSON, and the top-level drum_tones. Reading them needs a precedence
rule, because two of the three can be present at once.
Arrangement entries: the entry's `tones` replaces the arrangement JSON's
WHOLESALE (spec 5.2), unlike name/tuning/capo/centOffset beside it,
which override field by field. A merge would produce a sound nobody
authored -- one source's base under the other's changes -- which is
worse than either block alone. This is also what makes a notation-only
keys entry bindable at all, since it has no arrangement JSON to carry
tones in the first place.
Drums: the top-level drum_tones binds the song-level primary part, and
a `type: drums` entry's own tones takes precedence, with a Reader
forbidden from applying both to the same part (5.1). That is the same
shape as the drum_tab alias rule, so it lives inside
_resolve_drum_parts next to it rather than beside it -- one precedence
resolver, not two that drift. drum_tones is the PRIMARY's fallback
only: a second drummer with no binding gets None, never the primary's
kit.
An empty `tones: {}` reads as absent rather than as an override to
silence, matching how arrangement_from_wire already normalizes the
in-JSON empty dict, so a stray empty object cannot quietly unbind a
part.
Spec-conformance gate passes with drum_tones added to the keys core
reads (22 of the spec's 32, all declared).
Signed-off-by: gionnibgud <gionnibgud@gmail.com>
CHANGELOG entry for the core rig reader, plus the WS protocol table in CLAUDE.md, which described `tone_changes` as carrying only base + name. While in that row: its time key was documented as `time`, but every producer emits `t` — both the sloppak builder and the legacy XML path. The 3D highway already carries a comment warning readers about exactly this discrepancy. Corrected here rather than left sitting next to the newly-added keys, where a reader would reasonably assume both were equally reliable. Signed-off-by: gionnibgud <gionnibgud@gmail.com>
Contributor
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 23, 2026
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
2 participants
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.
What
Core reader for source rigs — the first half of letting a pack declare what a MIDI part should sound like. Board: Keys Foundations #4, "Core reader for source rigs (spec 1.18.0)".
feedpak 1.18.0 (FEP #61) shipped the model: a rig can carry a
role: "source"block that voices a MIDI part, with a guaranteed GM floor. Core implemented none of it —rigs,base_riganddrum_tonesappeared nowhere inlib/,server.pyorstatic/, andlib/tones.pyactively dropped therigfield on its way to the wire. A chart could declare its sound and nothing downstream could ever see it.Three commits, each independently reviewable:
tonespayload carries the rig bindings.sloppak_tone_changesreturns(base, base_rig, changes)and keepsrigper change. Both ids are validated as non-blank strings and stripped; anything else is dropped rather than forwarded, so presence of the key means "this binds a rig".rigs:→LoadedSloppak.rigs(alongside the other side-files rather than onSong— it's pack-level, and every side-file hangs off the load result). Same permissive posture as its neighbours: missing / unreadable / malformed / traversing disables rigs with a warning and never fails the pack, which §7.9 requires outright.tonesreplaces the arrangement JSON's wholesale (§5.2) — no field-level merge. Top-leveldrum_tonesbinds the primary drum part, atype: drumsentry's owntonesoutranks it, and a Reader must never apply both to the same part (§5.1).Scope — what core deliberately does NOT do
It loads, resolves the binding, and exposes. It does not select a realization, does not apply the
intent.gmfloor, and does not make sound — those belong to whatever actually voices the part (the resolver-cascade work, and the sound-source plugin).Consequently rig objects pass through verbatim. §7.9 obliges a Reader to preserve unknown
role/engine/kindvalues andextnamespaces, so validating block structure here would be wrong as well as premature. The only entries dropped are ones unreachable by construction: a rig is addressable solely byid, so a non-dict entry or one without a usable string id can never be referenced. Duplicate ids resolve first-wins with a warning — ambiguity there would surface as the wrong sound rather than an error.Compatibility
Additive on the wire.
base_rigis omitted entirely when the chart binds no rig, so a pack without rigs produces the byte-identicaltone_changesmessage it always did, and existing consumers (highway_3dreads.name) are untouched.One behaviour worth calling out explicitly: an empty
tones: {}on a manifest entry reads as absent, not as "override to silence".arrangement_from_wirealready normalizes the in-JSON{}toNonethe same way, and the alternative would let a stray empty object silently unbind a part's sound.Verification
test_sloppak_rigs_load.py,test_sloppak_tones_precedence.py, and rig cases intest_tones.py).tools/check_spec_conformance.py: OK. Core now reads 22 of the spec's 32 declared keys;rigsanddrum_tonesare both spec-declared, so no new grandfather debt.Docs
CLAUDE.md's WS protocol table gainsbase_rig/rigon thetone_changesrow. While in that row I also corrected its time key fromtimetot— every producer emitst, andhighway_3d/screen.jsalready carries a comment warning readers about that exact discrepancy. Leaving a known-wrong field name next to the newly-added ones seemed worse than the small out-of-scope fix; happy to split it out if preferred.feedpak surface
Checklist
CHANGELOG.md[Unreleased]updated (user-visible changes)git commit -s)Related, not blocking: #1039 dedups the manifest-path containment guard that
_load_rigs_filehere also copies. If #1039 lands first I'll rebase this onto it and route the new loader through the shared helper.