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

Separate synth.py from midi.py #466

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Separate synth.py from midi.py #466

merged 5 commits into from
Feb 4, 2025

Conversation

bwhitman
Copy link
Collaborator

douglas gave us good feedback about midi.py having all the Synth stuff in it was a bit confusing, so here's a first pass at separating out these. docs updated as well.

@bwhitman bwhitman requested a review from dpwe January 31, 2025 01:43
Copy link
Collaborator

@dpwe dpwe left a comment

Choose a reason for hiding this comment

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

Looks great!

I think we should probably call the Synth class synth.PatchSynth() - it's not actually a base class for all synthesizers, although the voice rotation stuff probably should be a base class. What we currently call Synth is explicitly "a synthesizer constructed from voices that use the voices preset/stored patches mechanism".

I think in the future we'll have a VoicesSynth base-class that handles note-stealing, and PatchSynth will be a thin specialization of it, but that can come later.

I think the PCMSynth class that Douglas is kinda asking for will be a good future addition too.

Copy link
Collaborator

@dpwe dpwe left a comment

Choose a reason for hiding this comment

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

LGTM

@bwhitman bwhitman merged commit 1b0b1e9 into main Feb 4, 2025
1 check passed
@bwhitman bwhitman deleted the synthpy branch February 4, 2025 18:16
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