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 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]]); +} 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).