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

Added rudimentary MIDI controller support to the standalone tracker #166

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

LeStahL
Copy link
Contributor

@LeStahL LeStahL commented Oct 11, 2024

This PR adds a menu with all connected controllers:

image

After selecting a controller, Note on and Note off are relayed to the player.

I figured I'd open a PR early to leave room for discussion on

  • CC messages? (parameter mapping per "midi learn" context menu? or per config file? or both?)
  • do we want to support play, pause, time code system common messages?
  • do we want to enable record from midi controllers? (this does not seem to work automatically by just adding a MIDI context)

That's my first PR with diff in go, please let me know if I used any anti-patterns.

@vsariola
Copy link
Owner

vsariola commented Oct 11, 2024

Sweet! Looks pretty good to me already, minor nits:

  1. The package layout attempts to follow the ideas here https://medium.com/sellerapp/golang-project-structuring-ben-johnson-way-2a11035f94bc that is, we group packages by dependencies. TL;DR: I'd put all the stuff that imports gomidi/midi/v2/drivers/rtmididrv into a subpackage called gomidi (e.g. at tracker/gomidi) and in the tracker package, just define something like
type MIDIContext interface {
   ListInputDevices(yield func(item MIDIDevice) bool) 
}
type MIDIDevice interface {
   Name() string
   Open() error
}

This way the ListInputDevices follows the new iterator support in golang https://bitfieldconsulting.com/posts/iterators, even if we are not yet using golang 1.23.

Then the gomidi subpackage could implement this interfaces using gomidi, implementing the PlayerProcessContext and the MIDIContext interfaces. This way we can easily mock the interfaces and make again something like a NullContext later on for testing and if we want to port the tracker to platforms that gomidi don't support (ok that's a stretch but the testing is a valid concern).

  1. You probably need some kind of mutex for the events slice; the events arrive from whatever goroutine the MIDI stuff gets processed from and the audio processing runs in its own thread. So there will be race conditions to modifying that events slice otherwise. Or just use a channel. Perhaps even just pumping NoteOnMsg or NoteOffMsg to the already existing channel that is for messages from Model -> Player.
  2. There's a couple of empty useless lines here in the commit.
  3. Recording should work already like you did, I think? The VST2 implements its PlayerProcessContext in its own way, but the recording does not know where the MIDI events arrive.
  4. CC messages could work somehow along the ideas here Support control changes #152. So we could "link" a parameter to a value from CC messages using a popup menu. And then in the VST version, this linking could be also to VST parameters. I'm wondering if CC messages also arrive through VST, does it then get complicated to have two ways of linking the parameters... anyway, I'd worry about this a bit later, once the work done for Support control changes #152 is in better shape.
  5. The play, pause messages could be pretty easy to handle. The only thing is that these would have to be communicated to the Model somehow safely, probably through a channel. What's in the timecode messages?

@LeStahL LeStahL force-pushed the feature/midi branch 2 times, most recently from 6731bf2 to 4a9517b Compare October 13, 2024 18:11
@LeStahL
Copy link
Contributor Author

LeStahL commented Oct 13, 2024

  1. Updated package layout.
  2. Added channel.
  3. Removed empty lines
  4. Record already works! :) Nice.
  5. Agree - left that out for now.
  6. Timecode messages contain the playback position of the song, MIDI sequencers use this to communicate the exact song position. I agree adding play and pause features would be easy, will open another PR for this however (because player does not yet have enough information to be able to control the tracker; so this will be a certain diff). When adressing those, we could also add the effect of playing MIDI notes that the currently selected note in the tracker gets replaced (like when entering values using the keyboard).

@vsariola vsariola merged commit 1aa91e6 into vsariola:master Oct 14, 2024
18 checks passed
@vsariola
Copy link
Owner

Merged, managed to mess up the commit message despite trying :D Well, next time! Great work!

I'll probably do a bit of refactoring but this should not change the fundamental concepts.

@LeStahL
Copy link
Contributor Author

LeStahL commented Oct 14, 2024

Thanks for the code review! :)

@vsariola
Copy link
Owner

vsariola commented Oct 14, 2024

I just had to use the iterators now that go has those; I even require now go 1.23 just for this ;) But now I can start refactoring other places too to use those, there's a few places where they would have been nice.

@vsariola
Copy link
Owner

Oh, I learned that the signature for the iterator functions was slightly different than what I suggested to you; sorry! My excuse is that I never actually used them before, because didn't have go 1.23 yet

@LeStahL
Copy link
Contributor Author

LeStahL commented Oct 14, 2024

don't worry, I took the invitation of this PR to learn a ton about the language and the design patterns coders use in go. :)

@vsariola
Copy link
Owner

vsariola commented Oct 14, 2024

For "style-education" (don't we all love that ;), here's what I've learned:

  1. The -er suffix is applied to single method interfaces e.g. type Reader interface { Read(...) } but as outlined here https://medium.com/@dotronglong/interface-naming-convention-in-golang-f53d9f471593 following this blindly can lead to quite stupid names. Anyway, interfaces with more methods don't generally follow this.
  2. Constructor like functions that take arguments and return a pointer to the constructed object generally are named NewXXX (this is not universally followed e.g. the os package has function Open to open a file)
  3. The "destructor" method is usually named Close() (the one that you use with defer obj.Close(). There's also the runtime.finalizer which gets ran later when the gc cleans up the object, but I have never needed those yet; defer obj.Close() is more deterministic.

@qm210 qm210 deleted the feature/midi branch October 19, 2024 12:48
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