JPverb for SuperNova fix

[capocasa]

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]

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!

[capocasa]

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

[florian-grond]

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]

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

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

[florian-grond]

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

[capocasa]

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]

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]

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

Will check when back, thanks!

…

On Jul 18, 2017 8:28 PM, "Carlo Capocasa" ***@***.***> 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 <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVJxmWsIcjQGCejw1t0jHgkKqQ8p7jxks5sPU2vgaJpZM4N9gQg> .

[capocasa]

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]

great ! a good step forward

[capocasa]

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]

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]

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

[capocasa]

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]

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

[capocasa]

No worries! Thanks for looking into it!

[LFSaw]

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]

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 ***@***.***> 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 <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVJxitE6jtkYgbCS68a3ayeB-9c0aqiks5s0KxMgaJpZM4N9gQg> .

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

I guess, you mean

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

?

[capocasa]

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]

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 + updated FAUST file (if needed) + updated (generated) c++ file + updated [faust_src/README.md](https://github.com/supercollider/sc3-plugins/blob/master/source/DEINDUGens/faust_src/README.md) 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 ***@***.***> 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]

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 ***@***.***> 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 + updated FAUST file (if needed) + updated (generated) c++ file + updated [faust_src/README.md](https://github.com/supercollider/sc3-plugins/blob/master/source/DEINDUGens/faust_src/README.md) 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 ***@***.***> 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]

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

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]

others are welcome to test, too :)

[florian-grond]

Fantastic Till, will test soon!

…

On Thursday, November 16, 2017, LFSaw ***@***.***> wrote: others are welcome to test, too :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#149 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVJxj9cbLcpE31kgb2fT2sD6TrMEITcks5s3CZ8gaJpZM4N9gQg> .

-- www.grond.at

[florian-grond]

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]

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

[LFSaw]

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

[capocasa]

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]

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.

[dyfer]

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]

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]

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]

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.

[capital-G]

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]

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.