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

Rework Note field order and pretty printing #95

Merged
merged 6 commits into from
May 22, 2018
Merged

Rework Note field order and pretty printing #95

merged 6 commits into from
May 22, 2018

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented May 20, 2018

This PR changes the Note struct to

mutable struct Note <: AbstractNote
    pitch::UInt8
    velocity::UInt8
    position::UInt
    duration::UInt
    channel::UInt8

and gives default value 0 to channel. It also implements very nice pretty printing for note and Notes. Here is how it looks like now:

midi = readMIDIfile("doxy.mid")
notes = getnotes(midi.tracks[4])
533 Notes with tpq=960
 Note F5   | vel = 69  | pos = 7427, dur = 181
 Note A♯5  | vel = 85  | pos = 7760, dur = 450
 Note D6   | vel = 91  | pos = 8319, dur = 356
 Note D5   | vel = 88  | pos = 8323, dur = 314
 Note G♯4  | vel = 88  | pos = 8327, dur = 358
 Note A♯5  | vel = 76  | pos = 8694, dur = 575
 Note G5   | vel = 66  | pos = 9281, dur = 273
 Note A♯5  | vel = 94  | pos = 9594, dur = 666
 Note F♯4  | vel = 98  | pos = 10189, dur = 307
 Note C5   | vel = 87  | pos = 10206, dur = 285
  ⋮

if channel is not 0, it is also printed at the end

However tests fail, in the specific programchange test. I really don't know how to fix it and therefore @JoelHobson I would appreciate your help on how to fix this programchange. I think what broke it is that I changed the field order...?

closes #93

Datseris added 4 commits May 14, 2018 17:01
Also added pretty printing that prints note signature :)
and also improved printing
@Datseris Datseris requested a review from JoelHobson May 20, 2018 10:15
@JoelHobson
Copy link
Collaborator

@Datseris I think you're right about the field order change breaking it. I looked at the test, and it's very poorly written. I suspect what I did when writing the test was write the track, then manually inspect it to make sure it made sense, and used the results of that write to create the test. That's extremely brittle, and not a good approach for testing. A better way might've been to write the track, then read what was written and make sure the data structure matched the original. I don't think there's a good way around this other than re-writing the test.

As an aside, I haven't actually written anything in Julia since I released this library. I'm happy to give pull requests a quick look over, but my opinion on a review shouldn't count for too much. You've contributed a lot more to the project than I have since the initial release, and as far as I'm concerned it's your project now since you're actively using it and contributing. If you want to make larger changes or change ownership of the project to you, that's fine with me.

@Datseris
Copy link
Member Author

Hey @JoelHobson thanks for letting me know.

No reason to pass me ownership at the moment. However, we are developing https://github.com/Datseris/MusicManipulations.jl at the moment, which has a lot of content for doing stuff with music data.

I was thinking to propose to you to add`MIDI.jl to a new GitHub organization called JuliaMusic. Of course this would also imply at least sharing ownership. You can decide this if you want!

Back to the test, I will be very honest and say that I do not know how to fix programchange because I have no idea what it does... At the moment I do not know whether the function behaves as expected and its just that I have to re-phrase the tests, or that I simply completely broke programchange.

If you have any ideas let me know. If not, I think because this function is really for very rare occassions I can put a warning message to make sure if someone ever uses it then they should first test it.

@JoelHobson
Copy link
Collaborator

I have no problem with adding this a new organization if you want to do that.

As for the test, it's been so long I'm not entirely sure what it does either 😄

I wouldn't object very strongly to removing the test entirely, but I think a better way to write the test would be to create a track with some program change events (these are just midi events used to change the preset on a piece of hardware, like a synth), then write that track to a file. Read the file in again, and make sure the program change events still appear in the new track at the same places.

@Datseris
Copy link
Member Author

I added a warning message in programchange for now.

@Datseris Datseris merged commit b0abc55 into master May 22, 2018
@Datseris Datseris deleted the pitch branch May 22, 2018 11: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.

Suggestion: replace note.value with note.pitch
2 participants