mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-21 20:31:21 +00:00
Restore GP6/7/8 tremolo picking on import (#572)
* Map GP7/8 tremolo picking on import GP7/8 (GPIF) encodes tremolo picking as a beat-level <Tremolo> element, which the importer ignored — so tremolo was silently dropped on .gp import, while note vibrato and the GP3-5 path were unaffected. Read the beat-level <Tremolo> and set the note tremolo flag across the beat, independent of vibrato (a note can carry both). Signed-off-by: Sin <deathlysin@outlook.com> * test: cover GP6/7/8 tremolo-picking import Extract the beat-level <Tremolo> detection into a pure _beat_has_tremolo helper (mirroring the tested _note_has_vibrato) so it's unit-testable in this suite's fixture-free style, then add: - 4 unit tests on _beat_has_tremolo: direct <Tremolo> child detected (rate-agnostic), absent -> False, direct-child-only (nested Tremolo ignored), independent of the VibratoWTremBar whammy property. - 1 end-to-end test driving convert_file via a crafted GPIF (monkeypatched _load_gpif): a tremolo-picked beat's note serializes tremolo="1" while a plain beat stays "0". Both the detection and integration tests fail without the fix; full GP suite 238 passed. Refactor is behavior-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Sin <deathlysin@outlook.com> Co-authored-by: Byron Gamatos <xasiklas@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Sin
GitHub
parent
32127bc70b
commit
37aedd4251
@ -722,6 +722,20 @@ def _note_has_vibrato(note_el: ET.Element, prop_map: dict) -> bool:
|
||||
return 'Vibrato' in prop_map or note_el.find('Vibrato') is not None
|
||||
|
||||
|
||||
def _beat_has_tremolo(beat_el: ET.Element) -> bool:
|
||||
"""True if a GP7/GP8 beat carries tremolo picking.
|
||||
|
||||
GPIF encodes tremolo picking as a DIRECT beat-level
|
||||
``<Tremolo>1/8</Tremolo>`` child of ``<Beat>`` (the value is the rate). The
|
||||
RS note model has a single boolean tremolo flag with no rate, so the rate is
|
||||
intentionally ignored — any tremolo-picked beat maps to note tremolo across
|
||||
it. Matched as a direct child (not ``.//``) so it is never confused with the
|
||||
whammy-bar ``VibratoWTremBar`` Property, a separate beat-level effect
|
||||
handled elsewhere.
|
||||
"""
|
||||
return beat_el.find('Tremolo') is not None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# list_tracks — mirrors gp2rs.list_tracks interface
|
||||
# ---------------------------------------------------------------------------
|
||||
@ -1696,6 +1710,17 @@ def convert_file(
|
||||
for _bn in beat_rs_notes:
|
||||
_bn.vibrato = True
|
||||
|
||||
# Tremolo picking: GP7/GP8 encodes the rate as a
|
||||
# beat-level <Tremolo>1/8</Tremolo> child. The note
|
||||
# model has a single tremolo flag (no rate), so map
|
||||
# any tremolo-picked beat to note tremolo across it.
|
||||
# Independent of vibrato above — a note can carry
|
||||
# both. (Beat-level <Tremolo>, not the whammy
|
||||
# VibratoWTremBar Property, which is handled above.)
|
||||
if _beat_has_tremolo(beat_el):
|
||||
for _bn in beat_rs_notes:
|
||||
_bn.tremolo = True
|
||||
|
||||
if len(beat_rs_notes) == 1:
|
||||
rs_notes.append(beat_rs_notes[0])
|
||||
elif len(beat_rs_notes) > 1:
|
||||
|
||||
@ -21,6 +21,7 @@ from gp2rs_gpx import (
|
||||
_safe_filename_stem,
|
||||
_note_is_tie,
|
||||
_note_has_vibrato,
|
||||
_beat_has_tremolo,
|
||||
_note_midi,
|
||||
_gpx_percussion_midis,
|
||||
_gpx_tuning,
|
||||
@ -558,6 +559,55 @@ def test_convert_file_gp8_ascending_tuning_not_mirrored(tmp_path, monkeypatch):
|
||||
assert (5, 0) in placed # high-e open note on RS string 5
|
||||
|
||||
|
||||
# Beat 0 carries a beat-level <Tremolo>; beat 1 does not. End-to-end proof that
|
||||
# the picked beat's note serializes tremolo="1" and the other stays "0".
|
||||
_GPIF_GUITAR_TREMOLO = """
|
||||
<GPIF>
|
||||
<Score><Title>T</Title><Artist>A</Artist></Score>
|
||||
<Tracks>
|
||||
<Track id="0"><Name>Lead Guitar</Name>
|
||||
<Property name="Tuning"><Pitches>40 45 50 55 59 64</Pitches></Property></Track>
|
||||
</Tracks>
|
||||
<MasterBars>
|
||||
<MasterBar><Time>4/4</Time><Bars>0</Bars></MasterBar>
|
||||
</MasterBars>
|
||||
<Bars>
|
||||
<Bar id="0"><Voices>0</Voices></Bar>
|
||||
</Bars>
|
||||
<Voices>
|
||||
<Voice id="0"><Beats>0 1</Beats></Voice>
|
||||
</Voices>
|
||||
<Beats>
|
||||
<Beat id="0"><Rhythm ref="r0"/><Tremolo>1/8</Tremolo><Notes>0</Notes></Beat>
|
||||
<Beat id="1"><Rhythm ref="r0"/><Notes>1</Notes></Beat>
|
||||
</Beats>
|
||||
<Notes>
|
||||
<Note id="0">
|
||||
<Property name="String"><String>0</String></Property>
|
||||
<Property name="Fret"><Fret>0</Fret></Property></Note>
|
||||
<Note id="1">
|
||||
<Property name="String"><String>5</String></Property>
|
||||
<Property name="Fret"><Fret>0</Fret></Property></Note>
|
||||
</Notes>
|
||||
<Rhythms><Rhythm id="r0"><NoteValue>Quarter</NoteValue></Rhythm></Rhythms>
|
||||
</GPIF>
|
||||
"""
|
||||
|
||||
|
||||
def test_convert_file_gp8_tremolo_beat_flags_note(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(gp2rs_gpx, "_load_gpif", lambda _p: ET.fromstring(_GPIF_GUITAR_TREMOLO))
|
||||
out_files = convert_file(
|
||||
"dummy.gp", str(tmp_path),
|
||||
track_indices=[0], arrangement_names={0: "Lead"},
|
||||
)
|
||||
root = ET.parse(out_files[0]).getroot()
|
||||
tremolo_by_string = {int(n.get("string")): n.get("tremolo")
|
||||
for n in root.iter() if n.tag == "note"}
|
||||
# Beat 0 note (RS string 0) was tremolo-picked; beat 1 note (string 5) wasn't.
|
||||
assert tremolo_by_string[0] == "1"
|
||||
assert tremolo_by_string[5] == "0"
|
||||
|
||||
|
||||
def test_vocal_pitch_sidecar_sorts_multi_voice_by_time():
|
||||
# Two voices in one bar. Voice 0 (traversed first) emits its lyric note at
|
||||
# t=0.5 (a no-lyric quarter precedes it); voice 1 (traversed second) emits
|
||||
@ -732,6 +782,40 @@ def test_note_vibrato_ignores_whammy_trembar_property():
|
||||
assert _note_has_vibrato(n, tp) is False
|
||||
|
||||
|
||||
# ── _beat_has_tremolo (GP6/7/8 tremolo-picking import) ──────────────────────
|
||||
|
||||
def test_beat_tremolo_direct_element():
|
||||
# GPIF encodes tremolo picking as a direct <Tremolo>rate</Tremolo> child of
|
||||
# <Beat> — the regression this fixes (GPX never read it). Rate-agnostic.
|
||||
for rate in ("1/8", "1/16", "1/32"):
|
||||
b = ET.fromstring(f'<Beat id="1"><Rhythm ref="1"/>'
|
||||
f'<Tremolo>{rate}</Tremolo></Beat>')
|
||||
assert _beat_has_tremolo(b) is True
|
||||
|
||||
|
||||
def test_beat_tremolo_absent_is_false():
|
||||
b = ET.fromstring('<Beat id="1"><Rhythm ref="1"/><Notes>1</Notes></Beat>')
|
||||
assert _beat_has_tremolo(b) is False
|
||||
|
||||
|
||||
def test_beat_tremolo_is_direct_child_only():
|
||||
# Matched as a direct child (not `.//`), so a <Tremolo> buried deeper must
|
||||
# NOT trigger — guards against a false positive from unrelated nested markup.
|
||||
b = ET.fromstring('<Beat id="1"><Properties>'
|
||||
'<Property name="X"><Tremolo>1/8</Tremolo></Property>'
|
||||
'</Properties></Beat>')
|
||||
assert _beat_has_tremolo(b) is False
|
||||
|
||||
|
||||
def test_beat_tremolo_independent_of_whammy_trembar():
|
||||
# VibratoWTremBar is the separate whammy-bar effect; a beat carrying only
|
||||
# that (no <Tremolo>) is not tremolo picking.
|
||||
b = ET.fromstring('<Beat id="1"><Properties>'
|
||||
'<Property name="VibratoWTremBar"><Strength>Slight</Strength></Property>'
|
||||
'</Properties></Beat>')
|
||||
assert _beat_has_tremolo(b) is False
|
||||
|
||||
|
||||
# ── _gpif_left_fingering (GP7/GP8 per-note fret-hand finger -> fg) ───────────
|
||||
# GPIF stores a single note's fret-hand finger as a direct <LeftFingering>
|
||||
# child of <Note> (NOT a <Property>), with classical p-i-m-a-c letter codes —
|
||||
|
||||
Loading…
Reference in New Issue
Block a user