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

Implement an internal CLI for loading soundfonts and setting engine parameters. #973

Merged
merged 9 commits into from
Sep 21, 2021

Conversation

jofemodo
Copy link
Contributor

BTW, add a new command line argument (--num_voices).

}


std::vector<std::string> string_tokenize(std::string str) {
Copy link
Member

Choose a reason for hiding this comment

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

str could be const ref here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix ...

try {
synth.setOversamplingFactor(stoi(args));
} catch (...) {
std::cout << "ERROR: Can't set oversampling!\n";
Copy link
Member

Choose a reason for hiding this comment

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

None of these throw, however they're not thread-safe so we need a mutex in the callback. You can check the SfizzVstProcessor.cpp file for an example, especially the doBackgroundWork() and process() functions. Basically you need to fully lock in this cli_thread_proc() method, and use try_lock() in the real-time thread (and exit if you can't take the lock).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I've been testing and the "random breaks" when loading soundfonts seems to be fixed.
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

You may still have breaks for big soundfonts. If you want to avoid them you could somehow fade out the current loaded file before loading a new one in, but none of our plugins does this currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a little bit what you mean for "fade out" the "currently loaded soundfont"? ;-)
Do you mean ending active notes before loading the new soundfont?

@paulfd
Copy link
Member

paulfd commented Sep 10, 2021

I made a change and did some formatting but I cannot push to your branch. Would you mind allowing it? Otherwise I'll do another PR from my fork.

@jofemodo
Copy link
Contributor Author

You should allowed to write in my fork now.

Thanks!

@jofemodo
Copy link
Contributor Author

Hi @paulfd !

Is there some problem with merging this? Can i do something? We are preparing a new stable version and we would like to install sfizz from your deb packages, so we can keep updated easily.

Thanks!

@jofemodo
Copy link
Contributor Author

BTW, I sent you and invitation to write on my fork but it's pending yet.

@paulfd
Copy link
Member

paulfd commented Sep 21, 2021

Hi ! Sorry I'm too taken in RL things, and I missed the invitation which is now expired. You can resend it, but I think there's a checkbox in the right column of the pull request interface that says "Allow edits by maintainers" which would allow me to write on your branch without having write access on the whole repository. Then I can push my changes and if you're OK with them we can merge :)

@jofemodo
Copy link
Contributor Author

jofemodo commented Sep 21, 2021

Hi @paulfd !

I didn't find this check anywhere. Perhaps it's only available when creating the PR?
Anyway, i resend the invitation ;-)

@paulfd
Copy link
Member

paulfd commented Sep 21, 2021

We can let the CI run although there are no "tests" of the program. In the meantime maybe check my changes ! Once the command-line interface is stable on your side maybe it'd be good to write down some tests so that we don't accidentally regress.

Copy link
Contributor Author

@jofemodo jofemodo left a comment

Choose a reason for hiding this comment

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

Everything looks OK now ;-)
I will write some tests ASAP.

@jofemodo
Copy link
Contributor Author

BTW, are the tests required for merging this PR?

@paulfd
Copy link
Member

paulfd commented Sep 21, 2021

No no

@paulfd paulfd merged commit 55ac56e into sfztools:develop Sep 21, 2021
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