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.

Sign up for GitHub

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

Jump to bottom

JPverb for SuperNova fix #149

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

JPverb for SuperNova fix #149

wants to merge 2 commits into from

Conversation

capocasa
Copy link
Member

@capocasa capocasa commented Jun 18, 2017 •
edited
Loading

In my ongoing quest to find native SuperCollider equivalents to various sound processing that people are used to from everyday music listening, I found JPverb to be just what the doctor ordered. Lush reverb, complex and algorithmic, somewhat Alesis-ish but still unique. Unfortunately, supernova didn't recognize the compiled plugin library file .so.

I figured out that Faust-Generated UGens like JPverb use the string Faust for their symbols, and, without really taking the time to understand the details, I figured out you can just replace all instances of the String Faust with the plugin name and SuperNova will work just fine (along with a server flag):

# Replace symbol names
sed -i 's/Faust/PluginName/g' PluginName.cpp

# Add flag
echo "
+#ifdef SUPERNOVA 
+extern "C" FAUST_EXPORT int server_type(void) { return sc_server_supernova; } 
+#else 
+extern "C" FAUST_EXPORT int server_type(void) { return sc_server_scsynth; } 
+#endif
" >> PluginName.cpp

To modify any Faust-generated supercollider ugen to work with Supernova.

I took the liberty of applying this to JPverb manually. If there are no complaints, I would take this upstream to the Faust .CPP template for supercollider.

@LFSaw
Copy link
Member

LFSaw commented Jun 23, 2017

looking great, though I strongly object against manual editing of faust-generated cpp files.
Please talk to the FAUST guys at https://github.com/grame-cncm/faust (please mention this thread (and possibly me @LFSaw in the issue you raise there).

Thanks!

@LFSaw LFSaw self-requested a review June 23, 2017 21:52
@capocasa
Copy link
Member Author

Thanks for the feedback! OK I'll take it upstream.

@florian-grond
Copy link
Contributor

Curious to learn about the outcome of this, I just submitted the freshly compiled HOA UGens and can't wait to learn if they work for supernova.

@capocasa
Copy link
Member Author

capocasa commented Jun 27, 2017 •
edited
Loading

@florian-grond Great to know about your HOA port!

I'm guessing they won't, without using the patched Faust template I intend to create.

@florian-grond
Copy link
Contributor

florian-grond commented Jun 27, 2017 •
edited
Loading

Fantastic @carlocapocasa, the patched Faust template will be much appreciated.

@florian-grond
Copy link
Contributor

Hi @carlocapocasa I was curious if you had integrated the supernova patch to Faust already?

@capocasa
Copy link
Member Author

OK I narrowed down the fix with the latest version of the cpp file. Two things are required to make it work in SuperNova: (1) The SC_FAUST_PREFIX must be PluginName (in my case JPverbRaw) and I needed to manually add

#ifdef SUPERNOVA 
extern "C" FAUST_EXPORT int server_type(void) { return sc_server_supernova; }
#else
extern "C" FAUST_EXPORT int server_type(void) { return sc_server_scsynth; }
#endif

Couldn't figure out what to change in Faust yet to make that work.

@capocasa capocasa mentioned this pull request Jul 19, 2017
Merged
@capocasa
Copy link
Member Author

capocasa commented Jul 19, 2017 •
edited
Loading

OK I asked the Faust people to merge my pull request grame-cncm/faust#69 to include the supernova API version function.

I am less sure about what to do about SC_FAUST_PREFIX- they are using it for their faust2sc script, but it prefixes every possible plugin name with "Faust", so it is more an accident that the plugin loads with scsynth- supernova is correct to assume the plugin is called FaustJPverb.

I am wondering if the simplest solution would not be to make an exception with manual patching in this case- the faust people specifically request one do so when compiling without the faust2sc script.

@capocasa
Copy link
Member Author

@florian-grond This is not all that trivial- for a quick fix to immediately compile your plugin, refer to the minimal manual patching above.

@florian-grond
Copy link
Contributor

florian-grond commented Jul 19, 2017 via email

This was referenced Jul 19, 2017
Open
Open
@capocasa
Copy link
Member Author

Great news! Faust merged the patch.

As soon as we get the plugin to accept SC_FAUST_PREFIX="" from CMakeList.txt we may start compiling Faust plugins with SuperNova support with the latest master-dev Faust.

@telephon
Copy link
Member

great ! a good step forward

@capocasa
Copy link
Member Author

capocasa commented Jul 20, 2017 •
edited
Loading

Okay I've got it licked- The makefile was not setting the faust prefix for targets JPverb_supernova and Greyhole_supernova- just JPverb and Greyhole. Therefore, the Faust prefix was not disabled and it could not be loaded.

As a side effect, NDEBUG was not set for JPverb_supernova and Greyhole_superova either, which is why we were seeing some Faust debug output- this was not what the faust template intended and this pull request fixes this issue as well.

I modified the pull request to purely affect the CMakeLIsts.txt.

@capocasa
Copy link
Member Author

capocasa commented Jul 20, 2017 •
edited
Loading

I wonder why travis fails- builds fine for me.

Edit: fix needed to be conditional on SUPERNOVA flag. @scztt, thanks a lot for setting CI up.

@LFSaw
Copy link
Member

LFSaw commented Nov 5, 2017

What's the state of this? @carlocapocasa , sorry to follow up so late in the process, is this still a non-stale PR?

@capocasa
Copy link
Member Author

capocasa commented Nov 5, 2017

Yeah, this will merge and is necessary to build JPverb working on supernova as well as get rid of some error messages that look like Faust-plugins are noisy, but it's really the build system's fault.

@LFSaw
Copy link
Member

LFSaw commented Nov 5, 2017

ok, I'll have a look at it ASAP.

@capocasa
Copy link
Member Author

capocasa commented Nov 5, 2017

No worries! Thanks for looking into it!

@LFSaw
Copy link
Member

LFSaw commented Nov 7, 2017 •
edited
Loading

Do I see this right and I'd need to re-run faust1 (in its latest versions) to generate c++ code in order for your changes to take effect?

@florian-grond
Copy link
Contributor

florian-grond commented Nov 8, 2017 via email

@LFSaw
Copy link
Member

LFSaw commented Nov 8, 2017

I believe Carlos changes to make Faust fit for supernova did, in fact, add some lines to the C++ code, I plan to test it soon on the SC-HOA plugins.

I guess, you mean

did, in fact, add some lines to the C++ code generator

?

@capocasa
Copy link
Member Author

capocasa commented Nov 8, 2017

Pure conservatism- we know that the present code worked for lots of people, but merely think that the new code will work for lots of people.

In my estimation, a benefit to end users, such as improved performance, would justify the change, while a benefit to us developers, such as the satisfying application of principle, would not.

@LFSaw
Copy link
Member

LFSaw commented Nov 8, 2017 via email

@LFSaw
Copy link
Member

LFSaw commented Nov 8, 2017 via email

LFSaw added a commit to LFSaw/sc3-plugins that referenced this pull request Nov 16, 2017
@LFSaw
bump faust version to 0.12.0, includes fixes made by @carlocapocasa in …
5393b40 
…supercollider#149
@LFSaw LFSaw mentioned this pull request Nov 16, 2017
Open
@LFSaw
Copy link
Member

LFSaw commented Nov 16, 2017

@carlocapocasa, I updated the .cpp and header files in a separate PR, integrating your changes #172 . Please have a look and test on SuperNova.

@capocasa
Copy link
Member Author

Thanks, great stuff!

It will take a little while until I'll be able to context-switch my brain back to SC dev to test it properly, but I'll get to it as soon as I can.

@LFSaw
Copy link
Member

LFSaw commented Nov 16, 2017

others are welcome to test, too :)

@florian-grond
Copy link
Contributor

florian-grond commented Nov 16, 2017 via email

@florian-grond
Copy link
Contributor

As far as the noisy (i.e. debug messages) supernova Ugens are concerned, I just submitted a PR for the HOA Ugens where we fix this by adding to The CmakeList.txt:

if (NOT CMAKE_BUILD_TYPE MATCHES Debug)
add_definitions(-DNDEBUG)
endif()

now using cmake in release mode (-DCMAKE_BUILD_TYPE=Release) results in no debug noise no more for all Faust based Ugens

@patrickdupuis
Copy link
Contributor

@carlocapocasa @LFSaw I'm afraid that this and #172 will go stale. Are we really waiting for testing?

@LFSaw
Copy link
Member

LFSaw commented Jan 27, 2018

I don't get any debug messages here, also without the fix suggested by @florian-grond .

@patrickdupuis patrickdupuis mentioned this pull request Jan 30, 2018
Merged
@capocasa
Copy link
Member Author

OK so I've got a supernova setup again!

@LFSaw No dice! From checking the repo, the minimum faust version that contains my supernova patch is 2.5.9

I gave it a quick test with 2.15.11 (latest in arch linux) and that builds and works fine.

Would you like to do the rebuild for release or shall I?

@capocasa
Copy link
Member Author

capocasa commented Dec 3, 2019

Rebased the build file patch onto the current master and compiled JPverb and Greyhole from faust source using 2.5.11 according to readme instructions.

@TauPan TauPan mentioned this pull request Dec 20, 2020
Closed
@dyfer
Copy link
Member

dyfer commented Aug 6, 2021

Hello @capocasa @LFSaw
What's the status of this PR?
Also, we now have GitHub Actions in main, it would be great to rebase this so that the new CI runs.

@LFSaw
Copy link
Member

LFSaw commented Aug 6, 2021

I consider this branch stale. In fact, I have not worked on this for a long time and am currently not able to contribute any time to this issue. Sorry.

@LFSaw
Copy link
Member

LFSaw commented Aug 6, 2021

Looking a little bit deeper, I see that I did not file this PR, so it is not really my thing to tell about the staleness of this PR, sorry. Just saying that I neither know about the status of this, nor do I have the time looking into it atm. If someone decides to go along with this, please remember to add possible changes to the README (incl. how to compile the C sources from the faust patch files).

cheers

@capocasa
Copy link
Member Author

I upstreamed the fix to Faust so regenerating the DEIND .cpp files with any recent Faust version is enough, discarding this branch. I can do that.

@thresholdpeople thresholdpeople mentioned this pull request Jun 25, 2022
Open
@capital-G
Copy link
Contributor

Sorry @capocasa - is this branch still in dev or stale? :D If you want to get this merged it would be great to rebase it from main so we can have a CI run of this.

@capocasa
Copy link
Member Author

Aw, awesome! I understand, there are a lot of tickets in SuperCollider, thank you for helping to maintain it. I wanted to help too but ultimately couldn't...

Sure I'll rebase it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LFSaw LFSaw Awaiting requested review from LFSaw

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
comp: plugins fix proposed waiting for testing
Projects
SuperCollider development
Status: Waiting for review response
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@capocasa @LFSaw @florian-grond @telephon @patrickdupuis @dyfer @capital-G