From b9c037bbf1e16a26ed0606987e9b6bf274084c20 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 08:27:23 +0500 Subject: [PATCH 1/3] =?UTF-8?q?test(core):=20red=20=E2=80=94=20GPIF=20impo?= =?UTF-8?q?rts=20keep=20tapping=20and=20hammer-on=20origin=20semantics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two losses in the guitarpro 0.4.2 GPIF conversion, measured on a 410-file Guitar Pro corpus: - the Tapped note property is never read (the beat's tap effect is left a placeholder): 31 of 145 GPIF files carry tapping (1,006 Tapped note definitions) and none of it reached griff; - HopoOrigin and HopoDestination both set the one legacy hammer flag. GP3/4/5 set it on the origin note only and griff's HammerOn span follows that note; on GPIF input the destination got a span too, so adjacent independent pairs read as one chain. On the corpus, "same string" across derived hammer-on / pull-off edges is ~98% for GP3-5 but 79-85% for GP6-7. Five failing tests over authored GPIF text packed with the crate's public writer: only the origin carries the hammer span; a chain marks every origin but not the last destination; two adjacent pairs stay two pairs; a tapped note imports as a tap; a tapped chord note taps its beat (the legacy model's per-beat tapping, as for GP3/4/5). Co-Authored-By: Claude Opus 5 --- core/tests/gp_gpif_techniques.rs | 195 +++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 core/tests/gp_gpif_techniques.rs diff --git a/core/tests/gp_gpif_techniques.rs b/core/tests/gp_gpif_techniques.rs new file mode 100644 index 0000000..c9febe3 --- /dev/null +++ b/core/tests/gp_gpif_techniques.rs @@ -0,0 +1,195 @@ +//! Red → GPIF (GP6 `.gpx` / GP7 `.gp`) imports keep tapping and read +//! hammer-on/pull-off with the GP3/4/5 semantics. +//! +//! Two losses in the `guitarpro` 0.4.2 GPIF conversion, measured on a 410-file +//! corpus (31 of 145 GPIF files carry tapping): +//! +//! - the `Tapped` note property is never read (the beat's tap effect is left a +//! placeholder), so no GP6/GP7 tapping reached griff; +//! - `HopoOrigin` and `HopoDestination` both set the one legacy `hammer` flag. +//! GP3/4/5 set it on the origin note only, and griff's `HammerOn` span +//! follows that note. On GPIF input the destination got a span too, so two +//! adjacent independent hammer-on pairs were indistinguishable from one chain. +//! +//! The fixtures are authored GPIF text packed with the crate's public writer; +//! no copyrighted tab. + +#![allow( + clippy::expect_used, + clippy::unwrap_used, + clippy::missing_assert_message, + clippy::indexing_slicing +)] + +use griff_core::{ + event::{NoteMark, SpanTechnique}, + gp::import_gp_score, + score::{AtomEvent, Score}, +}; +use guitarpro::io::gpx::{compress_bcfz, pack_bcfs}; +use std::fmt::Write as _; + +/// One note: GPIF raw string (0 = lowest), fret, and enabled note properties. +struct GpifNote { + string: u8, + fret: u8, + enabled: &'static [&'static str], +} + +const fn n(string: u8, fret: u8, enabled: &'static [&'static str]) -> GpifNote { + GpifNote { + string, + fret, + enabled, + } +} + +/// A one-track Standard-E GPIF, one quarter-note beat per entry of `beats`. +fn gpif(beats: &[Vec]) -> String { + let flat: Vec<(usize, &GpifNote)> = beats + .iter() + .enumerate() + .flat_map(|(b, notes)| notes.iter().map(move |note| (b, note))) + .collect(); + let mut note_xml = String::new(); + for (id, (_, note)) in flat.iter().enumerate() { + write!( + note_xml, + r#"{}{}"#, + note.fret, note.string + ) + .expect("writing to a String"); + for p in note.enabled { + write!(note_xml, r#""#) + .expect("writing to a String"); + } + note_xml.push_str(""); + } + let mut beat_xml = String::new(); + for b in 0..beats.len() { + let ids: Vec = flat + .iter() + .enumerate() + .filter(|(_, (beat, _))| *beat == b) + .map(|(id, _)| id.to_string()) + .collect(); + write!( + beat_xml, + r#"{}"#, + ids.join(" ") + ) + .expect("writing to a String"); + } + let beat_ids: Vec = (0..beats.len()).map(|b| b.to_string()).collect(); + format!( + r#" + +7 +<![CDATA[synthetic]]> +0 + + + + +40 45 50 55 59 64 + + + +0 + +0 -1 -1 -1 +{} +{beat_xml} +{note_xml} +Quarter +"#, + beat_ids.join(" ") + ) +} + +fn import(beats: &[Vec]) -> Score { + let xml = gpif(beats); + import_gp_score(&compress_bcfz(&pack_bcfs("score.gpif", xml.as_bytes()))) + .expect("synthetic GPIF imports") +} + +/// Per event group: whether it carries a hammer-on span, and each note's tap mark. +fn groups(score: &Score) -> Vec<(bool, Vec)> { + score.tracks[0].voices[0] + .event_groups + .iter() + .map(|g| { + let hammer = g + .technique_spans + .iter() + .any(|s| s.technique == SpanTechnique::HammerOn); + let taps = g + .atoms + .iter() + .filter_map(|a| match a { + AtomEvent::Note(note) => Some(note.marks.contains(NoteMark::Tap)), + AtomEvent::Rest(_) => None, + }) + .collect(); + (hammer, taps) + }) + .collect() +} + +#[test] +fn only_the_hopo_origin_carries_the_hammer_span() { + // D string: 5 hammered to 7, then an unrelated 5. + let score = import(&[ + vec![n(2, 5, &["HopoOrigin"])], + vec![n(2, 7, &["HopoDestination"])], + vec![n(2, 5, &[])], + ]); + let hammers: Vec = groups(&score).iter().map(|g| g.0).collect(); + assert_eq!(hammers, vec![true, false, false]); +} + +#[test] +fn a_hopo_chain_marks_every_origin_and_not_the_last_destination() { + // 5 → 7 → 5: the middle note is destination and origin. + let score = import(&[ + vec![n(2, 5, &["HopoOrigin"])], + vec![n(2, 7, &["HopoDestination", "HopoOrigin"])], + vec![n(2, 5, &["HopoDestination"])], + ]); + let hammers: Vec = groups(&score).iter().map(|g| g.0).collect(); + assert_eq!(hammers, vec![true, true, false]); +} + +#[test] +fn two_adjacent_hopo_pairs_stay_two_pairs() { + // 5 → 7, then 5 → 7: no hammer span may join the first pair to the second. + let score = import(&[ + vec![n(2, 5, &["HopoOrigin"])], + vec![n(2, 7, &["HopoDestination"])], + vec![n(2, 5, &["HopoOrigin"])], + vec![n(2, 7, &["HopoDestination"])], + ]); + let hammers: Vec = groups(&score).iter().map(|g| g.0).collect(); + assert_eq!(hammers, vec![true, false, true, false]); +} + +#[test] +fn tapped_notes_are_imported_as_taps() { + // 5, tap 12, 5 on the D string. + let score = import(&[ + vec![n(2, 5, &[])], + vec![n(2, 12, &["Tapped"])], + vec![n(2, 5, &[])], + ]); + let taps: Vec> = groups(&score).into_iter().map(|g| g.1).collect(); + assert_eq!(taps, vec![vec![false], vec![true], vec![false]]); +} + +#[test] +fn a_tapped_note_in_a_chord_taps_its_beat() { + // Guitar Pro's legacy model holds tapping per beat, as GP3/4/5 do: a chord + // beat with one tapped note marks the beat's notes. Other beats stay untouched. + let score = import(&[vec![n(2, 5, &[]), n(3, 7, &["Tapped"])], vec![n(2, 5, &[])]]); + let taps: Vec> = groups(&score).into_iter().map(|g| g.1).collect(); + assert_eq!(taps, vec![vec![true, true], vec![false]]); +} From 89f3bdea60a75882231d1d17eea9715560a11493 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 08:32:35 +0500 Subject: [PATCH 2/3] fix(core): GPIF imports keep tapping and hammer-on origin semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adapter already reads the GPIF document itself (#198); it now also restores two note techniques the guitarpro 0.4.2 conversion drops or merges, walking the document exactly as the crate walks it (each track's bar per master bar, voice slots skipping -1, existing beats and notes in order) and zipping it with the converted song: - Tapped: a beat with a tapped note gets the legacy beat-level SlapEffect::Tapping — how GP3/4/5 store tapping — so its notes become NoteMark::Tap through the existing path; - HopoOrigin / HopoDestination: the legacy hammer flag is reset to HopoOrigin alone, the GP3/4/5 meaning ("legato to the next note on this string"), so a destination no longer carries a HammerOn span of its own. A voice whose converted beats or notes do not line up with the document is left as converted and reported in the loss report, never re-labelled out of step. Measured on the 410-file corpus, GP6/GP7 files (145): - files with tapped notes 0 -> 28, tapped notes 0 -> 2,013; - HammerOn spans on single notes 18,284 -> 10,906 (7,378 destination-only spans gone); the next single note is on the same string 76.8% -> 99.7%, in line with GP3-5's 99.1%; - voices left unrestored: 0. GP3/4/5 imports are unchanged (same tap and span counts). cargo test --workspace green (1,697 tests; missing_file_golden skipped locally only, as before); clippy -D warnings and fmt clean for griff-core. Co-Authored-By: Claude Opus 5 --- core/src/gp.rs | 138 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 128 insertions(+), 10 deletions(-) diff --git a/core/src/gp.rs b/core/src/gp.rs index 0493c6c..30d7e74 100644 --- a/core/src/gp.rs +++ b/core/src/gp.rs @@ -43,7 +43,10 @@ use crate::{ }, slice::TickRange, }; -use guitarpro::io::gpif::{Gpif, Property as GpifProperty, Track as GpifTrack}; +use guitarpro::io::gpif::{ + Bar as GpifBar, Beat as GpifBeat, Gpif, Note as GpifNote, Property as GpifProperty, + Track as GpifTrack, Voice as GpifVoice, +}; use guitarpro::io::gpx::{read_gp, read_gpx}; use guitarpro::model::legacy::key_signature::Duration as GpDuration; use guitarpro::model::legacy::note::NoteEffect as GpNoteEffect; @@ -158,10 +161,19 @@ pub enum GpImportError { /// Conversion losses are carried on the returned [`Score`] as a [`LossReport`]. pub fn import_gp_score(data: &[u8]) -> Result { let mut song = guitarpro::Song::default(); - match detect_gp_version(data) { - Some(3) => song.read_gp3(data)?, - Some(4) => song.read_gp4(data)?, - Some(5) => song.read_gp5(data)?, + let unrestored_voices = match detect_gp_version(data) { + Some(3) => { + song.read_gp3(data)?; + 0 + } + Some(4) => { + song.read_gp4(data)?; + 0 + } + Some(5) => { + song.read_gp5(data)?; + 0 + } Some(6) => read_gpif_song(&mut song, &read_gpx(data)?, 6), // GP7/8 decode to the same GPIF the GP6 path uses; `read_gp` unzips // `Content/score.gpif`. The Song's version.number.0 becomes 7, so @@ -169,19 +181,125 @@ pub fn import_gp_score(data: &[u8]) -> Result { // (raw repeat counts). Some(7) => read_gpif_song(&mut song, &read_gp(data)?, 7), _ => return Err(GpImportError::UnsupportedFormat), - } - Ok(gp_song_to_score(&song)) + }; + let mut score = gp_song_to_score(&song); + if unrestored_voices > 0 { + score.loss.add(ImportWarning::Other(format!( + "GPIF tapping and hammer-on origins not restored in {unrestored_voices} voice(s): \ + the converted beat/note layout differs from the GPIF document" + ))); + } + Ok(score) } // ── GPIF string orientation ─────────────────────────────────────────────────── /// Reads a parsed GPIF document (GP6 `.gpx`, GP7/8 `.gp`) into `song` — the -/// crate's own `read_gpx` / `read_gp` steps — then renumbers its strings to the -/// convention the rest of this adapter reads ([`normalise_gpif_strings`]). -fn read_gpif_song(song: &mut guitarpro::Song, gpif: &Gpif, major: u8) { +/// crate's own `read_gpx` / `read_gp` steps — then restores the note +/// techniques the conversion loses ([`restore_gpif_note_techniques`]) and +/// renumbers its strings to the convention the rest of this adapter reads +/// ([`normalise_gpif_strings`]). Returns how many voices could not be restored. +fn read_gpif_song(song: &mut guitarpro::Song, gpif: &Gpif, major: u8) -> usize { song.version.number = (major, 0, 0); song.read_gpif(gpif); + let unrestored = restore_gpif_note_techniques(song, gpif); normalise_gpif_strings(song, gpif); + unrestored +} + +/// Whitespace-separated GPIF ids (`-1` marks an empty voice slot). +fn gpif_ids(ids: &str) -> Vec { + ids.split_whitespace() + .filter_map(|id| id.parse().ok()) + .collect() +} + +/// Restores the GPIF note techniques the `guitarpro` 0.4.2 conversion drops +/// or merges: +/// +/// - **`Tapped`** is never read there (the beat's tap effect stays a +/// placeholder). A beat with a tapped note gets the legacy beat-level +/// `SlapEffect::Tapping`, exactly how GP3/4/5 store tapping, so the rest of +/// this adapter marks its notes `NoteMark::Tap`; +/// - **`HopoOrigin` / `HopoDestination`** both set the one legacy `hammer` +/// flag there, while GP3/4/5 set it on the origin note only (the flag means +/// "legato to the next note on this string"). The flag is reset to +/// `HopoOrigin` alone. +/// +/// The GPIF document is walked the way the crate walks it — each track's bar +/// per master bar, voice slots skipping `-1`, then existing beats and notes in +/// order — and zipped with the converted song. A voice whose converted beats +/// or notes do not line up with the document is left as converted and +/// counted, never re-labelled out of step. Returns that count. +fn restore_gpif_note_techniques(song: &mut guitarpro::Song, gpif: &Gpif) -> usize { + let bars: HashMap = gpif.bars.bars.iter().map(|b| (b.id, b)).collect(); + let voices: HashMap = gpif.voices.voices.iter().map(|v| (v.id, v)).collect(); + let beats: HashMap = gpif.beats.beats.iter().map(|b| (b.id, b)).collect(); + let notes: HashMap = gpif.notes.notes.iter().map(|n| (n.id, n)).collect(); + let enabled = |note: &GpifNote, name: &str| { + note.properties + .properties + .iter() + .any(|p| p.name == name && p.enable.is_some()) + }; + + let mut unrestored = 0_usize; + for (track_index, track) in song.tracks.iter_mut().enumerate() { + for (measure, master_bar) in track.measures.iter_mut().zip(&gpif.master_bars.master_bars) { + let Some(bar) = gpif_ids(&master_bar.bars) + .get(track_index) + .and_then(|id| bars.get(id)) + else { + continue; + }; + let voice_ids: Vec = gpif_ids(&bar.voices) + .into_iter() + .filter(|&id| id >= 0) + .collect(); + for (voice, voice_id) in measure.voices.iter_mut().zip(voice_ids) { + let Some(g_voice) = voices.get(&voice_id) else { + continue; + }; + let g_beats: Vec<&GpifBeat> = gpif_ids(&g_voice.beats) + .iter() + .filter_map(|id| beats.get(id).copied()) + .collect(); + let g_notes: Vec> = g_beats + .iter() + .map(|b| { + b.notes + .as_deref() + .map(gpif_ids) + .unwrap_or_default() + .iter() + .filter_map(|id| notes.get(id).copied()) + .collect() + }) + .collect(); + let aligned = voice.beats.len() == g_notes.len() + && voice + .beats + .iter() + .zip(&g_notes) + .all(|(beat, g)| beat.notes.len() == g.len()); + if !aligned { + unrestored = unrestored.saturating_add(1); + continue; + } + for (beat, g_beat_notes) in voice.beats.iter_mut().zip(&g_notes) { + let mut tapped = false; + for (note, g_note) in beat.notes.iter_mut().zip(g_beat_notes) { + note.effect.hammer = enabled(g_note, "HopoOrigin"); + tapped |= enabled(g_note, "Tapped"); + } + if tapped { + beat.effect.slap_effect = guitarpro::SlapEffect::Tapping; + } + } + } + } + } + unrestored } /// A GPIF track's tuning as stored: open-string pitches, **lowest string From f0ca08f6c6d9b3a6c5aa104d6eaec057bea3600b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 08:32:36 +0500 Subject: [PATCH 3/3] docs: record the GPIF technique-fidelity decision Co-Authored-By: Claude Opus 5 --- docs/decisions.log.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/decisions.log.md b/docs/decisions.log.md index d2c5e27..e2bdde8 100644 --- a/docs/decisions.log.md +++ b/docs/decisions.log.md @@ -2834,3 +2834,20 @@ Architectural decisions go to [`adr/`](adr/) instead. fingering audit (PhysShell/griff#197), whose line normalization masked the GP6 half. The crate defect is upstream's to fix; this adapter no longer depends on it. + +- 2026-09-17 — In the context of GPIF imports losing note techniques in + `guitarpro` 0.4.2 (the `Tapped` property never read; `HopoOrigin` and + `HopoDestination` merged into one hammer flag), we decided to **restore + them at the import boundary in griff, walking the GPIF document as the + crate walks it, rather than depend on a fork of the crate**, to achieve + GP6/7 technique labels with the same semantics as GP3/4/5 now, accepting + a second workaround next to the tuning one (#198) until upstream ships a + fix. A git dependency on a fork is ruled out by `deny.toml` + (`unknown-git = "deny"`) and would need vendoring hashes in the nix and + wasm builds; the fix is instead offered upstream (Codeberg + `slundi/scorelib`), and griff drops the workaround once a release carries + it, keeping its regression tests. Measured on the corpus's 145 GPIF files: + tapped notes 0 → 2,013, destination-only hammer spans removed (18,284 → + 10,906), hammer edges on the same string 76.8% → 99.7%; GP3/4/5 unchanged. + Deriving hammer-on versus pull-off direction for all formats is a separate + decision (it changes corpus technique tags everywhere).