Skip to content
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

Notes, BPM, ms_per_tick #82

Merged
merged 9 commits into from
Dec 5, 2017
Merged

Notes, BPM, ms_per_tick #82

merged 9 commits into from
Dec 5, 2017

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented Dec 4, 2017

I have added an alias Notes = Vector{Note}. It feels very natural for me plus I was using it all the time for research. I think it is nice, plus it is only something optional.

I have also added two functions: BPM(midi) and ms_per_tick(midi) which were super important for me, and I think fit perfectly on this package.

Soon I will upload a new package about manipulating music (using MIDI). I had these 2 there but I feel they fit much more here.

Please let me know if you agree, and if yes go ahead and merge!

@Datseris Datseris requested a review from JoelHobson December 4, 2017 22:55
using Notes() calls Note[]
I removed `MIDI.` from all exported names and added `using MIDI` at the start of the example. This is more natural, less verbose, and more in line with how other packages operate.
@Datseris
Copy link
Member Author

Datseris commented Dec 4, 2017

@JoelHobson okay that is everything. I improved significantly the README as well, I think.

function BPM(t::MIDI.MIDIFile)
# META-event list:
tlist = [x for x in t.tracks[1].events]
tttttt = Vector{UInt32}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment explaining the reasoning behind tttttt? I see your comment below referring to it as tt-tt-tt, but I still don't quite follow what it means.

@JoelHobson
Copy link
Collaborator

Those sound like excellent utility functions to include. I'm happy to merge your changes, I just have one nitpicking comment.

I'm also really happy that you're still using this library and contributing to it!

@JoelHobson JoelHobson self-assigned this Dec 5, 2017
@Datseris
Copy link
Member Author

Datseris commented Dec 5, 2017

@JoelHobson I added an explanation.
HOwever I cannot explain the lines:

  # Get the microsecond number from tttttt
  unshift!(tttttt , 0x00)
  u = ntoh(reinterpret(UInt32, tttttt)[1])

because I do not know what they do, you do. You told me to use them over one and a half years ago when I started using this library!

@JoelHobson
Copy link
Collaborator

@Datseris Hahaha, no worries, I just meant an explanation of what 'tttttt' represented. Your explanation is fine. I'll merge this in. Thanks again!

@JoelHobson JoelHobson merged commit e5a7850 into master Dec 5, 2017
@Datseris Datseris deleted the Notes branch December 5, 2017 22:06
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