Resetting playTime

[page200]

The following code resets playTime to 0 when the Format isn't 2.

midifile/src/MIDIFile.js

Lines 90 to 91 in 11fd971

	// reset playtime if format is 2
	playTime = 2 === format && playTime ? playTime : 0;

(That's good, because all tracks should play simultaneously if the Format isn't 2. Edit: The simultaneity of tracks isn't handled by resetting playTime to 0 (and this code isn't reached by Format 1 anyway, although it might get reached by corrupt Format 0 files that have several tracks in spite of Format 0). The simultaneity of tracks is handled by the smallestDelta loop below.)

However, the code comment says the opposite: "reset playtime if format is 2". That comment should be changed, I guess. Edit: I don't know whether the users of Format 2 want to restart the playTime for each track. The comment implies that they do, and in that case the code should be changed to match the comment, although maybe the users have gotten used to the current code behavior. The current code probably has no effect, it sets playTime to 0 only when it's already 0, if I'm not mistaken.

To make the code clearer in that regard, it should be replaced by something like

if (format == 2) {
  playTime = 0;
}

(What seems a bigger problem is that that code is currently inside a if (1 !== format || 1 === this.tracks.length) condition, so it would not execute when it is most important: when Format is 1 and there are multiple tracks. Maybe the current version hasn't been tested on such files, or am I missing something? Edit: Format 1 with multiple tracks is handled by the smallestDelta loop.)

While we're at it, can we rename playTime (and event.playTime) to something like time (and event.time)? "Play" is an appropriate word for Note On events, but most other events don't "play" in the normal sense. It's even confusing for Note Off events, because "playTime" seems like the duration for which the note has been playing.

Edit: TL;DR: The plan was probably to reset playTime to 0 at the start of each track of Format 2 files. I haven't checked whether that's common. In any case, this didn't happen due to a bug, and maybe the users have gotten used to that.

[mk-pmb]

Thanks for discovering this! Yeah, the original formula is really bad style. That is not a notation humans should have to parse.

With event.time, it could be confused with wall clock time of the event happening or being scheduled for. How about position, trackPosition or something like that?

[nfroidure]

Thanks for helping here, feel free to PR a fix, I'll merge asap.