grimmdude / MidiWriterJS

♬ A JavaScript library which provides an API for programmatically generating and creating expressive multi-track MIDI files and JSON.
MIT License
547 stars 58 forks source link

`StaveNote.wait` is undefined.. related to `trackFromVoice` #73

Closed artsyca closed 3 years ago

artsyca commented 3 years ago

I'm not very well versed in this package but I believe there's a situation where wait is undefined whereas it should be zero.

This is causing an error in some cases when calling get TickDuration there is an error

image

In the NoteOnEvent function, there is a call to Object.assign with some default values, including wait: 0

...
// Set default fields
fields = Object.assign({
    channel: 1,
    startTick: null,
    velocity: 50,
    wait: 0
}, fields)
...

However, in trackFromVoice the value of wait is returned as undefined for most notes of noteType==='n', because in the first if clause wait is not set for the first loop...

...
if (tickable.noteType === 'n') {
    tickable.keys.forEach(function (key) {
    // build array of pitches
    pitches.push(_this.convertPitch(key));
    });
} else if (tickable.noteType === 'r') {
    // move on to the next tickable and use this rest as a `wait` property for the next event
    wait = _this.convertDuration(tickable);
    return;
}

track.addEvent(new NoteEvent({
    pitch: pitches,
    duration: _this.convertDuration(tickable),
    wait: wait
})); 

// reset wait
wait = 0;
...

This results in the defaults not actually being applied because the key is already present and Object.assign doesn`t overwrite it.

Object.assign({wait: 0}, {wait: undefined})
// {wait: undefined}

The value of wait should either be set at the top, or omitted from the returned object, or maybe the addEvent should be defaulted to zero e.g:

track.addEvent(new NoteEvent({
    pitch: pitches,
    duration: _this.convertDuration(tickable),
    wait: wait || 0 // Set to 0 by default or
})); 
artsyca commented 3 years ago

OK I see this is related to the whole trackFromVoice discussion.. I'm not 100% sure if this covers all the cases, but this is an implementation which gets a bit closer to the target I believe

/**
 * Support for converting VexFlow voice into MidiWriterJS track
 * @return MidiWritier.Track object
 */
trackFromVoice(voice) {
  var track = new Track();
  var wait = [];
  var pitches = [];

  voice.tickables.forEach(tickable => {
    pitches = [];

    if (tickable.noteType === 'n') {
      tickable.keys.forEach(key => {
        // build array of pitches
        pitches.push(this.convertPitch(key));
      });
      track.addEvent(new NoteEvent({pitch: pitches, duration: this.convertDuration(tickable), wait: wait}));
      wait = [];

    } else if (tickable.noteType === 'r') {
      // move on to the next tickable and use this rest as a `wait` property for the next event
      wait.push(this.convertDuration(tickable));
      return;
    }
  });

  // Rests at the end of the track, add a ghost note..
  if(wait.length > 0) {
    track.addEvent(new NoteEvent({pitch: '[C4]', duration: '0', wait, velocity: '0'}));
  }

  return track;
}
grimmdude commented 3 years ago

Hey @artsyca,

Thanks for documenting this! I will more closely look at this as soon as I can. PRs always welcome!

-Garrett