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

Add midi output support #12

Merged
merged 9 commits into from
Aug 8, 2022
Merged

Add midi output support #12

merged 9 commits into from
Aug 8, 2022

Conversation

grace125
Copy link
Contributor

@grace125 grace125 commented Aug 4, 2022

Objective

  • Create a plugin for sending midi events
  • Add an example to showcase the new feature

@grace125 grace125 marked this pull request as ready for review August 6, 2022 00:53
@BlackPhlox
Copy link
Owner

Amazing! 🤩 I got time to look at this on Sunday, can you run the cargo fmt command and commit? 😊

@grace125
Copy link
Contributor Author

grace125 commented Aug 6, 2022

Done!

I'm also thinking about submitting a PR to change how MidiInputPlugin works to more closely match MidiOutputPlugin (e.g. provide a more general way to connect to input ports). Let me know if that'd be welcome.

@BlackPhlox
Copy link
Owner

Sure, I'll look at it in detail Sunday. I think a group plugin would also be nice, grouping both input and output called Midi (thus only needing users to change from .add_plugin(Midi) to .add_plugins(Midi)). Though that might require some to change to both plugins in terms of the connecting process. I would like to hear your opinion on this 😊

@grace125
Copy link
Contributor Author

grace125 commented Aug 6, 2022

I think a group plugin would make sense, but it should probably be named MidiPlugins just to follow the convention of other plugins. Plus, naming it Midi might encourage users updating their code to add MidiOutputPlugin when they don't need it.

Copy link
Owner

@BlackPhlox BlackPhlox left a comment

Choose a reason for hiding this comment

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

Overall this is some really amazing work and a huge improvement of the ergonimics! Though I think we can improve it even further. Though time is always a factor. I think we should move this pr into a new branch as part of an overhaul instead and then when we think we are done, then it merged in, just like with Leafwing-Studios/leafwing-input-manager#164. What do you think?

grace125 and others added 2 commits August 7, 2022 11:58
Co-authored-by: Mikkel Rasmussen <[email protected]>
@grace125
Copy link
Contributor Author

grace125 commented Aug 7, 2022

I'm down for moving this pr to a branch and working on it there.

Though I just realised that maybe MidiOutput should be a non send resource. I don't really like how I did error handling. Let me try it out first.

@BlackPhlox
Copy link
Owner

Amazing! I've added the branch 0.6 so that you can now change the target branch of this pr :)

@BlackPhlox BlackPhlox merged commit 7f8b16d into BlackPhlox:0.6 Aug 8, 2022
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