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.

Sign up for GitHub

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

Jump to bottom

Handling Staff-tuning in MusicXML (fixes #778) #1169

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Handling Staff-tuning in MusicXML (fixes #778) #1169

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
780a425
solves #778
lbigo Nov 18, 2021
ddc10e3
remove unused variable
lbigo Nov 18, 2021
41adc5e
line too long
lbigo Nov 18, 2021
68d1213
Merge branch 'master' into pr/1169
mscuthbert Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions music21/musicxml/m21ToXml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6062,6 +6062,16 @@ def staffLayoutToXmlStaffDetails(self, staffLayout):
mxStaffLines.text = str(staffLayout.staffLines)

# TODO: staff-tuning
louisbigo marked this conversation as resolved.
Show resolved Hide resolved
if hasattr(staffLayout, 'fretboard'):
tuning_pitches = staffLayout.fretboard.tuning
for i in range(len(tuning_pitches)):
louisbigo marked this conversation as resolved.
Show resolved Hide resolved
mxStaffTuning = SubElement(mxStaffDetails, 'staff-tuning')
mxStaffTuning.set('line', str(i + 1))
mxTuningStep = SubElement(mxStaffTuning, 'tuning-step')
mxTuningStep.text = tuning_pitches[i].name
mxTuningOctave = SubElement(mxStaffTuning, 'tuning-octave')
mxTuningOctave.text = str(tuning_pitches[i].octave)
louisbigo marked this conversation as resolved.
Show resolved Hide resolved

# TODO: capo
# TODO: staff-size
return mxStaffDetails
Expand Down
12 changes: 12 additions & 0 deletions music21/musicxml/xmlToM21.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5691,7 +5691,19 @@ def xmlStaffLayoutFromStaffDetails(
stl.staffType = stream.enums.StaffType(xmlText)
except ValueError:
environLocal.warn(f'Got an incorrect staff-type in details: {mxStaffType}')

# TODO: staff-tuning*
mxStaffTuning = mxDetails.findall('staff-tuning')
if mxStaffTuning is not None:
tuning_pitches = []
for i in range(len(mxStaffTuning)):
louisbigo marked this conversation as resolved.
Show resolved Hide resolved
staff_tuning = mxStaffTuning[i]
tuning_step = staff_tuning.find('tuning-step').text
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@louisbigo louisbigo Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tuning_octave = int(staff_tuning.find('tuning-octave').text)
tuning_pitches.append(pitch.Pitch(tuning_step + str(tuning_octave)))
fretboard = tablature.FretBoard.getFretBoardFromTuning(tuning_pitches)
setattr(stl, 'fretboard', fretboard)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jacobtylerwalls jacobtylerwalls Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

freatboard

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>:

image

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !


# TODO: capo
# TODO: show-frets
# TODO: print-spacing
Expand Down
19 changes: 19 additions & 0 deletions music21/tablature.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ def getPitches(self):

return pitchList

@staticmethod
louisbigo marked this conversation as resolved.
Show resolved Hide resolved
def getFretBoardFromTuning(pitches):
standard_fret_boards = [GuitarFretBoard, UkeleleFretBoard,
BassGuitarFretBoard, MandolinFretBoard]
for standard_fret_board in standard_fret_boards:
if pitches == standard_fret_board().tuning:
return standard_fret_board()
return CustomFretBoard(pitches)

class FirstFret:
'''
Expand Down Expand Up @@ -344,6 +352,17 @@ def __init__(self, fretNotes=None, displayFrets=4):
super().__init__(numStrings, fretNotes, displayFrets)

self.tuning = [pitch.Pitch('G3'), pitch.Pitch('D4'), pitch.Pitch('A4'), pitch.Pitch('E5')]

class CustomFretBoard(FretBoard):
'''
A custom fretboard tuned with any list of pitches
'''

def __init__(self, pitches, fretNotes=None, displayFrets=4):
numStrings = len(pitches)
super().__init__(numStrings, fretNotes, displayFrets)

self.tuning = pitches.copy()
louisbigo marked this conversation as resolved.
Show resolved Hide resolved
# ------------------------------------------------------------------------------


Expand Down