sync echo effect to BPM by Be-ing · Pull Request #1256 · mixxxdj/mixxx

Be-ing · 2017-05-10T07:27:08Z

make Delay knob range from 0 to 2 beats

ronso0 · 2017-05-10T17:41:59Z

Cool, thanks for working on this and #1254 and #1255!!
So the knob would be linear, meaning center position would be 1 beat?

Be-ing · 2017-05-10T17:56:59Z

So the knob would be linear, meaning center position would be 1 beat?

Kinda but not exactly. The minimum is not 0, so the exact center of the knob is approximately 1 beat but not exactly 1 beat. I'll try implementing a workaround for that.

ronso0 · 2017-05-10T18:16:22Z

Okay. But the default (right-clicking the knob) can be set to 1 beat, right?

Be-ing · 2017-05-10T18:59:21Z

Okay. But the default (right-clicking the knob) can be set to 1 beat, right?

I have the default set as 1/2 beat. Try it, I think you'll find it makes sense.

daschuer · 2017-05-10T20:10:40Z

We cannot just replace delay by beats. Echo is a typical mic effect without any beat information.

Be-ing · 2017-05-10T20:14:08Z

We cannot just replace delay by beats. Echo is a typical mic effect without any beat information.

As explained by the comment in the code, a tempo of 100 BPM is assumed if there is no tempo information available. Do you have any ideas for how to handle that case better? Previously the delay value was random, so assuming 100 BPM isn't much better or worse.

and make parameter knob center exactly 1 beat / 1 second

Be-ing · 2017-05-11T15:41:37Z

+    delay->setName(QObject::tr("Time"));
+    delay->setDescription(QObject::tr("Delay time\n"
+        "0 - 2 beats if tempo is detected (decks and samplers) \n"
+        "0 - 2 seconds if no tempo is detected (mic & aux inputs, master mix, headphone mix)."));


@esbrandt is this a good way to handle the tooltip?

ronso0 · 2017-05-12T18:43:16Z

I tested this together with #1254 (which works great with Echo!) but somehow the default delay is always a bit higher than twice the BPM.
To test I played 2-beat loops with clear high hats or bass drums, Time knob I left at default position (1/2 beat?). For a 160 BPM track the echo had around 180 BPM, and for a 100 BPM track I got someting like 108.

ronso0 · 2017-05-12T18:50:55Z

And Sent was full On after start although Meta knob was a lower limit.

Be-ing · 2017-05-12T19:24:18Z

I tested this together with #1254 (which works great with Echo!) but somehow the default delay is always a bit higher than twice the BPM.
To test I played 2-beat loops with clear high hats or bass drums, Time knob I left at default position (1/2 beat?). For a 160 BPM track the echo had around 180 BPM, and for a 100 BPM track I got someting like 108.

Maybe you're getting confused by the feedback? Try playing with the feedback parameter. And yes, the default time is 1/2 beat.

ronso0 · 2017-05-13T13:53:22Z

Maybe you're getting confused by the feedback? Try playing with the feedback parameter.

May be, but I don't see how feedback affects timing. This sounded odd that's why I measured it.
Will test again.

In case someone does not want it to synchronize to the tempo

Be-ing · 2017-05-26T17:10:45Z

Do you like the new scaling for the Time parameter? It ranges from 1/4 to 2 beats and the value is floored to the nearest 1/4 beat. If we like this, I'll adopt it for the other temporal effects after merging this. I think flooring to the nearest 1/2 beat might be more appropriate for flanger and phaser.

daschuer · 2017-05-27T22:18:48Z

-    delay->setDescription(QObject::tr("Delay time (seconds)"));
+    delay->setName(QObject::tr("Time"));
+    delay->setDescription(QObject::tr("Delay time\n"
+        "1/4 - 2 beats rounded down to nearest 1/4 beat if sync parameter is enabled and tempo is detected (decks and samplers) \n"


1/4 - 2 beats, snapping to the nearest 1/4 beat step if ...

It is a pity that the current value is only part of the debug tooltip. In this case it would be handy to have this in normal mode as well.

It is visible without running in developer mode.

I mean the "current value of the CO like "1.5". This is not visible.

Oh, yeah that is a shame. But let's leave that issue for after 2.1.

daschuer · 2017-05-27T22:35:36Z


    EffectManifestParameter* delay = manifest.addParameter();
    delay->setId("delay_time");
-    delay->setName(QObject::tr("Delay"));


Delay is IMHO the correct name for this parameter and should be kept. Time is only the physical quantity, without further meaning.

I think "Delay" is confusing because that often refers to a more complex effect:

The term “echo” was used more often in the early days, and is sometimes used today to refer to the distinct and distant repeats of a signal, while “delay” refers to anything from the same, to the short repeats heard as reverb, to the complex, long, manipulated repeats of an intricate digital delay line.

http://www.gibson.com/News-Lifestyle/Features/en-us/effects-explained-echo-delay.aspx

OK.In this case "Time" is OK.

daschuer · 2017-05-27T22:37:05Z

-    delay->setName(QObject::tr("Delay"));
-    delay->setDescription(QObject::tr("Delay time (seconds)"));
+    delay->setName(QObject::tr("Time"));
+    delay->setDescription(QObject::tr("Delay time\n"


"Delay time", is redundant, because a delay is always a time.

daschuer · 2017-05-27T22:51:14Z

I have just tested this on top of master.
It looks like the echo still saves its buffer over a enabled/disabled cycles. Haven't we fixed this earlier?

daschuer · 2017-05-27T22:54:57Z

-    feedback->setMaximum(1.0);
+    feedback->setMinimum(0.25);
+    feedback->setDefault(0.75);
+    feedback->setMaximum(0.90);


This should stay on 1.0. This allows to cycle a echo until infinity, after we have no input anymore.

I think there is also a use-case for feedback 0.0. this allows to fade the effect out or in from zero.

I think there is also a use-case for feedback 0.0. this allows to fade the effect out or in from zero.

That is what the send knob is for.

This should stay on 1.0. This allows to cycle a echo until infinity, after we have no input anymore.

This is an echo effect, not a looper. The effect should not be able to continue forever without input. I tried setting the maximum feedback to 0.99 and it prevents the output level from continuously escalating and getting out of hand until the deck clips, but the output continues for a very long time and effectively acts as a loop.

Do we have any complains about it? I think not, so this is not a required change for this PR and for me a undesired regression.

Clipping can happen anyway with electronic music and an exact beat loop.
We should consider to replace the clipper with a tanh clipper to make it sound more natural (more analog).
https://github.com/mixxxdj/mixxx/blob/master/src/effects/native/phasereffect.cpp#L147
or better the fast approximation:

mixxx/src/engine/enginefiltermoogladder4.h

Line 238 in 75e85d3

inline float tanh_approx(float input) {

Do we have any complains about it?

I am complaining about it. The effect's buffer clipping isn't the issue; the ease of clipping the deck with feedback at 1, send at 1, and dry/wet at 1 is the issue.

That seems to be a known issue of an echo/delay effect, and sometimes a desired feature.

Each one of these repetitions will gradually fade out as new ones are created, unless you have the feedback cranked up full, in which case the signal will keep compounding until it reaches a deafening squall.

http://www.musicradar.com/tuition/tech/the-ultimate-guide-to-effects-delay-457920

I have just read some stompbox manuals and there is no other feedback range than 0 .. 100 % implemented.

I think we have an other issue here with our gain leveling. If we play a track with RG loudness, the digital level is likely at 50 %, our delay buffer clamper on the other hand, clamps on digital full level, which is most likely much louder than the source signal and will clip the soundcard output with any tiny other signal added.

If we use a tanh clipper this would be better because it kicks in somhow earlier. We can also add a gain to the clipping value to let et clip even earlier and may also make this a parameter which is default to a value that matches the input level.

That seems to be a known issue of an echo/delay effect, and sometimes a desired feature.

Hmm, if it's the standard way I could revert it. Anyone else have an opinion?

I think we have an other issue here with our gain leveling. If we play a track with RG loudness, the digital level is likely at 50 %, our delay buffer clamper on the other hand, clamps on digital full level, which is most likely much louder than the source signal and will clip the soundcard output with any tiny other signal added.
If we use a tanh clipper this would be better because it kicks in somhow earlier. We can also add a gain to the clipping value to let et clip even earlier and may also make this a parameter which is default to a value that matches the input level.

That's an interesting idea. The output of the effect is divided by 2 before mixing with the dry signal. Let's not delay merging this PR for that though. If you have ideas for improving that, let's merge this first, then you can open a new PR for that.

I did a bit more research, and you are right, a restricted range for the feedback parameter is not normal on an echo effect. I have restored it to the 0 - 1 range with 0.75 as default. I came up with an interesting alternate metaknob linking that somewhat approximates a beatmasher effect: link both the Time and Feedback parameters to the metaknob, invert the link for the Time parameter, unlink the Send parameter and turn it all the way up.

daschuer · 2017-05-27T23:21:35Z

It look like there is something wrong with the Echo effect.
Test: play just one beat from a track, using a feedback of one and a delay of 2 beats.
Expected behavior: The beat should loop at halve tracks bpm.
Actual behavior: I hear two beats with different tempos cycling arround.

Does the beat go back to 2 s after the track has stopped? I think this should stick to the current track bpm as it were playing.
In an earlier version, it is possible to change the BPM of the single beat by the delay knob. This is now somehow broken as well.

daschuer · 2017-05-28T00:49:03Z

    return power;
 }

+inline double roundToFraction(double value, int denominator) {


we should round to the nearest value, otherwise all values have a "snaping region", but not the topmost value,
2 in this case, is only adopted on the very maximum.

daschuer · 2017-05-28T14:29:33Z

I think "Delay" is confusing because that often refers to a more complex effect. Ah, yes, I have just checked echo stomp boxes and they use "time" or "echo" for this knob. Since echo also names the effect on some these boxes, "time" is just fine. Sorry for the noise.

Be-ing · 2017-05-28T17:52:43Z

Does the beat go back to 2 s after the track has stopped? I think this should stick to the current track bpm as it were playing.

No, it does not. I tested adding a qDebug() << groupFeatures.beat_length and watching the debugging output. The beat length is still reported to the effect correctly when the deck reaches the end of the track.

Be-ing · 2017-05-28T17:53:38Z

I have just tested this on top of master.

I suggest testing it with #1254. It's a lot of fun :D

It looks like the echo still saves its buffer over a enabled/disabled cycles. Haven't we fixed this earlier?

I cannot reproduce this. I merged the fix in master into this branch.

ronso0 · 2017-09-18T01:11:22Z

Anyone else care to test? @ronso0?

Sure, if I find time to compile this week

Be-ing · 2017-09-18T02:23:18Z

Ardour ships with a set of LV2 plugins including a delay which has the desired effect when adjusting its time parameter. For reference, the code of the effect is here: https://github.com/Ardour/ardour/blob/0b6e2d1e46b9dfe72fe879d2ddddafeafda07023/libs/plugins/a-delay.lv2/a-delay.c#L448

Be-ing · 2017-09-18T20:55:24Z

I copied the algorithm from Ardour's a-Delay plugin. It seems to work, but I don't fully understand all the details. There may still be room for improvement.

Be-ing · 2017-09-18T20:57:16Z

+            }
+
+            gs.wet_sample_left = SampleUtil::clampSample(
+                    (gs.wet_sample_left + gs.delay_buffer[read_position_left]) / 2.0);


a-Delay does some curious crossfading between the samples here using 1 / numSamples, but I don't understand the reasoning behind that.

Without fully understanding the code, that might be for the pitch effect, of e real tape delay if you change the delay time. Without it we might here click sounds.

There are some minor click sounds when changing the Time parameter quickly but I am not sure of their source. I tried the crossfading with 1/numSamples technique that a-Delay uses but it does not solve the issue. The size of the engine's buffer is an artifact of the software's configuration and shouldn't have any impact on the sound of an effect.

Emulating the pitch shifting of an analog delay would be fun, but I don't know how to implement that.

Guitarix has a nice sounding tape delay LV2 plugin but it is written in Faust and does some complicated modeling of tape including wow & flutter. I checked a-Delay again and adjusting its Time parameter does not have the popping sound this does. I'm not sure what is different that is causing the popping here.

daschuer · 2017-09-18T22:09:07Z

+        // period is a number of beats
+        if (m_pTripletParameter->toBool()) {
+           if (period <= 0.125) {
+              period = 0.041666666666666664; // 1/8 / 3


Use her directly "1/8 / 3". The preprocessor is able to calculate that at compile time.

daschuer · 2017-09-18T22:20:23Z

The parameter interface is nice now.
Did you consider to reduce the quantization points on the delay knob?

Be-ing · 2017-09-18T22:21:53Z

Do you mean to round to 1/8 beats or to 1/2 beats?

daschuer · 2017-09-18T22:26:58Z

The range is OK. I originally thought to halve the delay on every step, like we do with the looping controls.
The real issue is, that we have no visual feedback if we run in non developer mode.

Be-ing · 2017-09-18T22:34:45Z

Yes, that is definitely an issue. But solving the visual feedback issue would require quite some more work to make a new skin widget, update the skins, and create a way for the effect to show a nicely formatted value (like 1/4 instead of 0.25223134). IMO that can wait for the following release.

Be-ing · 2017-09-18T22:38:42Z

Although we do not have visual feedback of the current value of parameters, we can at least specify the ranges in the descriptions so the user can have a clue what is going on by reading the tooltip.

Be-ing · 2017-09-19T05:12:53Z

If you would prefer, I can split the last commit off to a new branch so we can merge this now.

daschuer · 2017-09-22T05:56:49Z

Yes good idea.The interface is nice now, so the buffering changes are more easy to review in a separate PR.
Can you fix the looong decimals issues in this PR?

Be-ing · 2017-09-25T02:21:03Z

Ready for merge?

daschuer · 2017-09-25T06:11:38Z

Thank you, this is a real fun effect now.
LGTM.

Be-ing · 2017-09-25T12:34:02Z

Great! I will test the syncing with flanger and phaser again. I am not sure if the Quantize and Triplet parameters make sense for those effects like they do for Echo.

ronso0 · 2017-10-08T13:21:32Z

Is it intended that echo fades out much faster with Triplets enabled?
Would be handy to have the same fade time like without it.

Be-ing · 2017-10-08T14:39:55Z

Yeah, the echo time is divided by 3. If you want to extend the fade out, you could turn up the feedback.

daschuer · 2017-10-08T20:17:43Z

Adopting the normal fade out time for triplets is IMHO a valid request. Unfortunately the feedback gain behaviour might be unexpected in case it is adopted for triplets.
Mmm, what is the most expected behaviour?

sync echo effect to BPM

bfaf108

make Delay knob range from 0 to 2 beats

Be-ing added 2 commits May 10, 2017 15:36

Merge remote-tracking branch 'upstream/master' into tempo_sync_echo

b6e3eeb

rename Echo parameter Delay to Time, use 0-2 sec range without BPM

032f7b6

and make parameter knob center exactly 1 beat / 1 second

Be-ing commented May 11, 2017

View reviewed changes

add sync button to echo effect

c710ac8

In case someone does not want it to synchronize to the tempo

Be-ing mentioned this pull request May 15, 2017

sync flanger period to tempo #1257

Merged

1 task

round synced echo time to 1/4 beats

1c9c5a1

Be-ing force-pushed the tempo_sync_echo branch from e816e4e to 1c9c5a1 Compare May 17, 2017 01:37

daschuer reviewed May 27, 2017

View reviewed changes

daschuer reviewed May 28, 2017

View reviewed changes

make roundToFraction round to nearest instead of clamp down

963c384

Be-ing commented Sep 18, 2017

View reviewed changes

daschuer reviewed Sep 18, 2017

View reviewed changes

Be-ing force-pushed the tempo_sync_echo branch from f01048d to e3f7618 Compare September 18, 2017 22:10

Be-ing added 2 commits September 22, 2017 17:35

Echo: code cleanup

43ad894

Echo: correct parameter descriptions

15334f4

Be-ing force-pushed the tempo_sync_echo branch from e3f7618 to 15334f4 Compare September 22, 2017 22:46

daschuer merged commit 7946a80 into mixxxdj:master Sep 25, 2017

Be-ing deleted the tempo_sync_echo branch September 25, 2017 12:30

This was referenced Sep 26, 2017

Components EffectUnit rework #1211

Merged

Components EffectUnit button parameters #1353

Merged

sohet mentioned this pull request Oct 15, 2017

Flanger #1362

Merged

Be-ing mentioned this pull request Dec 27, 2018

Echo: double the maximum delay to 4 beats, one bar #1928

Closed

This was referenced Aug 22, 2022

BPM Sync for effects #8312

Closed

Trans effect #8870

Open

Uh oh!

Conversation

Be-ing commented May 10, 2017

Uh oh!

ronso0 commented May 10, 2017

Uh oh!

Be-ing commented May 10, 2017

Uh oh!

ronso0 commented May 10, 2017

Uh oh!

Be-ing commented May 10, 2017

Uh oh!

daschuer commented May 10, 2017

Uh oh!

Be-ing commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented May 12, 2017

Uh oh!

ronso0 commented May 12, 2017

Uh oh!

Be-ing commented May 12, 2017

Uh oh!

ronso0 commented May 13, 2017

Uh oh!

Be-ing commented May 26, 2017

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!

daschuer commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

daschuer May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

daschuer commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented May 28, 2017 via email

Uh oh!

Be-ing commented May 28, 2017

Be-ing commented May 10, 2017 •

edited

Loading

Be-ing May 28, 2017 •

edited

Loading

Be-ing May 28, 2017 •

edited

Loading

daschuer May 28, 2017 •

edited

Loading

Be-ing commented May 28, 2017 •

edited

Loading

Be-ing Sep 18, 2017 •

edited

Loading

daschuer commented Sep 22, 2017 •

edited

Loading