search

grimmdude / MidiWriterJS

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

fixed the handling of startTick = 0 #48

Closed jpriebe closed 5 years ago

jpriebe commented 5 years ago

Congrats on 1.7.0! I found a little bug with notes where startTick = 0. When they are created, the startTick is set to null, so they are treated by the track as being sequential instead of explicit.

grimmdude commented 5 years ago

Hey thanks @jpriebe!

I'm thinking it might be clearer to change the way we set these default values overall. Maybe could use Object.assign():

// Set defaults
fields = Object.assign({
    repeat: 1,
    sequential: false,
    startTick: null,
    velocity: 50,
    wait: 0,
}, fields);

What do you think?

jpriebe commented 5 years ago

That sounds like a good idea.

I was thinking you might also run into similar problems with things like velocity=0, where the current code would see velocity as unspecified and default it to 50. You might ask why would somebody ever send velocity=0, but I think that if you were generating the notes programmatically, you might have an algorithm that gradually reduces the velocity, and it takes it all the way to zero. You wouldn't want that last note to jump back to velocity=50.

So I think your proposed solution would set reasonable defaults, but still allow for zero-value properties.

jpriebe commented 5 years ago

Any chance you can get a 1.7.1 official release into npm?

grimmdude commented 5 years ago

Hey Jason,

I think 1.7.1 is currently the latest version available in npm, no? https://www.npmjs.com/package/midi-writer-js

-Garrett

jpriebe commented 5 years ago

You're right. I set up my project on a new machine, and I assumed that when I ran npm install, I would get the latest version of all the libraries, including midi-writer-js. But my package-lock.json had me stuck on 1.6.0. All is good. Thanks!