Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In conversion from musicXML, allow multiple {Fret/String}Indications for chord. #1673

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

seffka
Copy link

@seffka seffka commented Dec 13, 2023

Currently, it's not possible to access frets and string information in guitar tablatures for chords after conversion from musicXML, because StringIndication and FretIndication objects are stored in chord.articulation property only for a single (lowest) note of a chord.

E.g., when running the attached example with very simple musicXML, expected:

<music21.chord.Chord D3 A3 D4 F4 A4> [<music21.articulations.FretIndication 5>, <music21.articulations.StringIndication 5>, <music21.articulations.FretIndication 7>, <music21.articulations.StringIndication 4>, <music21.articulations.FretIndication 7>, <music21.articulations.StringIndication 3>, <music21.articulations.FretIndication 6>, <music21.articulations.StringIndication 2>, <music21.articulations.FretIndication 5>, <music21.articulations.StringIndication 1>]

but have only:
<music21.chord.Chord D3 A3 D4 F4 A4> [<music21.articulations.FretIndication 5>, <music21.articulations.StringIndication 5>]

chord_test.zip

Suggested fix is following already established path, i.e., similar to excluding articulations.Fingering from articulations uniqueness check.

@mscuthbert
Copy link
Member

Great! In the future please try to add a test, but this is pretty complex code, and your solution will work.

The inner notes of a chord have an articulations class, and we should be adding the indications there too, but that's a major refactor that I don't want to do now.

@coveralls
Copy link

coveralls commented Jan 2, 2024

Coverage Status

coverage: 93.037% (-0.005%) from 93.042%
when pulling 529febb on seffka:master
into a938a8c on cuthbertLab:master.

@mscuthbert
Copy link
Member

mscuthbert commented Jan 2, 2024

Please fix the line-too-long Done

@mscuthbert mscuthbert merged commit 815821a into cuthbertLab:master Jan 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants