sc3-plugins icon indicating copy to clipboard operation
sc3-plugins copied to clipboard

JPverb for SuperNova fix

Open capocasa opened this issue 7 years ago • 41 comments

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.

capocasa avatar Jun 18 '17 14:06 capocasa

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 avatar Jun 23 '17 21:06 LFSaw

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

capocasa avatar Jun 25 '17 15:06 capocasa

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.

florian-grond avatar Jun 27 '17 01:06 florian-grond

@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.

capocasa avatar Jun 27 '17 08:06 capocasa

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

florian-grond avatar Jun 27 '17 13:06 florian-grond

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

florian-grond avatar Jul 13 '17 21:07 florian-grond

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 avatar Jul 18 '17 22:07 capocasa

OK I asked the Faust people to merge my pull request https://github.com/grame-cncm/faust/pull/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 avatar Jul 19 '17 00:07 capocasa

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

capocasa avatar Jul 19 '17 00:07 capocasa

Will check when back, thanks!

On Jul 18, 2017 8:28 PM, "Carlo Capocasa" [email protected] wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/supercollider/sc3-plugins/pull/149#issuecomment-316237110, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVJxmWsIcjQGCejw1t0jHgkKqQ8p7jxks5sPU2vgaJpZM4N9gQg .

florian-grond avatar Jul 19 '17 10:07 florian-grond

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.

capocasa avatar Jul 19 '17 13:07 capocasa

great ! a good step forward

telephon avatar Jul 20 '17 07:07 telephon

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 avatar Jul 20 '17 08:07 capocasa

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.

capocasa avatar Jul 20 '17 08:07 capocasa

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

LFSaw avatar Nov 05 '17 18:11 LFSaw

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.

capocasa avatar Nov 05 '17 18:11 capocasa

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

LFSaw avatar Nov 05 '17 18:11 LFSaw

No worries! Thanks for looking into it!

capocasa avatar Nov 05 '17 20:11 capocasa

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?

LFSaw avatar Nov 07 '17 19:11 LFSaw

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.

www.grond.at

On Tue, Nov 7, 2017 at 2:15 PM, LFSaw [email protected] wrote:

Do I see this right and I'd need to re-run faust to generate c++ code in order for the changes to take effect?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/supercollider/sc3-plugins/pull/149#issuecomment-342590688, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVJxitE6jtkYgbCS68a3ayeB-9c0aqiks5s0KxMgaJpZM4N9gQg .

florian-grond avatar Nov 08 '17 15:11 florian-grond

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

?

LFSaw avatar Nov 08 '17 16:11 LFSaw

Yes, that is correct- a recompile is required.

However, in order to avoid possible regressions, I would suggest to make an exception and manually edit the cpp file instead, unless there is something compelling Faust has improved in the mean time.

Adding this to the bottom:

#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

If you prefer to recompile, I would suggest to check out the latest stable Faust release branch and cherry pick my commit b4e4dab0 before building it. Otherwise, you would have to build from Faust master-dev. The Faust build process itself is quite painless in any case.

capocasa avatar Nov 08 '17 17:11 capocasa

curious: why do you think it is not a good idea to use the latest stable that includes your commit?

On 08. Nov 2017, at 18:26, Carlo Capocasa [email protected] wrote:

Yes, that is correct- a recompile is required.

However, I would suggest to make an exception and manually edit the cpp file and 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

in order to avoid possible regressions, unless there is something compelling Faust has improved since the present compilation.

If you prefer to recompile, I would suggest to check out the latest stable Faust release branch and cherry pick my commit b4e4dab0 before building it. Otherwise, you would have to build from Faust master-dev. The Faust build process itself is quite painless in any case.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

LFSaw avatar Nov 08 '17 18:11 LFSaw

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.

capocasa avatar Nov 08 '17 18:11 capocasa

my idea behind this is:

I'd like to provide material that is easily (and documented'ly (?)) to be repeated by anyone else but me, in case there are things coming up that e.g. make a change in the faust-code necessary (this is what triggered my last update). That's my reason why I don't want to mess with generated code...

In case you'd like to fumble around with it, I'm happy to get an update pf this PR that consists of

I'd find it important that both c++ sources (for Greyhole, JPVerb) are built with the same faust version. If you don't, I'll try to find time to do this (hopefully soon) myself.

On 08. Nov 2017, at 19:49, Carlo Capocasa [email protected] wrote:

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.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

LFSaw avatar Nov 08 '17 19:11 LFSaw

I was kinda sure that the README contains the git-sha of the faust version used but this is apparently not the case. For the future, I think this is a good idea, though.

On 08. Nov 2017, at 20:31, Till Bovermann [email protected] wrote:

my idea behind this is:

I'd like to provide material that is easily (and documented'ly (?)) to be repeated by anyone else but me, in case there are things coming up that e.g. make a change in the faust-code necessary (this is what triggered my last update). That's my reason why I don't want to mess with generated code...

In case you'd like to fumble around with it, I'm happy to get an update pf this PR that consists of

I'd find it important that both c++ sources (for Greyhole, JPVerb) are built with the same faust version. If you don't, I'll try to find time to do this (hopefully soon) myself.

On 08. Nov 2017, at 19:49, Carlo Capocasa [email protected] wrote:

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.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

LFSaw avatar Nov 08 '17 19:11 LFSaw

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

LFSaw avatar Nov 16 '17 11:11 LFSaw

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.

capocasa avatar Nov 16 '17 11:11 capocasa

others are welcome to test, too :)

LFSaw avatar Nov 16 '17 12:11 LFSaw

Fantastic Till, will test soon!

On Thursday, November 16, 2017, LFSaw [email protected] wrote:

others are welcome to test, too :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/supercollider/sc3-plugins/pull/149#issuecomment-344905043, or mute the thread https://github.com/notifications/unsubscribe-auth/ABVJxj9cbLcpE31kgb2fT2sD6TrMEITcks5s3CZ8gaJpZM4N9gQg .

--

www.grond.at

florian-grond avatar Nov 16 '17 13:11 florian-grond