Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
daschuer wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/effects/builtin/echoeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ EffectManifestPointer EchoEffect::getManifest() {
delay->setShortName(QObject::tr("Time"));
delay->setDescription(QObject::tr(
"Delay time\n"
"1/8 - 2 beats if tempo is detected\n"
"1/8 - 2 seconds if no tempo is detected"));
"1/8 - 4 beats if tempo is detected\n"
"1/8 - 4 seconds if no tempo is detected"));
delay->setControlHint(EffectManifestParameter::ControlHint::KNOB_LINEAR);
delay->setSemanticHint(EffectManifestParameter::SemanticHint::UNKNOWN);
delay->setUnitsHint(EffectManifestParameter::UnitsHint::BEATS);
delay->setMinimum(0.0);
delay->setDefault(0.5);
delay->setMaximum(2.0);
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually become a preference setting. Extending the maximum range would not affect users that prefer to stick with the current, hard-coded default setting of 2 bars.

I mostly use the center position = 1 beat. Setting the knob to exactly 1 beat becomes much harder after this change. Much worse than that: After starting Mixxx the knob is still at the center position, i.e. now resulting in a 2 beat echo. Silently modifying existing settings is not acceptable.

Copy link
Copy Markdown
Contributor

@Be-ing Be-ing Dec 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually become a preference setting.

This will be possible after #1705 is merged with custom per-effect snapshots of parameter values and parameter ranges. Then we can set the absolute minimum and maximum values very wide (as wide as technically possible without breaking the logic of the DSP code) and ship Mixxx with narrowed ranges by default. I do not think we should hack a preference option for this one effect before that more general solution is available.

I mostly use the center position = 1 beat. Setting the knob to exactly 1 beat becomes much harder after this change.

IIRC this is why I chose 2 beats as the range in #1256.

delay->setMaximum(4.0);

EffectManifestParameterPointer feedback = pManifest->addParameter();
feedback->setId("feedback_amount");
Expand Down
4 changes: 2 additions & 2 deletions src/effects/builtin/echoeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@

class EchoGroupState : public EffectState {
public:
// 3 seconds max. This supports the full range of 2 beats for tempos down to
// 6 seconds max. This supports the full range of 4 beats for tempos down to
// 40 BPM.
static constexpr int kMaxDelaySeconds = 3;
static constexpr int kMaxDelaySeconds = 6;

EchoGroupState(const mixxx::EngineParameters bufferParameters)
: EffectState(bufferParameters) {
Expand Down