From e57ad16caac1a70b1b8f72d60c0c5efa38fcd2b2 Mon Sep 17 00:00:00 2001 From: gionnibgud Date: Thu, 23 Jul 2026 09:24:48 +0200 Subject: [PATCH] Resolve which tones block binds a part 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 --- lib/sloppak.py | 62 ++++++- tests/test_sloppak_tones_precedence.py | 230 +++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 2 deletions(-) create mode 100644 tests/test_sloppak_tones_precedence.py diff --git a/lib/sloppak.py b/lib/sloppak.py index cacd864..7c4028a 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -855,18 +855,46 @@ def _load_rigs_file(source_dir: Path, rel: str) -> dict | None: } +def _entry_tones(entry: dict) -> dict | None: + """A manifest entry's `tones` binding, or None when it doesn't carry one. + + Spec §5.2: a manifest arrangement entry's `tones` overrides the arrangement + JSON's `tones` **wholesale** — no field-level merge. This normalizes the + "does it carry one" test for both the arrangement path and the drum path. + + An empty dict reads as *absent*, not as "override to silence": it is what a + Writer emits by accident, `arrangement_from_wire` already normalizes the + in-JSON `{}` to None the same way, and treating it as an override would let + a stray empty object silently unbind a part's sound. + """ + tones = entry.get("tones") + return tones if isinstance(tones, dict) and tones else None + + def _resolve_drum_parts( source_dir: Path, drum_tab_rel: object, drum_tab_data: dict | None, drum_pointer_entries: list[dict], + drum_tones: dict | None = None, ) -> tuple[dict | None, list[dict] | None]: - """Resolve drum pointers into a primary-first list with unique ids.""" + """Resolve drum pointers into a primary-first list with unique ids. + + Also binds each part's sound (feedpak 1.18.0). The precedence mirrors the + `drum_tab` alias rule this function already implements: a `type: drums` + entry's own `tones` wins for that part, and the song-level `drum_tones` is + the fallback for the **primary** part only. A Reader MUST NOT apply both to + the same part (spec §5.1/§5.2), which is why the primary picks one or the + other here rather than merging them. + """ if drum_tab_data is None and not drum_pointer_entries: return drum_tab_data, None primary_id = "drums" primary_name = None + # The primary's own binding, lifted from its alias pointer entry when it has + # one. Stays None if no entry claims the primary — `drum_tones` fills in. + primary_tones = None extra_parts: list[dict] = [] seen_rels: set[str] = set() # Use the same canonical, traversal-safe identity as zip member lookup so @@ -890,6 +918,11 @@ def _resolve_drum_parts( primary_id = entry_id if entry_name: primary_name = entry_name + # This entry IS the primary (an alias pointer at the same file), so + # its binding is the primary's — and it outranks `drum_tones`. + _alias_tones = _entry_tones(entry) + if _alias_tones is not None: + primary_tones = _alias_tones continue tab = _load_drum_tab_file(source_dir, rel, f"drum part {entry_id or rel}") if tab is None: @@ -900,6 +933,9 @@ def _resolve_drum_parts( "name": entry_name or (tab_name if isinstance(tab_name, str) and tab_name else "Drums"), "drum_tab": tab, + # Non-primary parts bind through their own entry only; `drum_tones` + # is explicitly the primary's fallback, never theirs. + "tones": _entry_tones(entry), }) parts: list[dict] = [] @@ -908,7 +944,14 @@ def _resolve_drum_parts( if primary_name is None: tab_name = drum_tab_data.get("name") primary_name = tab_name if isinstance(tab_name, str) and tab_name else "Drums" - parts.append({"id": primary_id, "name": primary_name, "drum_tab": drum_tab_data}) + parts.append({ + "id": primary_id, + "name": primary_name, + "drum_tab": drum_tab_data, + # Entry `tones` takes precedence; `drum_tones` is the fallback. One + # or the other, never both on the same part (spec §5.1). + "tones": primary_tones if primary_tones is not None else drum_tones, + }) used_ids.add(primary_id) next_generated_id = 2 @@ -1027,6 +1070,14 @@ def load_song( # _finite_float keeps a malformed manifest NaN/Infinity from # poisoning the song_info JSON (same guard as the wire path). arr.cent_offset = _finite_float(entry["centOffset"]) + # `tones` overrides WHOLESALE, unlike the field-level overrides above: + # the entry's object replaces the arrangement JSON's entirely, with no + # per-field merge (spec §5.2). A Writer SHOULD NOT emit both, but when + # one does, a half-merged sound — this pack's base with that pack's + # changes — would be worse than either source alone. + _entry_tone_block = _entry_tones(entry) + if _entry_tone_block is not None: + arr.tones = _entry_tone_block # Beats/sections can live on the arrangement itself in the wire format. # If the manifest-level arrangement JSON carries them, pull them onto @@ -1099,8 +1150,15 @@ def load_song( # Keep the dense compatibility logic independently testable and guarantee # ids are unique before the highway exposes them as selectors. + # Top-level `drum_tones` (spec §5.1) binds the song-level drum part — the + # fallback for packs without `type: drums` arrangements. Same shape as an + # arrangement entry's `tones`; `_resolve_drum_parts` owns the precedence. + _raw_drum_tones = manifest.get("drum_tones") + drum_tones_data = _raw_drum_tones if isinstance(_raw_drum_tones, dict) and _raw_drum_tones else None + drum_tab_data, drum_parts = _resolve_drum_parts( source_dir, drum_tab_rel, drum_tab_data, drum_pointer_entries, + drum_tones_data, ) # Drum-only sloppak: every GP track was percussion, so it ships a diff --git a/tests/test_sloppak_tones_precedence.py b/tests/test_sloppak_tones_precedence.py new file mode 100644 index 0000000..490b5ef --- /dev/null +++ b/tests/test_sloppak_tones_precedence.py @@ -0,0 +1,230 @@ +"""Loader coverage for the manifest-vs-in-JSON `tones` precedence cascade +(feedpak 1.18.0, spec §5.1 / §5.2). + +Two rules, both about *which* sound binding wins, neither about interpreting it: + + - A manifest arrangement entry's `tones` replaces the arrangement JSON's + `tones` **WHOLESALE** — no field-level merge. A half-merged block (this + source's `base` with that source's `changes`) would be a sound nobody + authored, so the two never blend. + - Top-level `drum_tones` binds the song-level (primary) drum part and is the + fallback; a `type: drums` entry's own `tones` takes precedence, and a + Reader MUST NOT apply both to the same part. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import yaml + +import sloppak as sloppak_mod + + +IN_JSON_TONES = { + "base": "In-JSON Clean", + "base_rig": "injson-clean", + "changes": [{"t": 5.0, "name": "In-JSON Lead", "rig": "injson-lead"}], +} +ENTRY_TONES = { + "base": "Entry Grand", + "base_rig": "entry-grand", + "changes": [{"t": 9.0, "name": "Entry Rhodes", "rig": "entry-rhodes"}], +} + + +def _tab(name: str) -> dict: + return { + "version": 1, + "name": name, + "kit": [{"id": "kick", "name": "Kick"}], + "hits": [{"t": 1.0, "p": "kick", "v": 100}], + } + + +def _write_pak(root: Path, manifest_extras: dict, arr_tones: dict | None = None, + files: dict[str, dict] | None = None) -> Path: + """Directory-form sloppak with one Lead arrangement, optionally carrying an + in-JSON `tones` block, plus any extra files.""" + pak = root / f"{root.name}.sloppak" + pak.mkdir() + arr_dir = pak / "arrangements" + arr_dir.mkdir() + arr = { + "name": "Lead", "tuning": [0, 0, 0, 0, 0, 0], "capo": 0, + "notes": [], "chords": [], "anchors": [], "handshapes": [], + "templates": [], "beats": [], "sections": [], + } + if arr_tones is not None: + arr["tones"] = arr_tones + (arr_dir / "lead.json").write_text(json.dumps(arr)) + + manifest = { + "title": "Test", "artist": "Tester", "album": "", "year": 2026, + "duration": 10.0, + "arrangements": [{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}], + "stems": [{"id": "full", "file": "stems/full.ogg", "default": True}], + } + manifest.update(manifest_extras) + (pak / "manifest.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False)) + for rel, payload in (files or {}).items(): + (pak / rel).write_text(json.dumps(payload)) + return pak + + +def _load(pak_path: Path, tmp_path: Path): + cache = tmp_path / "cache" + cache.mkdir() + return sloppak_mod.load_song(pak_path.name, pak_path.parent, cache) + + +# ── Arrangement entry vs in-JSON (§5.2) ────────────────────────────────────── + +def test_entry_tones_replaces_in_json_wholesale(tmp_path: Path): + """The entry object replaces the in-JSON one entirely — no key survives + from the loser, not even ones the winner doesn't define.""" + entry_tones = {"base": "Entry Only"} # no base_rig, no changes + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", + "tones": entry_tones}]}, + arr_tones=IN_JSON_TONES, + ) + arr = _load(pak, tmp_path).song.arrangements[0] + assert arr.tones == entry_tones + # The in-JSON `base_rig` and `changes` must NOT have been merged in. + assert "base_rig" not in arr.tones + assert "changes" not in arr.tones + + +def test_in_json_tones_survive_when_entry_has_none(tmp_path: Path): + pak = _write_pak(tmp_path, {}, arr_tones=IN_JSON_TONES) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_empty_entry_tones_is_absent_not_an_override(tmp_path: Path): + """`{}` reads as "didn't specify", not "override to silence" — otherwise a + stray empty object silently unbinds the part's sound.""" + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", "tones": {}}]}, + arr_tones=IN_JSON_TONES, + ) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_malformed_entry_tones_is_ignored(tmp_path: Path): + """A non-dict `tones` must not override, and must not crash the load.""" + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "lead", "name": "Lead", + "file": "arrangements/lead.json", + "tones": ["not", "a", "dict"]}]}, + arr_tones=IN_JSON_TONES, + ) + assert _load(pak, tmp_path).song.arrangements[0].tones == IN_JSON_TONES + + +def test_entry_tones_binds_a_notation_only_arrangement(tmp_path: Path): + """§5.2: entry `tones` is available whether or not the arrangement has a + `file` — a keys part is a notation-only entry, and binding its sound is the + whole point of the 1.18.0 work.""" + notation = {"version": 1, "measures": []} + pak = _write_pak( + tmp_path, + {"arrangements": [{"id": "keys", "name": "Keys", + "notation": "notation_keys.json", + "tones": ENTRY_TONES}]}, + files={"notation_keys.json": notation}, + ) + arr = _load(pak, tmp_path).song.arrangements[0] + assert arr.name == "Keys" + assert arr.tones == ENTRY_TONES + + +# ── drum_tones vs entry tones (§5.1) ───────────────────────────────────────── + +def test_drum_tones_binds_the_primary_part(tmp_path: Path): + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json", "drum_tones": ENTRY_TONES}, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert len(parts) == 1 + assert parts[0]["tones"] == ENTRY_TONES + + +def test_entry_tones_outrank_drum_tones_on_the_primary(tmp_path: Path): + """An alias pointer entry naming the same file IS the primary, so its own + binding wins — and `drum_tones` must not also be applied.""" + alias_tones = {"base": "Alias Kit", "base_rig": "alias-kit"} + pak = _write_pak( + tmp_path, + { + "drum_tab": "drum_tab.json", + "drum_tones": ENTRY_TONES, + "arrangements": [ + {"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}, + {"id": "drums", "name": "Drums", "type": "drums", + "drum_tab": "drum_tab.json", "tones": alias_tones}, + ], + }, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert len(parts) == 1 + assert parts[0]["tones"] == alias_tones + + +def test_drum_tones_does_not_leak_to_secondary_parts(tmp_path: Path): + """`drum_tones` is the PRIMARY's fallback only. A second drummer with no + binding of its own gets None — not the primary's kit.""" + live_tones = {"base": "Live Kit", "base_rig": "live-kit"} + pak = _write_pak( + tmp_path, + { + "drum_tab": "drum_tab.json", + "drum_tones": ENTRY_TONES, + "arrangements": [ + {"id": "lead", "name": "Lead", "file": "arrangements/lead.json"}, + {"id": "drums-live", "name": "Drums (Live)", "type": "drums", + "drum_tab": "drum_tab_live.json", "tones": live_tones}, + {"id": "drums-prog", "name": "Drums (Prog)", "type": "drums", + "drum_tab": "drum_tab_prog.json"}, + ], + }, + files={ + "drum_tab.json": _tab("Drums"), + "drum_tab_live.json": _tab("Drums Live"), + "drum_tab_prog.json": _tab("Drums Prog"), + }, + ) + parts = {p["id"]: p for p in _load(pak, tmp_path).drum_parts} + assert parts["drums"]["tones"] == ENTRY_TONES # primary, from drum_tones + assert parts["drums-live"]["tones"] == live_tones # own entry + assert parts["drums-prog"]["tones"] is None # no binding, no leak + + +def test_drum_parts_carry_none_when_pack_binds_nothing(tmp_path: Path): + """A pack with drums and no sound binding at all still loads, with the key + present and None — consumers can read `part["tones"]` unconditionally.""" + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json"}, + files={"drum_tab.json": _tab("Drums")}, + ) + parts = _load(pak, tmp_path).drum_parts + assert parts[0]["tones"] is None + + +def test_malformed_drum_tones_is_ignored(tmp_path: Path): + pak = _write_pak( + tmp_path, + {"drum_tab": "drum_tab.json", "drum_tones": "not-a-dict"}, + files={"drum_tab.json": _tab("Drums")}, + ) + assert _load(pak, tmp_path).drum_parts[0]["tones"] is None