TonicAudio / Tonic

Easy and efficient audio synthesis in C++
The Unlicense
523 stars 63 forks source link

Mp control switcher advance trigger #229

Closed morganpackard closed 9 years ago

morganpackard commented 11 years ago

In addition to accepting an "index" parameter, controlswitcher now accepts an "advanceTrigger" parameter, which increments the index.

ndonald2 commented 11 years ago

I'm a bit concerned that ControlSwitcher is getting too big for its britches. There are a lot of ways to change the active input, with varying degrees of mutual exclusiveness and precedence. Last time I went to use it after not using it for awhile, it took me awhile to figure out what parameter did what, exactly.

Rather than adding a bunch of semi-incompatible options, I'm more in favor of keeping the core purpose of each ControlGenerator relatively simple while providing ways to achieve more complex behavior through other combinations of ControlGenerators.

For example, keeping the input choosing mechanism confined to just inputIndex makes a simple promise that the output will always be equal to whatever input the inputIndex is pointing to - any additional logic re: the order in which the inputIndex advances or switches can be achieved pretty easily using combinations of other ControlGenerators. The functionality of advanceTrigger can easily be implemented using a ControlCounter as the inputIndex.

I'm willing to be convinced otherwise, but it just seems like this class is getting more complicated than it needs to be for the sake of saving a few lines of code elsewhere.

morganpackard commented 11 years ago

I've found the need to cycle through a ControlSwitcher pretty common, and am getting tired of wiring up a ControlSwitcher and ControlCounter together. I'm up for a totally separate class, but I think there needs to be some way to concisely cycle through values without having to combine generators.

On Mon, Nov 11, 2013 at 4:45 PM, Nick D. notifications@github.com wrote:

I'm a bit concerned that ControlSwitcher is getting too big for its britches. There are a lot of ways to change the active input, with varying degrees of mutual exclusiveness and precedence. Last time I went to use it after not using it for awhile, it took me awhile to figure out what parameter did what, exactly.

Rather than adding a bunch of semi-incompatible options, I'm more in favor of keeping the core purpose of each ControlGenerator relatively simple while providing ways to achieve more complex behavior through other combinations of ControlGenerators.

For example, keeping the input choosing mechanism confined to just inputIndex makes a simple promise that the output will always be equal to whatever input the inputIndex is pointing to - any additional logic re: the order in which the inputIndex advances or switches can be achieved pretty easily using combinations of other ControlGenerators. The functionality of advanceTrigger can easily be implemented using a ControlCounter as the inputIndex.

I'm willing to be convinced otherwise, but it just seems like this class is getting more complicated than it needs to be for the sake of saving a few lines of code elsewhere.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/229#issuecomment-28242553 .

Morgan Packard cell: (720) 891-0122 twitter: @morganpackard

ndonald2 commented 11 years ago

These situations are when I wish there were categories in C++.

What if advanceTrigger just wraps its argument inside a ControlCounter and sets that as inputIndex, rather than having extra precedence-dependent logic to handle a totally separate input? It would just need to be well documented that advanceTrigger and inputIndex will overwrite each other.

ndonald2 commented 11 years ago

Or just wrapping it in a separate "adapter" class would also work.

morganpackard commented 11 years ago

ControlRotater?

I'm thinking for now maybe just two different wrapper classes, but not modify the underscore class.

Sent from my iPhone

On Nov 11, 2013, at 5:09 PM, "Nick D." notifications@github.com wrote:

Or just wrapping it in a separate "adapter" class would also work.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/229#issuecomment-28244583 .

ndonald2 commented 11 years ago

I like the idea of just wrapping it differently, or using a factory method (see below). If we're gonna wrap it, how about ControlStepSequencer with stepTrigger as the input? Could also have a jumpToIndex input or something to handle the triggerForInput stuff.

Speaking of which, the triggerForInput stuff confuses me a bit. It seems like it's just meant to immediately switch to a particular input, but can't you just do that by changing inputIndex in response to something? Maybe we should figure out a better way to do that externally to the class (such as a ControlValue that can be set by another ControlGenerator)

FWIW, I had originally envisioned ControlSwitcher as a multiplexer - you just pick which input is sent to the output, no more, no less. I get that there are other scenarios that you want to use it for, but I still think that should be broken out externally to the switcher's core logic, which could be made easier through other adapters/wrappers or "factory" static class methods if it's a really common scenario, e.g.

// this is a public member function on `ControlSwitcher`
// as long as this is implemented in the .cpp file, it's fine to just import "ControlCounter.h"
static ControlSwitcher MakeRotatingSwitcher(vector<float> inputs, ControlGenerator trigger)
{
   return ControlSwitcher().setFloatInputs(inputs).inputIndex(ControlCounter().end(inputs.size()).trigger(trigger));
}

// then you just call this to get one
ControlSwitcher mySwitcher = ControlSwitcher::MakeRotatingSwitcher(my_floatvec, my_trigger);

This approach keeps things incredibly flexible by having each component do one particular task, rather than having lots of precedence-dependent logic internal to the component.

ndonald2 commented 11 years ago

Actually, thinking more about the factory method approach, we could feasibly make "extension" classes that just provide static factory methods to create complex generator chains. That's sort of like a category...

morganpackard commented 11 years ago

My first impression is that I'd rather not introduce a new paradigm, and would rather just create a new class. Totally up for more discussion though.

ndonald2 commented 11 years ago

That's a very good point. Wrapping is probably the way to go, then.