Handling Staff-tuning in MusicXML (fixes #778)

[louisbigo]

I followed your recommendations in #778 (thanks for this). I realized that I will have to use StaffLayout as

In music21, the <staff-layout> and <staff-details> are
intertwined in a StaffLayout object.

so I think this will need to be handled in xmlStaffLayoutFromStaffDetails. But therefore I can not drop the identified fretboard in the beginning of the part as you suggested, so I tried to simply add it as a new attribute in the StaffLayer. Hope it makes sense.

I still haven't added any test, I wanted to first make sure we agree on the direction.

[coveralls]

Coverage increased (+0.002%) to 93.079% when pulling 68d1213 on louisbigo:staff-tuning into c2f5c6d on cuthbertLab:master.

[jacobtylerwalls]

Thanks for taking this on! I think we shouldn't couple the fretboards too tightly to m21 StaffDetails objects just because MusicXML couples them (what about other formats, etc.)

[jacobtylerwalls]

But therefore I can not drop the identified fretboard in the beginning of the part as you suggested, so I tried to simply add it as a new attribute in the StaffLayer.

Can we just insert into the current measure? staffLayoutToXmlStaffDetails is a method on the MeasureExporter, so we have access to self.stream.

[louisbigo]

self.stream.append(fretboard) does not work because fretboard is not a music21 object (the err message recommends to create a music21.ElementWrapper, should we ?)

[jacobtylerwalls]

Goodness, alright. My mistake. Looking now, what do you suppose about setting .stringPitches on self.activeInstrument, if it is a StringInstrument? We don't even need to make or check fretboards, I guess.

[jacobtylerwalls]

This is probably worth a ping for @mscuthbert before you end up wasting too much motion! Myke, any thoughts about these three options:

1a -- use FretBoards, and put them on StaffLayout objects
1b -- use FretBoards, but make them Music21Objects so they can go in streams
2 -- use StringInstrument.stringPitches

[jacobtylerwalls]

One benefit of the instrument approach is that if we have ambiguous data for an instrument but we have staff-tuning tags, we could possibly reclassify the object to be a StringInstrument, or possibly even more specific.

[mscuthbert]

I think that I'm a bit confused also. We can have ChordWithFretBoard, but we can also have the same thing without a Chord symbol. That's what I'm talking about that I call a FretBoard. I think I'll need to see an image of what everyone is talking about to catch up. But it doesn't seem like any of this belongs on StaffLayout unless it affects the size or amount of space around a staff or the change in the visual appearance. If it's about the tuning of different lines, that's a different object. I don't want to put things into one object that do not belong with it.

[louisbigo]

The FretBoard definition in the music21 doc says : A FretBoard represents a displayed fretboard (i.e. used in chord symbols).. So I think that the FretBoard class has initially be thought to handle the displaying of string/frets positions accompanying chord symbols (see image below).

So the question is whether the class FretBoard could also be used to describe the string instrument linked to a whole tablature staff. Looking at the code of FretBoard, it seems to me that it is too related to positions (see for examples methods getFretNoteByString which does not make sense in the context of a whole tablature staff). So I'm starting to think that we might not want to use Fretboard here (I'm also wondering if to avoid futur similar confusion, we could consider renaming the class FretBoard in something like FretBoardPosition, but it would require changing the name of many related classes / methods with similarly ambiguous names in tablature.py like FretNote, etc.).

On the other hand, I agree that the tuning of the string instrument is not supposed to affect the layout of the staff so using StaffLayout might not seem justified here. Unfortunately it does not seem that <staff-tuning> , as a <staff-details> child, could be easily be processed outside xmlStaffLayoutFromStaffDetails. Any idea of where it could be processed ?

[jacobtylerwalls]

So I'm starting to think that we might not want to use Fretboard here

I agree, we need some other data structure in music21 for tab staves. The image of what we're talking about can be found from the tutorial Louis linked to--we need to capture the pitches of the lines of a tab-staff, which may or may not (although it should) be in the same musicxml <part>:

I'm thinking the StaffType enum needs a new member TAB, and StaffTypes do currently exist on StaffLayout objects. Myke, what do you think of that?

Where to encode the actual pitches, I'm not sure. We could add attributes to TabClef or we could keep StringInstrument.stringPitches up to date with the pitches of the tab staff. 🤷🏻‍♂️

Any idea of where it could be processed ?

handleStaffDetails() could call a new method to read the <staff-tuning> tag.

[jacobtylerwalls]

Maybe this means breaking out into two PartStaff objects when both are in one <part> just as we do for piano music.

[louisbigo]

we need to capture the pitches of the lines of a tab-staff, which may or may not (although it should) be in the same musicxml <part>

I think the tuning pitches will allways appear on the tab-staff (unless the file is uncorrect) - however the instrument information (guitar in that case) that might only appear in an other musicxml <part>.

Where to encode the actual pitches, I'm not sure. We could add attributes to TabClef or we could keep StringInstrument.stringPitches up to date with the pitches of the tab staff.

Both make sense to me. But I guess using TabClef is more safe as StringInstrument.stringPitches might not work correctly in case instrument / parts are not indicated properly (problem above). So I'll go for TabClef is everyone is ok.

handleStaffDetails() could call a new method to read the <staff-tuning> tag.

Good for me !

[jacobtylerwalls]

You might verify that using textStripValid() makes sense here. (See related uses). Helps guard against None/bad data, so that we don't try to create a pitch from "FNone" or something.

[louisbigo]

Ok. I can do the following, but maybe we would prefer to give up the whole inclusion of tuning in the case one of the pitch is not valid (in the solution below a tuning of (n-1) pitches will be validated if one pitch is incorrect, which seems unlikely useful). What do you think ?

        mxStaffTuning = mxDetails.findall('staff-tuning')
        if mxStaffTuning is not None:
            tuning_pitches = []
            for staff_tuning in mxStaffTuning:
                mxTuningStep = staff_tuning.find('tuning-step') 
                mxTuningOctave = staff_tuning.find('tuning-octave')
                if textStripValid(mxTuningStep) and textStripValid(mxTuningOctave):
                    tuning_step = staff_tuning.find('tuning-step').text
                    tuning_octave = int(staff_tuning.find('tuning-octave').text)
                    tuning_pitches.append(pitch.Pitch(tuning_step + str(tuning_octave)))

[jacobtylerwalls]

I checked and tuning-step and tuning-octave are both required on any staff-tuning, so if we lack one, I would either let the rest of the data through, or if I wanted to abort, I would raise MusicXMLImportException, since as a machine-coded input format we don't need to be that permissive about bad encodings. If we remove all the pitches and pass the file, then it will look like music21 didn't have a feature to translate it (and would be difficult for users to debug in Python).

[jacobtylerwalls]

You might also include the optional <tuning-alter> element.

[mscuthbert]

how are we progressing on this?

[mscuthbert]

Pinging @louisbigo -- wondering if you can address Jacob's comments.

[mscuthbert]

Hello -- just as a note (from the music21list Google Group) I'm taking a sabbatical from reviewing music21 issues and PRs until at least Jan 1, 2024, so this PR will be deferred until then.