Skip to content
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

Effects refactoring: loaded effect #4426

Merged
merged 8 commits into from
Oct 18, 2021
Merged

Effects refactoring: loaded effect #4426

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5c79125
Rename loaded_effect related variables to match the CO name
daschuer Oct 15, 2021
890bd1b
Remove redundant check in loadEffectWithDefaults()
daschuer Oct 15, 2021
8c77516
Do not write m_pManifest before we are sure it is actually loaded.
daschuer Oct 15, 2021
272ad22
Remove redundant copy of default constructed EffectParameterPreset
daschuer Oct 15, 2021
32e7c40
Exit effect parameter presets loop once we have a match to save some …
daschuer Oct 15, 2021
c0b1864
Added missing 0 set of loaded_effect when it has been unloaded
daschuer Oct 15, 2021
f488375
Assert that m_pManifest is null after unloadEffect()
daschuer Oct 18, 2021
b7dfb7a
Assert for pManifest and pEffectPreset both set or null
daschuer Oct 18, 2021
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
27 changes: 13 additions & 14 deletions src/effects/effectslot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,11 @@ EffectSlot::EffectSlot(const QString& group,
this,
&EffectSlot::slotPrevEffect);

m_pControlLoadEffectAtListIndex = std::make_unique<ControlObject>(
m_pControlLoadedEffect = std::make_unique<ControlObject>(
ConfigKey(m_group, "loaded_effect"));
m_pControlLoadEffectAtListIndex->connectValueChangeRequest(this,
&EffectSlot::slotLoadEffectAtListIndexRequest);
m_pControlLoadedEffect->connectValueChangeRequest(
this,
&EffectSlot::slotLoadedEffectRequest);

connect(m_pVisibleEffects.get(),
&VisibleEffectsList::visibleEffectsListChanged,
Expand Down Expand Up @@ -267,10 +268,6 @@ void EffectSlot::loadEffectFromPreset(const EffectPresetPointer pPreset) {
}

void EffectSlot::loadEffectWithDefaults(const EffectManifestPointer pManifest) {
if (!pManifest) {
loadEffectInner(nullptr, nullptr, false);
return;
}
EffectPresetPointer pPreset = m_pPresetManager->getDefaultPreset(pManifest);
loadEffectInner(pManifest, pPreset, false);
}
Expand All @@ -287,29 +284,30 @@ void EffectSlot::loadEffectInner(const EffectManifestPointer pManifest,
}
unloadEffect();

m_pManifest = pManifest;

if (!pManifest || !pEffectPreset) {
// No new effect to load; just unload the old effect and return.
emit effectChanged();
return;
}

m_pManifest = pManifest;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong. m_pManifest being nullptr indicates that the EffectSlot is empty, which needs to happen before this function returns.

Copy link
Member Author

Choose a reason for hiding this comment

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

m_pManifest is always cleared at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, if loading nullptr this function would return before setting m_pManifest to nullptr.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is already cleared via unloadEffect();

Copy link
Contributor

@Be-ing Be-ing Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, but that's not obvious. There's no benefit to this change; it only makes the code harder to read. Rebase to remove this commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then a DEBUG_ASSERT must be added instead of implicitly assuming unloadEffect does what the caller expects.

Copy link
Member

Choose a reason for hiding this comment

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

Daniel just pointed the actual bug out that I didn't even spot after telling me there is one. The problem occurs when calling loadEffectInner with pManifest being valid but pEffectPreset being nullptr. The slot would result in being unloaded, but having a valid m_pManifest. From what I can tell, this violates an invariant.
The proposed change would eliminate the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. The DEBUG_ASSERT is still needed.

Copy link
Member

Choose a reason for hiding this comment

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

So have we found the consensus of sticking with the reordering but adding a debug assert between unloadEffect and return?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think a debug assertion should be added checking that pEffectPreset is not nullptr when pManifest is valid.

addToEngine();

// Create EffectParameters. Every parameter listed in the manifest must have
// an EffectParameter created, regardless of whether it is loaded in a slot.
for (const auto& pManifestParameter : m_pManifest->parameters()) {
// match the manifest parameter to the preset parameter
EffectParameterPreset parameterPreset = EffectParameterPreset();
EffectParameterPreset parameterPreset;
if (pEffectPreset) {
for (const auto& p : pEffectPreset->getParameterPresets()) {
if (p.id() == pManifestParameter->id()) {
parameterPreset = p;
break;
}
}
}
EffectParameterPointer pParameter(new EffectParameter(m_pEngineEffect,
EffectParameterPointer pParameter(new EffectParameter(
m_pEngineEffect,
m_pMessenger,
pManifestParameter,
parameterPreset));
Expand Down Expand Up @@ -362,7 +360,7 @@ void EffectSlot::loadEffectInner(const EffectManifestPointer pManifest,
}

// ControlObjects are 1-indexed
m_pControlLoadEffectAtListIndex->setAndConfirm(m_pVisibleEffects->indexOf(pManifest) + 1);
m_pControlLoadedEffect->setAndConfirm(m_pVisibleEffects->indexOf(pManifest) + 1);

emit effectChanged();
updateEngineState();
Expand All @@ -374,6 +372,7 @@ void EffectSlot::unloadEffect() {
}

m_pControlLoaded->forceSet(0.0);
m_pControlLoadedEffect->setAndConfirm(0.0);
for (const auto& pControlNumParameters : std::as_const(m_pControlNumParameters)) {
pControlNumParameters->forceSet(0.0);
}
Expand Down Expand Up @@ -503,7 +502,7 @@ void EffectSlot::slotNextEffect(double v) {
}
}

void EffectSlot::slotLoadEffectAtListIndexRequest(double value) {
void EffectSlot::slotLoadedEffectRequest(double value) {
// ControlObjects are 1-indexed
int index = static_cast<int>(value) - 1;
if (index < 0 || index >= m_pVisibleEffects->getList().size()) {
Expand All @@ -516,7 +515,7 @@ void EffectSlot::slotLoadEffectAtListIndexRequest(double value) {
void EffectSlot::visibleEffectsListChanged() {
if (isLoaded()) {
// ControlObjects are 1-indexed
m_pControlLoadEffectAtListIndex->setAndConfirm(
m_pControlLoadedEffect->setAndConfirm(
m_pVisibleEffects->indexOf(m_pManifest) + 1);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectslot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class EffectSlot : public QObject {

void slotNextEffect(double v);
void slotPrevEffect(double v);
void slotLoadEffectAtListIndexRequest(double value);
void slotLoadedEffectRequest(double value);
void slotClear(double v);
void slotEffectSelector(double v);
void slotEffectMetaParameter(double v, bool force = false);
Expand Down Expand Up @@ -190,7 +190,7 @@ class EffectSlot : public QObject {
std::unique_ptr<ControlPushButton> m_pControlEnabled;
std::unique_ptr<ControlObject> m_pControlNextEffect;
std::unique_ptr<ControlObject> m_pControlPrevEffect;
std::unique_ptr<ControlObject> m_pControlLoadEffectAtListIndex;
std::unique_ptr<ControlObject> m_pControlLoadedEffect;
std::unique_ptr<ControlEncoder> m_pControlEffectSelector;
std::unique_ptr<ControlObject> m_pControlClear;
std::unique_ptr<ControlPotmeter> m_pControlMetaParameter;
Expand Down