-
Notifications
You must be signed in to change notification settings - Fork 151
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
Store lyrics per track #240
base: main
Are you sure you want to change the base?
Conversation
pretty_midi/pretty_midi.py
Outdated
@@ -219,14 +217,11 @@ def _load_metadata(self, midi_data): | |||
event.text, self.__tick_to_time[event.time])) | |||
|
|||
if lyrics: | |||
tracks_with_lyrics.append(lyrics) | |||
self.lyrics[track.name] = lyrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are track names guaranteed to be unique? OTOH I don't think they are so I'm not sure we should key on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I changed it to key on the index of the midi tracks, in consonance to how is done in other parts of the code.
@@ -186,8 +186,8 @@ def _load_metadata(self, midi_data): | |||
# signature changes, and lyrics | |||
self.key_signature_changes = [] | |||
self.time_signature_changes = [] | |||
self.lyrics = [] | |||
self.text_events = [] | |||
self.lyrics = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to keep parallel lists of lyrics and tests events in both the PrettyMIDI objects and the Instrument objects it contains. In other words, the PrettyMIDI object should not have a lyrics or text_events attribute. These should only be populated when loading from the MIDI file, and then they should be passed on to the Instrument, and the PrettyMIDI object should forget about them. In any PrettyMIDI class method that accesses lyrics or text events, the lyrics and text events of the constitutuent Instruments should be grabbed instead. The logic for populating the lyrics should then go in the __load_instruments method.
@@ -461,7 +463,7 @@ def get_end_time(self): | |||
""" | |||
# Get end times from all instruments, and times of all meta-events | |||
meta_events = [self.time_signature_changes, self.key_signature_changes, | |||
self.lyrics, self.text_events] | |||
*self.lyrics.values(), *self.text_events.values()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instrument.get_end_time should be modified to take into account lyrics and text events and lyrics/text events should be removed from this list.
adjust_meta(self.text_events) | ||
for lyrics_key in self.lyrics: | ||
adjusted_lyrics = adjust_meta(self.lyrics[lyrics_key]) | ||
self.lyrics[lyrics_key] = adjusted_lyrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done to each Instrument's lyrics, not self.lyrics
This pull request solves #214.
Now each instrument has text and lyrics list attributes, that can be populated with MetaMessages for lyrics or text annotations