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 tempo support #172

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rhelsing
Copy link

@rhelsing rhelsing commented Dec 1, 2022

In reference to #158

@psobot Could you let me know if I'm going in the right direction with this? My c++ knowledge is rusty.

pedalboard/pedalboard.py Outdated Show resolved Hide resolved
pedalboard/ExternalPlugin.h Outdated Show resolved Hide resolved
pedalboard/ExternalPlugin.h Outdated Show resolved Hide resolved
@rhelsing
Copy link
Author

@psobot Trying to make a bit more progress on this. The method I found in Juce seems to be setBpm and getBpm.

@rhelsing
Copy link
Author

@psobot When you have time, I'd love to hear your feedback on this

Copy link
Member

@psobot psobot left a comment

Choose a reason for hiding this comment

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

Looks like this is moving in the right direction! I don't know that this will compile, though - getBpm and setBpm aren't defined on ExternalPlugin, and the ExternalPlugin constructor doesn't take the new tempoBpm argument yet.

Comment on lines +568 to +570
def __set_extra_functions__(self, tempo_bpm: int = 120):
tempo_bpm = self.tempo_bpm

Copy link
Member

Choose a reason for hiding this comment

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

This won't do anything: tempo_bpm is just a local variable. I think you probably want to pass tempo_bpm through as a parameter into _VST3Plugin.__init__(self, path_to_plugin_file, plugin_name) on line 682 instead.

pedalboard/ExternalPlugin.h Outdated Show resolved Hide resolved
@@ -994,9 +995,10 @@ inline void init_external_plugins(py::module &m) {
"(i.e.: if you're running Linux but trying to open a VST that does not "
"support Linux, this will fail).")
.def(py::init([](std::string &pathToPluginFile,
std::optional<std::string> pluginName) {
std::optional<std::string> pluginName,
std::optional<int> tempoBpm) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need to decide if tempoBpm can be optional or not; in your Python code below, you pass a default value of 120 BPM. Making tempoBpm an std::optional here means that it could be None as well; it sounds like it should be mandatory argument with a default value, rather than an optional type.

@tekkno-primitiv
Copy link

Please add this functionality to the master branch of Pedalboard! We can't use delays, reverbs as well as some synths which have synchronization with host tempo.

@RoyalCities
Copy link

In reference to #158

@psobot Could you let me know if I'm going in the right direction with this? My c++ knowledge is rusty.

Just wanted to throw in my support for this. If we could somehow set a global bpm variable it would push this into overdrive and open up ALOT of options given so many vst makers default to vst host sync and don't offer any manual control

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.

4 participants