beast
beast copied to clipboard
tim-janik
Portamento plugin and poly scale properties (rebase)
Portamento plugin (rebased), see original pull request https://github.com/tim-janik/beast/pull/27
Hey Stefan, this looks mostly good, a bit of polishing is left though:
-
Please squash the intermediate commits that contain "bug fix" and "printf". As long as things aren't merged you can still fix up the branch history to have meaningful commits only. git rebase -i also makes it really easy to fix or squash intermediate states.
-
Why is "using std::max" required in a namespace that already does "using namespace std" ? And, in general it's better not to do "using namespace std", long term that has high potential of causing conflicts, e.g. with the introduction of std::any we're likely running into problems with Aida::Any. Instead, use std::max, std::vector, or "using std::vector;" because you know that'd the one vector you mean to make use of in your code. I.e. just remove and fixup the "using namespace std" in portamento.cc.
-
Get rid of "FIXME"s in code meant to be merged into mainline. For the step/page increments I'd say just remove them in a commit and then rebase+fix that commit into the one that used to introduce the FIXMEs. If we find out later the increments don't do what we want, we can adjust them later.
-
Seeing that your plugin does two multiplications per sample just to convert back and force between frequency values makes me wonder if we choose the right path there. This should be tracked in a different bug, but what's your take on making BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ NOPs, i.e. *1.0 ? DO we really rely on freqs being scaled down into 0..1 somewhere? It's not like freqs are all that useful to run through a low-pass or an amp...
-
Finally, please rebase again, to track latest master ;-)
Ok, I believe I addressed all your concerns now.
Please squash the intermediate commits that contain "bug fix" and "printf". As long as things aren't merged you can still fix up the branch history to have meaningful commits only. git rebase -i also makes it really easy to fix or squash intermediate states.
Done.
Why is "using std::max" required in a namespace that already does "using namespace std" ? And, in general it's better not to do "using namespace std", long term that has high potential of causing conflicts, e.g. with the introduction of std::any we're likely running into problems with Aida::Any. Instead, use std::max, std::vector, or "using std::vector;" because you know that'd the one vector you mean to make use of in your code. I.e. just remove and fixup the "using namespace std" in portamento.cc.
Right. Today, I also believe that we should never have code like 'using namespace std', ever. I removed it and it still compiles.
Get rid of "FIXME"s in code meant to be merged into mainline. For the step/page increments I'd say just remove them in a commit and then rebase+fix that commit into the one that used to introduce the FIXMEs. If we find out later the increments don't do what we want, we can adjust them later.
Done.
Seeing that your plugin does two multiplications per sample just to convert back and force between frequency values makes me wonder if we choose the right path there. This should be tracked in a different bug, but what's your take on making BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ NOPs, i.e. *1.0 ? DO we really rely on freqs being scaled down into 0..1 somewhere? It's not like freqs are all that useful to run through a low-pass or an amp...
Yes, we discussed this in person: I don't have strong feelings for either version, but I believe we could safely use plain frequencies and maybe introduce some kind of type-tag on streams to show whether a frequency is expected in the ui.
On the other hand, if speed is all you care about, as long as BSE_SIGNAL_TO_FREQ / BSE_SIGNAL_FROM_FREQ is a linear function (not exponential or anything), we could simply operate on the scaled values directly, without calling either macro once-per-sample.
Finally, please rebase again, to track latest master ;-)
Done.
Note: I also added a minimal portamento demo, which just shows off this feature in isolation. As things should be self contained, I neither used bleposc nor drum samples (i.e. from some hydrogen kit), although this would probably improve the overall sound quality of the demo.
I rebased this branch, you should be able to merge this now without problems.