Flanger by daschuer · Pull Request #1362 · mixxxdj/mixxx

daschuer · 2017-10-14T23:55:59Z

This is now a "complete" Flanger as known from stomp boxes.
Try it out.

… function.

sohet · 2017-10-15T13:26:42Z

Thank you for your quick hack, but my first impression is not so different from #1256. To confirm the true flanger sound, I am hearing some tracks through one of calf-plugins. Calf flanger can produce "Jet flyby" as usual without additional effect such as feedback or stereo phase shift. And I remember nearly same sound made by Mixxx flanger before #1256. I can not look into the code now sorry.

daschuer · 2017-10-16T00:28:38Z

This is the original 30 Dec 2015 Version:
https://github.com/daschuer/mixxx/blob/645358c5ee2172b30731cdd85464b24c4bdb4980/src/effects/native/flangereffect.cpp

It has a sample rate depending sound. Which sample rate do you use?

@44100 we had before a delay range 0.22 ms ... 11 ms.
Now we have a sample rate independent 3 ms to 14 ms

I have just noticed that I lost a decimal point adopting the range of hardware flangers.
I have played a bit and 0.22 to 13 ms works nice for me.
Do you agree?

Be-ing

Thank you for working on this, we are getting towards a good flanger effect. There are still some details to figure out as noted in the comments below.

Be-ing · 2017-10-16T03:42:35Z

+
+    EffectManifestParameter* regen = manifest.addParameter();
+    regen->setId("regen");
+    regen->setName(QObject::tr("Regen."));


How about renaming this to "Feedback"?

This new parameter is really fun, thanks! :)

Regen is the established name for that on hardware flangers.

Okay, then set the full name to "Regeneration" and use "Regen" (no ".") for the shortName.

Be-ing · 2017-10-16T03:42:55Z

+    manual->setId("manual");
+    manual->setName(QObject::tr("Manual"));
+    manual->setDescription(QObject::tr("Controls the delay offset of the LFO (low frequency oscillator).\n"
+            "With width at zero, it allows to manual sweep over the entire delay range."));


I don't understand what this does. I hear no difference when I manipulate the parameter.

If width is fully right, it has no effect. If width is on half, you can shift the delay loop by manual. If width is zero, you can perform you own lfo loops using manual.
This is described here https://www.premierguitar.com/articles/Get_More_Out_of_Your_Flanger a standard behaviour on hardware flangers.

I am happy if you can propose a better tool tip.

Be-ing · 2017-10-16T03:43:59Z

+
+
+    // With and Manual is limited by amount of amplitude that remains from width
+    // to kMaxDelaMS


typo in comment: kMaxDelayMs

Be-ing · 2017-10-16T03:45:17Z


+namespace {
+constexpr double kMaxDelayMs = 13.0;
+constexpr double kMinDelayMs = 0.22;


Where did these numbers come from? Are they arbitrary values you determined by guessing and checking what sounds good? It is fine if that is the case as long as you document it in a comment.

I am curious to read @sohet results.
Hardware Flangers start at 0.4 or 0.3. to 13 or 14. I pick 0.22 because that's our old minimum at 44.1 kHz. It was 0.1 at @96 kHz though.

Be-ing · 2017-10-16T03:48:58Z

+    width->setControlHint(EffectManifestParameter::ControlHint::KNOB_LINEAR);
+    width->setSemanticHint(EffectManifestParameter::SemanticHint::UNKNOWN);
+    width->setUnitsHint(EffectManifestParameter::UnitsHint::UNKNOWN);
+    width->setDefaultLinkType(EffectManifestParameter::LinkType::LINKED);


This is not suitable as a parameter to link to the metaknob by default because there is no way to make the effect not change the sound by manipulating this parameter. When the metaknob is fully left, the effect should not be heard.

That is no requirement for the super knob, because this use case is issued with the dry /wet knob. Mapping super to the Mix will just duplicate that behaviour.

The Mix parameter is redundant anyway. I have kept it because it was the old depth knob. Can we remove it?

It is a requirement that every effect can make no audible change with the default metaknob linking, otherwise there is no way to bring in the effect without a sudden change without abusing the chain dry/wet knob which manipulates all the effects of the chain together.

What do you propose?

I propose keeping the metaknob linked to the Mix knob by default. Linking the metaknob to the Width parameter is a valid use case, but I don't think it should be the default.

Could we make it so the effect does not make an audible change when Width is 0? If so, let's remove the Mix parameter and leave Width linked to the metaknob.

Linking to width does not work, because width sets the width that is cycled around the manual position.

Be-ing · 2017-10-16T03:49:44Z

+    regen->setControlHint(EffectManifestParameter::ControlHint::KNOB_LINEAR);
+    regen->setSemanticHint(EffectManifestParameter::SemanticHint::UNKNOWN);
+    regen->setUnitsHint(EffectManifestParameter::UnitsHint::UNKNOWN);
+    regen->setDefault(0.0);


How about making this 0.25 or 0.50 by default?

Be-ing · 2017-10-16T03:50:38Z

    period->setUnitsHint(EffectManifestParameter::UnitsHint::BEATS);
    period->setMinimum(0.00);
-    period->setMaximum(4.00);
+    period->setMaximum(20.00);


20 beats? Why? How does that make sense musically? How about 16 beats? Maybe we should go back to a logarithmic scale for this parameter.

I do not mid about the maximum.
20 just marches our original version @96 kHz. Hardware flangers have a Max around 10 s that's 20 for a 120 BPM track.

Real flangers have also a speed (fast fully right) parameter with a Hz scale. I like this when using, but it does not match our other effects.

How about that? @sohet?

This is synced to the tempo, so the time in seconds is irrelevant. The maximum is important because turning the parameter to the maximum should be useful somehow. I can't think of a musical context in which a 20 beat period would make sense. A maximum of 16 beats or 4 bars with a 4/4 time signature seems to be a reasonable maximum. Then the user can also know the center of the knob is 8 beats and 1/4 of the knob is 4 beats (or 1 bar in 4/4).

20 beats? Why? How does that make sense musically? How about 16 beats? Maybe we should go back to a logarithmic scale for this parameter.

I don't want the max value to be shorter than the current 20. How about 32 and logarithmic scale?

Real flangers have also a speed (fast fully right) parameter with a Hz scale. I like this when using, but it does not match our other effects. How about that?

I also like the way.

Is this always going to be in beats rather than hertz where a beat is detected? It's often musically desirable to have an effect that is not synced to the tempo, to create tension

@christophski I tried adding a button parameter to disable tempo syncing in #1257 but I did not think it was useful. For Echo, yes it is helpful, but I don't think it is for Flanger.

Be-ing · 2017-10-16T03:51:45Z

        SampleUtil::clear(delayLeft, numSamples);
        SampleUtil::clear(delayRight, numSamples);
+        pState->previousPeriodFrames = -1;
+        pState->prev_regen  = -1;


extra space to the left of the =

Be-ing · 2017-10-16T07:10:41Z

+    const double width_delta = (width - pState->prev_width) /
+            (numSamples / kChannels);
+    const double width_start = pState->prev_width + width_delta;
+    pState->prev_width = width;


We are using this parameter ramping technique in several effects now. Perhaps you could factor it out to a common function.

sohet · 2017-10-16T11:53:55Z

True flanger is comming back! It seems that the deley shorter than 2ms is needed for "Jet flyby".

It has a sample rate depending sound. Which sample rate do you use?

44100 or 48000.

@44100 we had before a delay range 0.22 ms ... 11 ms. Now we have a sample rate independent 3 ms to 14 ms I have just noticed that I lost a decimal point adopting the range of hardware flangers. I have played a bit and 0.22 to 13 ms works nice for me. Do you agree?

In the case of Calf flanger LV2 plugin, the value "Min delay" ranges from 0.1 to 10ms . Even min. value 0.1ms can do well, and has a merit not blurring kick so much. Max. value 10ms exposes deleyed sound itself, and almost useless. I also apply the range 0.1-10ms to Mixxx, and get good result for me.

sohet · 2017-10-17T11:01:09Z

"manual" parameter is a bit difficult to be grasped and used. How about these alternatives?

"manual" value is min. of LFO delay. (Calf plugins)
// manual = math_clamp(manual, minManual, maxManual);
manual = width / 2.0 + manual;
"manual" value is center of LFO delay and limits max. of width. (my present favorite)
// manual = math_clamp(manual, minManual, maxManual);
width = 2.0 * width * (manual - kMinDelayMs) / kMaxLfoWidthMs;

Any way, the sweet spot lies around 1ms, so logarithmic scale seems to be appropriate to the knob.

Be-ing · 2017-10-17T18:04:16Z

width = 2 * width * (manual - kMinDelayMs) / kMaxLfoWidthMs;

Did you mean manual = 2 * width * (manual - kMinDelayMs) / kMaxLfoWidthMs;? This works better. The Manual parameter is no longer useless with a large Width parameter.

sohet · 2017-10-18T09:24:18Z

Did you mean manual = 2 * width * (manual - kMinDelayMs) / kMaxLfoWidthMs;?

No, "manual" is as is from the knob and scales "width" within the limit. So if manual = min, width = 0 irrespective of the knob position. If this manner is against your intuition, then the first alternative (Calf plugins) is your choice. Both "manual" and "width" can always change the values and sound in the implimentation. I think it to be suit for general use in spite of my preference for the other.

sohet · 2017-11-05T23:51:11Z

Unfortunately the behaviour does not know into which control it is installed, so the warning messages is only weak. Do you have an idea, which control issues this warning?

Critical [Controller]: DEBUG ASSERT: "false" in function virtual double ControlNumericBehavior::valueToMidiParameter(double) at src/control/controlbehavior.cpp:27
Warning [Controller]: valueToMidiParameter not implemented

The warning appears when "LoadSelectedTrack" is evoked by a controller via xml.

sohet · 2017-11-06T01:50:09Z

The warning appears when "LoadSelectedTrack" is evoked by a controller via xml.

And all controls mapped to buttons (not knobs) directly by xml (not via js) suffer from this warning.

daschuer · 2017-11-06T20:55:31Z

Ok,I have now implemented the missing midi conversion functions.
@sohet: Can you verify that this works? Thank you.

sohet · 2017-11-07T01:28:21Z

Can you verify that this works?

The warning does not appear any more, thank you.

(I noticed now another bug not specific to flanger. An effect seems to be inserted twice to the line when group_[Headphone]_enable.)

daschuer · 2017-11-07T07:09:00Z

Does it mean I can merge this now?

group_[Headphone]_enable issue: Do you use this plain branch or a combination with other branches?

sohet · 2017-11-07T10:50:13Z

group_[Headphone]_enable issue: Do you use this plain branch or a combination with other branches?

On this issue, current master and this branch are at the same state. I don't mind which Pull the bug will be fixed at.

Does it mean I can merge this now?

My wish list within this Pull is,

Scales of "manual" and "width": you have changed the scale of "width" to logarithmic, but the scale of "manual" is still linear. It is preferable that both scales are the same, for both parameters have the same dimension (ms). I want both to be logarithmic, but if many users don't think so, it is better to revert the scale of "width" to linear. (If a linear scale will be chosen, isn't it possible that max values of both parameters are reduced to about 10ms? If a max value is relatively small, a linear scale is tolerable.)
Output level: the output of this effect is a bit louder than the input. When the ratio of input and delay is 1:1, 3dB decrease makes the output to be at the same loudness for my ears as the input.

Adjusting LFO waveforms, parameter relations, ranges and defaults may be later.

sohet · 2017-11-14T06:30:03Z

One more point,

Initial sound quality: when enabled for the first time to the deck, sometimes a click noise is heard at the beginning and only single channel seems to be changed by the effect (only single channel is loud, or broken in a different way?).

daschuer · 2017-11-17T00:00:30Z

Ready!

JosepMaJAZ · 2017-11-17T15:41:23Z

It's good here too.

Just one thing, previously, tweaking the amplitude was fine, but current implementation feels like making small jumps so it's not as natural as before (maybe it resets the position, or maybe previously it attempted to soften the change, I don't know).

daschuer · 2017-11-17T16:42:46Z

The width parameter is now logarithmic. Ramping for the width parameter is in place. Mmm I will test it again.

daschuer · 2017-11-17T18:34:22Z

@JosepMaJAZ: it works for me, do you have a receipt to reproduce the issue?

JosepMaJAZ · 2017-11-17T19:08:23Z

Mmm... I have to apologize... It might have been the song that I tried it with initially, since now that I'm checking it with other songs it works as expected.

What I heard was as if the width (LFO amplitude) parameter affected the delay without ramping, which seemed to cause small delay jumps.

sohet · 2017-11-18T04:33:57Z

It sounds almost fine, but still remains one issue. When enabled for the first time to the deck, the right channel outputs in most cases loud sound for a moment and then silence. If "regen" is 0, the disturbance disappears mostly except initial faint noise from the right.

daschuer · 2017-11-18T18:29:11Z

@sohet: Good catch, thank you. Got it!

sohet · 2017-11-19T00:33:51Z

Thank you, @daschuer. LGTM!

daschuer · 2017-11-19T00:59:26Z

Greate! Thank you for review and testing.

ronso0 · 2017-12-10T01:44:07Z

Since this was merged I can't use [Library],MoveFocusBackward etc. with my controller anymore.

Critical [Controller]: DEBUG ASSERT: "pBehavior" in function double ControlDoublePrivate::getMidiParameter() const at src/control/control.cpp:256
Warning [Controller]: Cannot get "[Library]" , "MoveFocus" by Midi 

Critical [Controller]: DEBUG ASSERT: "pBehavior" in function void ControlDoublePrivate::setValueFromMidi(MidiOpCode, double) at src/control/control.cpp:247
Warning [Controller]: Cannot set "[Library]" , "MoveFocus" by Midi

Commits 49078e7 fbf7b45

daschuer · 2017-12-11T23:30:37Z

Hi @ronso0
I have fixed the failed assertion
#1408
However [Library],MoveFocusBackward is not effected.

daschuer added 8 commits October 14, 2017 14:49

Rename Depth to Mix and added the standard flanger parameters without…

a470792

… function.

Make flanger sample rate independent

7303e59

Enable width and manual parameter

bc06153

enable regen parameter and added a tanh clipper

daeecad

Rvert parameter speed back to period

0e5110d

De-crackel period knob

138ecfc

De-crackle mix and regen knob

d313ceb

De-crackle width and manual knob

54aa94c

daschuer mentioned this pull request Oct 14, 2017

sync flanger period to tempo #1257

Merged

1 task

tweak min an max delay

408a182

Be-ing suggested changes Oct 16, 2017

View reviewed changes

Be-ing reviewed Oct 16, 2017

View reviewed changes

Be-ing mentioned this pull request Oct 17, 2017

Flanger & Phaser Depth parameter improvement #1326

Closed

daschuer added 11 commits October 18, 2017 15:19

use override keyword in controlbehaviour.h

3b78e41

Disable broken midi access to COs where no behaviour is installed

49078e7

clean up function names a bit

12c286d

verify lokal pBehavior

fbf7b45

Warn about missing Midi representation

af7ea00

Added KNOB_LINEAR_INVERSE and KNOB_LOGARITHMIC_INVERSE

e797489

Made a logarithmic flanger speed knob

99cd401

Added long name for Regeneration parameter

6ec565c

default Regen to 0.25

87d5863

fix typo and code style

2ba1e81

Added new rampingvalue helper class template

c52cfd7

daschuer mentioned this pull request Nov 5, 2017

Features Gater effect #1299026 #1370

Merged

daschuer added 3 commits November 5, 2017 01:32

Fix constexpr issue

f287686

fix include guard

141fb99

Fix the constexpr issue again

4ed3a2d

Implement missing midi convert functions

7778953

daschuer added 3 commits November 16, 2017 23:07

make manual logarthmic

c3d91cc

Added gain correction

78fe9fe

Fix typo to remove click noise when starting flanger

d492f57

Fix typo in c-tor

4776def

daschuer merged commit 69d5e01 into mixxxdj:master Nov 19, 2017

daschuer mentioned this pull request Dec 11, 2017

Make two plain controls a PushButton to make them Midi accessable. #1408

Merged

daschuer deleted the fanger branch September 7, 2021 21:06



		// With and Manual is limited by amount of amplitude that remains from width
		// to kMaxDelaMS

Uh oh!

Conversation

daschuer commented Oct 14, 2017

Uh oh!

sohet commented Oct 15, 2017

Uh oh!

daschuer commented Oct 16, 2017

Uh oh!

Be-ing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sohet commented Oct 16, 2017

Uh oh!

sohet commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Oct 17, 2017

Uh oh!

sohet commented Oct 18, 2017

Uh oh!

sohet commented Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sohet commented Nov 6, 2017

Uh oh!

daschuer commented Nov 6, 2017

Uh oh!

sohet commented Nov 7, 2017

Uh oh!

daschuer commented Nov 7, 2017

sohet commented Oct 17, 2017 •

edited

Loading

sohet commented Nov 5, 2017 •

edited

Loading

sohet commented Nov 7, 2017 •

edited

Loading