grame-cncm / faust

Functional programming language for signal processing and sound synthesis
http://faust.grame.fr
Other
2.58k stars 322 forks source link

want to add to the library #150

Closed magnetophon closed 6 years ago

magnetophon commented 6 years ago

I'd like to add most of faustCompressors to the library.

If possible, I'd like it to include proper comment and explanation, in particular for the slidingReduce function that lays at the base of it, because without, that code is unreadable. I had trouble understanding my own code, using the comments I wrote 2 years ago, so I re-wrote them.

My questions are:

ps: Thanks @rmichon for encouraging me to add this to the libs. I still postponed it for way too long afterwards, mainly because of the above issues...

rmichon commented 6 years ago

Hey Bart! Yes, that would be great! In order for this to be added to the libraries, some standards should be respected so your code integrates with what's already out there. First, each function should be documented by declaring this before the function itself:

//--------------------`(co.)compressor_mono`-------------------
// Description
//
// #### Usage
//
// ```
// _ : compressor_mono(ratio,thresh,att,rel) : _
// ```
//
// Where:
//
// * `ratio`: compression ratio (1 = no compression, >1 means compression)
// * `thresh`: dB level threshold above which compression kicks in (0 dB = max level)
// * `att`: attack time = time constant (sec) when level & compression going up
// * `rel`: release time = time constant (sec) coming out of compression
//
// #### References
//
// * Potential references.
//------------------------------------------------------------

This system is used to generate the documentation of the libraries so this format should be meticulously respected. All the functions that are "exposed" to the users (i.e., at the root level of the library) should be documented that way: there's no "hidden functions" in the Faust libs. Then you must decide where the functions exposed to the users are going to be declared: which libraries (if a function is not a compressor, then it shouldn't be in the compressor lib), which section of the library (do we need to create new sections?), etc. If a function is only used once by another function and should be "hidden", then it should be declared inside the function that's using it using with{}. Examples (.dsp) files should go in the examples folder of the Faust repo. Just have a look at the source code of the Faust libs to get an idea of how this should be organized...

Beside that, comments within the code are more than welcome, of course and GPLv3 should be fine unless someone objects, @orlarey?

Ideally, to make our life easier, it would be great if you could send us a reformatted version of your system organized in the same way as in the Faust repo. Or you could fork the Faust repo and then we could merge your version with the current master-dev branch. It's up to you...

magnetophon commented 6 years ago

Hi Romain,

How do you like this and these declarations?

Could you also have a look at my comments and tell me:

Regarding location:

I think the slidingReduce is more general then just compressor useage, so I guess it should be in reduce.lib. What do you think @orlarey ? Any reason why reduce.lib is currently not in stdfaust.lib? The rest obviously goes in compressors.lib.

I have put all internal functions in a with{} statement. Anything else I need to do for slidingReduce?

The compression functions can follow later, I think.

Also, I'd love to get a codereview of slidingReduce. I'm sure there's room for improvement, and would love to learn from you all.

magnetophon commented 6 years ago

Any news on this?

rmichon commented 6 years ago

Sorry for the late reply, it's the end of the quarter here and I'm rather busy haha.

How do you like this and these declarations?

I think this is great!

Could you also have a look at my comments and tell me

I think so. I think all these functions should go in a new section of basics.lib. In that case, the "eduction functions" could go in the

//========Sliding Reduce==================
// Blahblah...
// Education function goes here.
// Blahblah
// Other education function goes here
// etc.
//=============================================

If you like that option, could you reformat the beginning of slidingReduce.lib to match this standard?

Other than that, it all looks good to me. As you said, I think we should start with that and then continue with the compressor functions. What do you think? Do you want me to go ahead?

magnetophon commented 6 years ago

Sorry for the late reply, it's the end of the quarter here and I'm rather busy haha.

No problem. If it weren't for your encouragement I'd have postponed it longer!

I've reformatted slidingReduce.lib. I chose the /* */ style of commenting so the code blocks can be easily copied without having to remove // from each line. Is that OK?

I'd like to add to the comments that the operator <op> can only be one where it doesn't matter in which order the samples are processed, but I'm not sure what the proper term for that is. Is "comunicative" the word I'm looking for?

Other than that, I'd love for you to go ahead! :smile:

rmichon commented 6 years ago

There it is: https://github.com/grame-cncm/faustlibraries/commit/c8badeecbbfb4f7a6635fcf066bdb939869d8b68

I had to do a fair amount of changes in the comments to make it compatible with the Faust libraries system. Let me know if you see anything to change. Also, this is completely untested! :) Make sure you like the corresponding documentation in doc/library.html.

For now this is only on the libraries repo but we can merge that to the main Faust repo once you approve my commit.

magnetophon commented 6 years ago

Thanks Romain!

Sorry you had to make all those changes. I'll try to be more alert, but if it happens again, feel free to just point me at the problems instead of fixing them yourself!

Some things I noticed:

magnetophon commented 6 years ago

I made a PR with the above changes.

Now for the next step: the actual compressors.

IMHO there are some problems with the current compressors:

Fixing these problems would involve either:

How do you feel about these issues?

cc @josmithiii

josmithiii commented 6 years ago

I vote for improving the implementations under the existing names! I don't think it breaks backward compatibility to improve placeholder compressor/limiter functions to bring them closer to their original aim.

On Fri, Mar 23, 2018 at 8:59 AM, Bart Brouns notifications@github.com wrote:

I made a PR with the above changes.

Now for the next step: the actual compressors.

IMHO there are some problems with the current compressors:

  • kneesmooth has nothing to do with what is normally called knee
  • Using the name of a well known compressor (1176) gives much higher expectations than can be met by the code. Especially whit the R4 added to the name, it conjures up images of something modeled after the hardware.
  • The stereo compressor uses the sum of L and R to calculate the gain, which should be the max, especially in the context of a hard limiter.

Fixing these problems would involve either:

  • broken backwards compatibility, or
  • awkward names for the new functions

How do you feel about these issues?

cc @josmithiii https://github.com/josmithiii

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/grame-cncm/faust/issues/150#issuecomment-375713587, or mute the thread https://github.com/notifications/unsubscribe-auth/ACGVFUFGSUjkvbpg6JjOV7ueNU7DJJ3lks5thRvmgaJpZM4SlzYg .

--

Julius O. Smith III jos@ccrma.stanford.edu Professor of Music and, by courtesy, Electrical Engineering CCRMA, Stanford University http://ccrma.stanford.edu/~jos/ http://ccrma.stanford.edu/

magnetophon commented 6 years ago

@josmithiii Even if that means adding parameters that weren't there before (knee), and changing the name, range and meaning of others (ratio -> strength)?

I think that would be the least bad solution,but I'm hoping someone thinks of a better one.

How do you feel about my complaint about the 1176? I think renaming it to limiter would improve the believability and quality of the lib.

josmithiii commented 6 years ago

I would use a new name when changing the parameters.

Please send me the new ideal API (just the electric markdown comments that feed to the autogenerated doc is enough) and I'll ponder the naming situation. There are several naming conventions going on, so one trick, for example, is to use underbars differently, etc.

Is this already in your PR?

On Fri, Mar 23, 2018 at 1:23 PM, Bart Brouns notifications@github.com wrote:

@josmithiii http:///josmithiii Even if that means adding parameters that weren't there before (knee), and changing the name, range and meaning of others (ratio -> strength)?

I think that would be the least bad solution,but I'm hoping someone thinks of a better one.

How do you feel about my complaint about the 1176? I think renaming it to limiter would improve the believability and quality of the lib.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/grame-cncm/faust/issues/150#issuecomment-375787670, or mute the thread https://github.com/notifications/unsubscribe-auth/ACGVFZ26XXcGEyBSa7XeLxzGQ5Ge_fS8ks5thVmpgaJpZM4SlzYg .

--

Julius O. Smith III jos@ccrma.stanford.edu Professor of Music and, by courtesy, Electrical Engineering CCRMA, Stanford University http://ccrma.stanford.edu/~jos/ http://ccrma.stanford.edu/

josmithiii commented 6 years ago

On GitHub I only see the PR "small fixes for slidingReduce".

As a practical matter, you can just send me your proposed final version of compressors.lib and I'll deal

On Fri, Mar 23, 2018 at 1:38 PM, Julius Smith jos@ccrma.stanford.edu wrote:

I would use a new name when changing the parameters.

Please send me the new ideal API (just the electric markdown comments that feed to the autogenerated doc is enough) and I'll ponder the naming situation. There are several naming conventions going on, so one trick, for example, is to use underbars differently, etc.

Is this already in your PR?

  • Julius

On Fri, Mar 23, 2018 at 1:23 PM, Bart Brouns notifications@github.com wrote:

@josmithiii Even if that means adding parameters that weren't there before (knee), and changing the name, range and meaning of others (ratio -> strength)?

I think that would be the least bad solution,but I'm hoping someone thinks of a better one.

How do you feel about my complaint about the 1176? I think renaming it to limiter would improve the believability and quality of the lib.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

--

Julius O. Smith III jos@ccrma.stanford.edu Professor of Music and, by courtesy, Electrical Engineering CCRMA, Stanford University http://ccrma.stanford.edu/~jos/

-- Julius O. Smith III jos@ccrma.stanford.edu Professor of Music and, by courtesy, Electrical Engineering CCRMA, Stanford University http://ccrma.stanford.edu/~jos/

magnetophon commented 6 years ago

I haven't written the formal documentation of the compressors yet. I'll do so tomorrow and make a PR.

sletz commented 6 years ago

For the next time, comments and discussions on "faust libraries" could better be done on the now separated grame-cncm/faustlibraries site... instead of grame-cncm/faust ((-;

rmichon commented 6 years ago

@sletz +1

@magnetophon I merged your PR here: https://github.com/grame-cncm/faustlibraries/commit/521c28637a2954b53b2b432675a6a7a21228fe02

I'll let you figure out the potential naming conflicts with @josmithiii but I would personally vote for not barking backward compatibility simply by using different names as Julius suggested. @magnetophon just make a PR to add your compressor functions. I'll review it and I'll make some adjustments if necessary. Moving forward, that's all really great!

magnetophon commented 6 years ago

Continued here.