Skip to content

Conversation

@migperfer
Copy link

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


if lyrics:
tracks_with_lyrics.append(lyrics)
self.lyrics[track.name] = lyrics
Copy link
Owner

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.

Copy link
Author

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.

self.time_signature_changes = []
self.lyrics = []
self.text_events = []
self.lyrics = {}
Copy link
Owner

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.

# 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()]
Copy link
Owner

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
Copy link
Owner

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

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.

2 participants