-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conditional audio flags #136
Conversation
@@ -5,6 +5,7 @@ set(CMAKE_CXX_STANDARD 17) | |||
set(CMAKE_POSITION_INDEPENDENT_CODE ON) | |||
|
|||
option(SST_JUCEGUI_BUILD_EXAMPLES "Add targets for building and running sst-filters examples" FALSE) | |||
option(SST_JUCEGUI_SKIP_AUDIO "Skip JUCE audio definitions" TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the default be false to retain old behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but since the examples build I think it is fine as is. Let's not insert them unless upstream wants them by default. then no surge change required even! if you agree let me know and we can merge this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im a subscriber to less is more
esp because this PR is bc macros were redefined, and because this library will be a common upstream dependency, less macro definitions will prevent future issues?
forward progress! i say merge as is but i wouldn't die on this hill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, most of the upstream ones are in the clap juce shim branch anyway. I think it is just vestigial. So yeah leave it as is.
@@ -26,7 +27,7 @@ if (${SST_JUCEGUI_BUILD_EXAMPLES}) | |||
|
|||
if (NOT TARGET juce::juce_gui_basics) | |||
if (NOT DEFINED SST_JUCEGUI_JUCE_VERSION) | |||
set(SST_JUCEGUI_JUCE_VERSION 7.0.12) | |||
set(SST_JUCEGUI_JUCE_VERSION 8.0.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before merge i want to point out that i have no idea if this upgrade will break any downstream dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't. We also CI test with 7 and 8 so this is just a default.
So merged away! If you want to pull the surge side we can now test CI then merge. Then I'd say next step is replace the fx knob class with |
SST_JUCEGUI_SKIP_AUDIO
which can be override downstream to add or to skip the audio flags.baconpaul said in discord:
I think he meant
cus - y'know -
"skip audio" mean "don't add audio flags"
NOT "skip audio" means "add flags"
horray for confusing double negatives
but opening it up to discussion.