mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-10 18:59:56 +00:00
feat(sloppak): load multiple drum parts (feedpak 1.17.0 drums-as-arrangements) (#1020)
* feat(sloppak): load multiple drum parts (feedpak 1.17.0 drums-as-arrangements)
A song can now ship SEVERAL drum charts (a second drummer, an aux-percussion
layer). The Arrangement Editor already writes them per the feedpak 1.17.0 FEP
(feedpak-spec#63): the primary stays the song-level `drum_tab:` key (what this
app has always played), and each part rides the manifest as a `type: drums`
arrangement entry carrying a per-arrangement `drum_tab` file pointer and NO
note `file` — an entry this loader's file/notation gate already skips, which
is exactly why old builds are unaffected by such packs.
lib/sloppak.py:
- The arrangements loop collects drum-part pointer entries instead of merely
skipping them — but still NEVER turns one into a fretted Arrangement. That
skip is the grading invariant (an empty drum chart must not reach the
fretted pipeline / note-detection grading) and is now pinned by test.
- New `LoadedSloppak.drum_parts`: [{id, name, drum_tab}], primary FIRST. The
entry aliasing the song-level file contributes its id/name but is never
loaded twice (the primary's payload IS `loaded.drum_tab`, same object).
Legacy single-drum packs read as a one-part list; a pointer-only pack (a
writer omitted the alias) promotes its first part so has_drum_tab, the
default stream, and the drum-only placeholder keep working.
- The song-level drum_tab loading block is extracted verbatim into
`_load_drum_tab_file()` and shared by both paths, so every part gets the
same permissive posture: missing file → that part silently absent;
traversal / parse / validation failure → that part skipped with a warning,
never an aborted load. (The 9 pinned drumtab-load tests pass unchanged.)
lib/routers/ws_highway.py:
- `song_info` gains `drum_parts` (names only; always a list, empty without
drums) so a part picker can bind unconditionally.
- `?drum_part=<id>` on the WS URL selects which part's tab streams as the
`drum_tab`/`drum_hits` messages; the default and any unknown id fall back
to the primary — byte-identical legacy behavior. The `drum_tab` message
carries `part_id` only when a parts list exists, keeping the legacy frame
unchanged.
Tests: tests/test_sloppak_drum_parts.py (9) — the grading invariant +
parallel-ids pin, primary-first resolution with alias identity, legacy
one-part list, pointer-only promotion, per-part failure isolation (bad JSON,
path traversal, duplicate rels), and the drum-only placeholder with pointer
entries. Full suite: the only failures are 9 machine-environmental tests
(installed desktop plugins under LOCALAPPDATA, CRLF/path-shape assertions)
that fail identically on an untouched origin/main checkout on this box.
tools/check_spec_conformance.py passes against the spec's current HEAD
(`drum_tab` and `type` are declared keys); the semantics of the
per-arrangement placement land in feedpak-spec#63 — this PR should merge
after it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
* Fix drum-part review findings
* Normalize drum part pointer identities
* fix(sloppak): enforce drums grading invariant + green the suite
- Gate the drum-pointer skip on type FIRST: a type:drums/drum entry never becomes
a fretted Arrangement even if it carries a note file/notation (with drum_tab it
is collected as a drum part, without it dropped+warned). Closes the spec
§5.2/§7.5 MUST-NOT hole (a malformed drums+file entry was being fretted-graded).
- Make test_drum_pointer_with_wrong_type_logs_warning robust (attach handler to the
feedBack logger + set WARNING, restore in finally) and fix the root-cause level
leak in test_tuning_provider_isolation.py (finally restored the handler but not
the level, leaking ERROR onto the feedBack tree and turning the suite red under
full ordering).
- Restore the chart-transform CHANGELOG bullet (#952) the drum entry had truncated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
ChrisBeWithYou
co-authored by
Claude Opus 4.8
byrongamatos
Claude Opus 4.8
byrongamatos
parent
a9be210f77
commit
605dbdfd25
@@ -0,0 +1,17 @@
|
||||
"""Wire-compatibility coverage for selectable drum parts."""
|
||||
|
||||
from routers.ws_highway import _drum_part_id_for_wire
|
||||
|
||||
|
||||
def test_single_synthesized_part_keeps_legacy_frame_without_part_id():
|
||||
parts = [{"id": "drums", "name": "Drums", "drum_tab": {}}]
|
||||
assert _drum_part_id_for_wire(parts, "drums") is None
|
||||
|
||||
|
||||
def test_multiple_parts_expose_selected_part_id():
|
||||
parts = [
|
||||
{"id": "drums", "name": "Drums", "drum_tab": {}},
|
||||
{"id": "drums-2", "name": "Aux", "drum_tab": {}},
|
||||
]
|
||||
assert _drum_part_id_for_wire(parts, "drums-2") == "drums-2"
|
||||
assert _drum_part_id_for_wire(parts, None) is None
|
||||
@@ -0,0 +1,295 @@
|
||||
"""Loader coverage for MULTIPLE drum parts (feedpak 1.17.0 "drums as
|
||||
arrangements").
|
||||
|
||||
A drum part rides the manifest as a `type: drums` arrangement entry carrying
|
||||
a per-arrangement `drum_tab` file pointer and NO note `file`. The loader:
|
||||
|
||||
- NEVER turns a pointer entry into a fretted Arrangement — that skip is
|
||||
the grading invariant (an empty drum chart must not reach the fretted
|
||||
pipeline, where note detection would grade it as garbage);
|
||||
- resolves the parts into `LoadedSloppak.drum_parts`, primary FIRST: the
|
||||
entry aliasing the song-level `drum_tab:` file contributes its id/name
|
||||
but is never loaded twice (its payload IS `loaded.drum_tab`);
|
||||
- loads each extra part's file with the same permissive posture as the
|
||||
song-level tab (a bad part disables that part only, never the load);
|
||||
- copes with a pointer-only pack (no song-level key): the first part
|
||||
becomes the primary so every legacy consumer keeps working.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import logging
|
||||
from pathlib import Path
|
||||
|
||||
import yaml
|
||||
|
||||
import sloppak as sloppak_mod
|
||||
|
||||
|
||||
def _tab(name: str, hits: list[dict] | None = None) -> dict:
|
||||
return {
|
||||
"version": 1,
|
||||
"name": name,
|
||||
"kit": [{"id": "kick", "name": "Kick"}],
|
||||
"hits": hits if hits is not None else [{"t": 1.0, "p": "kick", "v": 100}],
|
||||
}
|
||||
|
||||
|
||||
def _write_pak(root: Path, manifest_extras: dict, files: dict[str, dict | str]) -> Path:
|
||||
"""A minimal directory-form sloppak with one Lead arrangement plus the
|
||||
given extra files ({relpath: json-dict-or-raw-text})."""
|
||||
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": [],
|
||||
}
|
||||
(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.items():
|
||||
text = payload if isinstance(payload, str) else json.dumps(payload)
|
||||
(pak / rel).write_text(text)
|
||||
return pak
|
||||
|
||||
|
||||
def _load(pak_path: Path, tmp_path: Path):
|
||||
dlc_root = pak_path.parent
|
||||
cache = tmp_path / "cache"
|
||||
cache.mkdir()
|
||||
return sloppak_mod.load_song(pak_path.name, dlc_root, cache)
|
||||
|
||||
|
||||
def _two_part_manifest() -> dict:
|
||||
"""The exact shape the editor writes: primary alias entry + one extra."""
|
||||
return {
|
||||
"drum_tab": "drum_tab.json",
|
||||
"arrangements": [
|
||||
{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"},
|
||||
{"id": "drums", "name": "Drums", "type": "drums",
|
||||
"drum_tab": "drum_tab.json"},
|
||||
{"id": "drums-2", "name": "Drums (Live)", "type": "drums",
|
||||
"drum_tab": "drum_tab_drums-2.json"},
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
# ── The grading invariant ────────────────────────────────────────────────────
|
||||
|
||||
def test_pointer_entries_never_become_fretted_arrangements(tmp_path: Path):
|
||||
pak = _write_pak(tmp_path, _two_part_manifest(), {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
"drum_tab_drums-2.json": _tab("Drums (Live)"),
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
# Only the Lead chart is an Arrangement — neither drum part enters the
|
||||
# fretted pipeline (song.arrangements is what note detection grades).
|
||||
assert [a.name for a in loaded.song.arrangements] == ["Lead"]
|
||||
# And the ids list stays parallel to song.arrangements (skipped entries
|
||||
# contribute nothing) — a misalignment here would remap every chart edit.
|
||||
assert loaded.arrangement_ids == ["lead"]
|
||||
|
||||
|
||||
def test_drums_typed_entry_with_note_file_never_frets(tmp_path: Path):
|
||||
# A malformed entry: type:drums but ALSO carrying a note `file`. Keying the
|
||||
# skip on file absence would let it through as a fretted, selectable,
|
||||
# gradeable Arrangement (spec §5.2/§7.5 MUST-NOT). Routing on `type` first
|
||||
# drops it instead — it never reaches song.arrangements.
|
||||
bogus = {
|
||||
"name": "Bogus", "tuning": [0, 0, 0, 0, 0, 0], "capo": 0,
|
||||
"notes": [], "chords": [], "anchors": [], "handshapes": [],
|
||||
"templates": [], "beats": [], "sections": [],
|
||||
}
|
||||
pak = _write_pak(tmp_path, {
|
||||
"arrangements": [
|
||||
{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"},
|
||||
{"id": "bad", "name": "Bogus", "type": "drums",
|
||||
"file": "arrangements/bogus.json"},
|
||||
],
|
||||
}, {"arrangements/bogus.json": bogus})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [a.name for a in loaded.song.arrangements] == ["Lead"]
|
||||
assert loaded.arrangement_ids == ["lead"]
|
||||
|
||||
|
||||
# ── Parts resolution ─────────────────────────────────────────────────────────
|
||||
|
||||
def test_two_parts_resolve_primary_first_with_alias_identity(tmp_path: Path):
|
||||
pak = _write_pak(tmp_path, _two_part_manifest(), {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
"drum_tab_drums-2.json": _tab("Drums (Live)", [{"t": 2.0, "p": "kick", "v": 90}]),
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert loaded.drum_parts is not None
|
||||
assert [(p["id"], p["name"]) for p in loaded.drum_parts] == [
|
||||
("drums", "Drums"), ("drums-2", "Drums (Live)"),
|
||||
]
|
||||
# The primary's payload IS the song-level tab — same object, loaded once.
|
||||
assert loaded.drum_parts[0]["drum_tab"] is loaded.drum_tab
|
||||
assert loaded.drum_parts[1]["drum_tab"]["hits"][0]["t"] == 2.0
|
||||
|
||||
|
||||
def test_primary_pointer_equivalent_path_is_not_duplicated(tmp_path: Path):
|
||||
manifest = {
|
||||
"drum_tab": "drum_tab.json",
|
||||
"arrangements": [
|
||||
{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"},
|
||||
{"id": "kit", "name": "Live Kit", "type": "drums",
|
||||
"drum_tab": "./drum_tab.json"},
|
||||
],
|
||||
}
|
||||
pak = _write_pak(tmp_path, manifest, {"drum_tab.json": _tab("Drums")})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert loaded.drum_parts is not None
|
||||
assert [(p["id"], p["name"]) for p in loaded.drum_parts] == [
|
||||
("kit", "Live Kit"),
|
||||
]
|
||||
assert loaded.drum_parts[0]["drum_tab"] is loaded.drum_tab
|
||||
|
||||
|
||||
def test_legacy_single_drum_pack_gets_a_one_part_list(tmp_path: Path):
|
||||
pak = _write_pak(tmp_path, {"drum_tab": "drum_tab.json"}, {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert loaded.drum_parts is not None and len(loaded.drum_parts) == 1
|
||||
assert loaded.drum_parts[0]["id"] == "drums"
|
||||
assert loaded.drum_parts[0]["drum_tab"] is loaded.drum_tab
|
||||
|
||||
|
||||
def test_no_drums_means_no_parts(tmp_path: Path):
|
||||
pak = _write_pak(tmp_path, {}, {})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert loaded.drum_parts is None
|
||||
assert loaded.drum_tab is None
|
||||
|
||||
|
||||
def test_pointer_only_pack_promotes_the_first_part_to_primary(tmp_path: Path):
|
||||
# A writer that omitted the song-level alias: readers must cope (the
|
||||
# spec keeps the alias, but a reader never crashes on its absence).
|
||||
pak = _write_pak(tmp_path, {
|
||||
"arrangements": [
|
||||
{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"},
|
||||
{"id": "kit", "name": "Kit", "type": "drums",
|
||||
"drum_tab": "drum_tab_kit.json"},
|
||||
],
|
||||
}, {"drum_tab_kit.json": _tab("Kit")})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert loaded.drum_parts is not None and len(loaded.drum_parts) == 1
|
||||
# The part's tab becomes THE drum tab, so has_drum_tab / the default
|
||||
# stream / the drum-only placeholder all keep working.
|
||||
assert loaded.drum_tab is loaded.drum_parts[0]["drum_tab"]
|
||||
assert loaded.drum_parts[0]["id"] == "kit"
|
||||
|
||||
|
||||
# ── Permissive per-part failure ──────────────────────────────────────────────
|
||||
|
||||
def test_a_bad_extra_part_disables_that_part_only(tmp_path: Path):
|
||||
manifest = _two_part_manifest()
|
||||
manifest["arrangements"].append(
|
||||
{"id": "drums-3", "name": "Broken", "type": "drums",
|
||||
"drum_tab": "drum_tab_broken.json"})
|
||||
pak = _write_pak(tmp_path, manifest, {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
"drum_tab_drums-2.json": _tab("Drums (Live)"),
|
||||
"drum_tab_broken.json": "not json {{{",
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [p["id"] for p in loaded.drum_parts] == ["drums", "drums-2"]
|
||||
|
||||
|
||||
def test_a_traversal_part_path_is_skipped(tmp_path: Path):
|
||||
manifest = _two_part_manifest()
|
||||
manifest["arrangements"][2]["drum_tab"] = "../outside.json"
|
||||
(tmp_path / "outside.json").write_text(json.dumps(_tab("Evil")))
|
||||
pak = _write_pak(tmp_path, manifest, {"drum_tab.json": _tab("Drums")})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [p["id"] for p in loaded.drum_parts] == ["drums"]
|
||||
|
||||
|
||||
def test_duplicate_pointer_rels_load_once(tmp_path: Path):
|
||||
manifest = _two_part_manifest()
|
||||
manifest["arrangements"].append(
|
||||
{"id": "drums-dup", "name": "Dup", "type": "drums",
|
||||
"drum_tab": "drum_tab_drums-2.json"})
|
||||
pak = _write_pak(tmp_path, manifest, {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
"drum_tab_drums-2.json": _tab("Drums (Live)"),
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [p["id"] for p in loaded.drum_parts] == ["drums", "drums-2"]
|
||||
|
||||
|
||||
def test_duplicate_part_ids_are_made_unique(tmp_path: Path):
|
||||
manifest = _two_part_manifest()
|
||||
manifest["arrangements"][2]["id"] = "drums"
|
||||
manifest["arrangements"].append(
|
||||
{"id": "drums-2", "name": "Aux", "type": "drums",
|
||||
"drum_tab": "drum_tab_aux.json"})
|
||||
pak = _write_pak(tmp_path, manifest, {
|
||||
"drum_tab.json": _tab("Drums"),
|
||||
"drum_tab_drums-2.json": _tab("Drums (Live)"),
|
||||
"drum_tab_aux.json": _tab("Aux"),
|
||||
})
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [p["id"] for p in loaded.drum_parts] == ["drums", "drums-2", "drums-3"]
|
||||
|
||||
|
||||
def test_drum_pointer_with_wrong_type_logs_warning(tmp_path: Path, caplog):
|
||||
pak = _write_pak(tmp_path, {
|
||||
"arrangements": [
|
||||
{"id": "lead", "name": "Lead", "file": "arrangements/lead.json"},
|
||||
{"id": "typo", "type": "druns", "drum_tab": "drum_tab_typo.json"},
|
||||
],
|
||||
}, {"drum_tab_typo.json": _tab("Typo")})
|
||||
# feedBack sets propagate=False, so pytest's root capture sees nothing from
|
||||
# it — attach caplog's handler to the feedBack logger and pin WARNING
|
||||
# regardless of ambient level (a sibling test can leak ERROR onto this tree).
|
||||
lg = logging.getLogger("feedBack")
|
||||
orig_level = lg.level
|
||||
lg.addHandler(caplog.handler)
|
||||
lg.setLevel(logging.WARNING)
|
||||
try:
|
||||
loaded = _load(pak, tmp_path)
|
||||
finally:
|
||||
lg.removeHandler(caplog.handler)
|
||||
lg.setLevel(orig_level)
|
||||
assert loaded.drum_parts is None
|
||||
assert "has drum_tab" in caplog.text and "type='druns'" in caplog.text
|
||||
|
||||
|
||||
# ── Drum-only pack with parts ────────────────────────────────────────────────
|
||||
|
||||
def test_drum_only_pack_with_pointer_entries_still_synthesizes_placeholder(tmp_path: Path):
|
||||
# No pitched arrangements at all, drums via pointer entries only: the
|
||||
# placeholder "Drums" arrangement must still appear so the highway WS
|
||||
# proceeds and the tab reaches the drum highway.
|
||||
pak = _write_pak(tmp_path, {
|
||||
"arrangements": [
|
||||
{"id": "kit", "name": "Kit", "type": "drums",
|
||||
"drum_tab": "drum_tab_kit.json"},
|
||||
],
|
||||
}, {"drum_tab_kit.json": _tab("Kit", [{"t": 5.0, "p": "kick", "v": 100}])})
|
||||
# Remove the Lead arrangement _write_pak added to the manifest.
|
||||
manifest_path = pak / "manifest.yaml"
|
||||
manifest = yaml.safe_load(manifest_path.read_text())
|
||||
manifest["arrangements"] = [e for e in manifest["arrangements"] if e.get("id") != "lead"]
|
||||
manifest.pop("duration", None)
|
||||
manifest_path.write_text(yaml.safe_dump(manifest, sort_keys=False))
|
||||
loaded = _load(pak, tmp_path)
|
||||
assert [a.name for a in loaded.song.arrangements] == ["Drums"]
|
||||
assert loaded.drum_parts is not None and loaded.drum_parts[0]["id"] == "kit"
|
||||
# Song length derived from the last hit (the drum-only path's rule).
|
||||
assert loaded.song.song_length > 5.0
|
||||
@@ -60,12 +60,14 @@ def test_the_failure_is_actually_logged(registry, caplog):
|
||||
# capture_logger() context manager for this, but it is not importable from here:
|
||||
# pyproject pins pythonpath to [".", "lib"], so `tests` is not a package.)
|
||||
lg = logging.getLogger("feedBack")
|
||||
orig_level = lg.level
|
||||
lg.addHandler(caplog.handler)
|
||||
lg.setLevel(logging.ERROR)
|
||||
try:
|
||||
registry.get_merged()
|
||||
finally:
|
||||
lg.removeHandler(caplog.handler)
|
||||
lg.setLevel(orig_level) # restore, or ERROR leaks onto the feedBack tree
|
||||
|
||||
assert any("bad-plugin" in r.getMessage() for r in caplog.records), (
|
||||
"the raising provider was never named in the logs"
|
||||
|
||||
Reference in New Issue
Block a user