AlsaMIDIDevice and CoreMIDIDevice: bug fixes and improvements - #18
Conversation
|
There are news that some functions used in the CoreMidi backend are deprecated so unfortunately like always this PR is gonna get out of scope |
2b2ffe0 to
a972d36
Compare
|
https://datahacker.blog/files/86/MIDI-20-Specifications/96/UMP-and-MIDI-20-Protocol-Specification.pdf |
76cf1cc to
246e1da
Compare
charlesthobe
changed the title
f2570ba to
a812344
Compare
c168863 to
be3eeed
Compare
|
Seems like this PR now does a fair bit more than its title and description would imply (correct me if I'm wrong)? Would kinda prefer multiple PRs over a singular one fixing multiple things but would also be happy for the title and description to just be updated. Without that context it's kinda hard to review |
|
I'd like to add on to Guta's comment above that pushing multiple times after the pr is marked ready for review can make things harder to review, too. Occasional pushes (or when a change is requested) are a-okay, but many pushes suggest that the pr is still in heavy development, meaning it's not ready for review. It's not at all a big deal, but it is something to keep in mind for the future. |
charlesthobe
changed the title
… struct and change the naming convention of its members to be more fitting
… next to data types
This also makes sure the constructor for "time_until_pulled_ev" is the correct value via promotion of the operands in the constructor because even in cases where all values of the operands were correct any intermediary result or the final result could overflow/underflow.
|
idk what the problem with github CI is, it's not even trying to build... |
|
Looks like actions are just kinda very down right now. I've never seen an incident log this long before... 
Edit: Looks like it's been resolved: https://www.githubstatus.com/incidents/qcvjkzcs7j74 |
the-phinet
left a comment
the-phinet
left a comment
There was a problem hiding this comment.
This all looks good to me. I sincerely apologize for how long it took for any of us to look at this
Sidenote: I really appreciate your attention to detail and code quality
|
No worries and thank you |
now it's set in milliseconds while avoiding runtime convertion
3 participants
Changes:
CoreMidi has been moving toward MIDI 2.0 and using Universal Midi Packets (UMPs) for MIDI 1.0 and 2.0 to the point that they marked functions that deal with raw MIDI 1.0 data as deprecated.
with this PR the code packages raw midi data into UMPs and uses the newer non-deprecated functions.
Event tick position calculation was not accurate in AlsaMIDIDevice due to sign conversion and too small of a data type to ensure the value of thread::sleep timer is correct, this actually breaks playback of freedoom phase 2 0.13.0 map10 after 5 minutes, this has been fixed, Also a theoretical similar issue that could occur in CoreMIDIDevice has been mitigated.
Rest of the changes are less important or just coding style cleanups