surge-synthesizer / shortcircuit-xt

Will be a sampler when its done!
GNU General Public License v3.0
262 stars 33 forks source link

Per proc input/output gain #970

Closed baconpaul closed 5 months ago

baconpaul commented 6 months ago

Over in #947 and then on discord we had a lot of chats about wanting i/o gains

so the idea is

  1. Each processor gets a pre and post gain control implemented in the engine etc... probably in cubicdb with range -inf +24 or so
  2. do the engine and data model work - routing.h consolidation makes this pretty easy
  3. add the two knobs above the boxes in the routing diagram (which means 'draw the routing diagram'). For now probably just have a tiny 8x2 array of knobs so I can test it tho.
mkruselj commented 6 months ago

I think we just want the output gains in the routing panel TBH. Any effects that are supposed to be input gain sensitive would already have that parameter IMO.

baconpaul commented 6 months ago

So then we should add a second gain to the waveshaper?

mkruselj commented 6 months ago

The specification for improved waveshaper back on Surge end is here. I don't think we need anything more than that.

baconpaul commented 6 months ago

Remember the conversation we had on discord yesterday about the missing third gain pre bias?

the solution to thst problem was input and output gain on the proc

the spec in that issue is exactly what’s in rack and short circuit and it has the low signal input bias problem which is why we wanted a pre bias gain. So either

  1. we add a pre bias gain to the waveshaper
  2. We add an input gain to all the procs or
  3. We don’t fix the waveshaper problem
mkruselj commented 6 months ago

The input gain IS supposed to be pre-bias, I think.

baconpaul commented 5 months ago

the waveshaper doesn’t have an input gain now. It has drive and output gain. That was the whole point

Andreya-Autumn commented 5 months ago

Here's a thought. I think we'll want to eventually do these two things anyway: 1: Have a way to normalize samples to some sensible level. 2: Let the volume control in the mapping pane go up to say +24dB.

...and if we do those, I think we're good without input controls no? In ser1 we most definitely are since prior output gain == input gain. And I think in par1 we will also be fine if we can get the overall volume into the procs right. Thoughts?

baconpaul commented 5 months ago

Yeah you know those are two good ideas. that plus output level solves almost all cases.

Andreya-Autumn commented 5 months ago

Yup! And since we now have a nice simple way to adjust velocity sensitivity which assumes all zones play even levels... a normalize feature would be really really useful to have. And that helps with the bias problem too. I'll make a separate issue for that.

mkruselj commented 5 months ago

Yeah I maintain that we don't need third gain on the waveshaper. Korg would've put it into Kronos if it were really a must, I feel. Hehe.

baconpaul commented 5 months ago

Do they have a pre-wave shaper gain in their circuit? If so indeed they don't need it.

Andreya-Autumn commented 5 months ago

Yeah it seems like the solution to the bias issue is to get the input normalized into the useful range, like we also have in Rack. And with the other proposals floating around that solution will be available in Shortcircuit also. :)

Andreya-Autumn commented 5 months ago

Outputs are in place, and are indeed useful. Only thing remaining is making them default to unity!

baconpaul commented 5 months ago

Oh wow I did default them to zero didn’t I

let me look at that tonight or tomorrow

Andreya-Autumn commented 5 months ago

Seems like you did. :) I tried setting a default explicitly around line 345 of processor.h, but that only ends up changing where it goes on double-click. It loads at 0 anyway. I guess the answer lies further out into the template woods than I dare tread lol.

baconpaul commented 5 months ago

https://github.com/surge-synthesizer/shortcircuit-xt/blob/30e443d5542592256302265545d831c300ee2313/src/dsp/processor/processor.h#L184 That 0 is from the days when it went -1..1

now it needs instead to be this value

https://github.com/baconpaul/airwin2rack/blob/fdbcf6778ad1969b83ddfe48bc9481ac0c4546b7/src-juce/AWConsolidatedProcessor.h#L210

Andreya-Autumn commented 5 months ago

986 merged, and we're done here. :)