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 manifest defaults #10837

Merged
merged 3 commits into from
Mar 29, 2023
Merged

Effects manifest defaults #10837

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2920aa7
feature(effects): EffectPreset::updateParametersFrom(EffectPreset&)
Swiftb0y Aug 29, 2022
1f46f97
fix(effects): Fix parameters of effect not showing up when not in def…
Swiftb0y Aug 29, 2022
41d03d2
refactor: use temporary QHash to reduce algorithmic complexity
Swiftb0y Mar 13, 2023
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
19 changes: 18 additions & 1 deletion src/effects/presets/effectpreset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,5 +99,22 @@ const QDomElement EffectPreset::toXml(QDomDocument* doc) const {
return effectElement;
}

EffectPreset::~EffectPreset() {
void EffectPreset::updateParametersFrom(const EffectPreset& other) {
DEBUG_ASSERT(backendType() == other.backendType());

// technically algorithmically inefficient solution O(n²). May be
// optimizable by sorting first, gains depend on parameter count
Copy link
Member

@daschuer daschuer Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the best way would be to make m_effectParameterPresets a QHash.
Than the std::find_if becomes a fast:
auto currentParameterIt = m_effectParameterPresets.find(parameterToCopy.id());

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you clarify? Do you mean that we change m_effectParameterPresets to be a QHash instead of a QList? That won't work because we need to preserve the ordering of m_effectParameterPresets. What would work though is if we just built a temporary QHash on each call. What do you think?

for (const auto& parameterToCopy : other.m_effectParameterPresets) {
auto currentParameterIt =
std::find_if(m_effectParameterPresets.begin(),
m_effectParameterPresets.end(),
[&](const auto& ourParameter) {
return ourParameter.id() == parameterToCopy.id();
});
if (currentParameterIt == m_effectParameterPresets.end()) {
continue;
}
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = parameterToCopy;
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I just realized, does this create a dangling reference? How do I force a copy here so m_effectParameterPresets takes ownership?
Does this suffice?

Suggested change
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = parameterToCopy;
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = EffectParameterPreset(parameterToCopy);

Copy link
Member

Choose a reason for hiding this comment

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

I think the proposed version is redundant because the copy constructor is already used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking more about it, I'm pretty sure its the copy-assignment operator, so we should be good then.

}
}
10 changes: 9 additions & 1 deletion src/effects/presets/effectpreset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class EffectPreset {
EffectPreset(const QDomElement& element);
EffectPreset(const EffectSlotPointer pEffectSlot);
EffectPreset(const EffectManifestPointer pManifest);
~EffectPreset();

const QDomElement toXml(QDomDocument* doc) const;

Expand All @@ -39,6 +38,15 @@ class EffectPreset {
return m_effectParameterPresets;
}

/// updates all of the parameters of `this` with the parameters
/// of `preset`.
/// The operation is not symmetric:
/// Parameters which are present on `preset` but not on `this` will
/// not be added to `this`
/// Parameters present on `this` but not `preset` will keep their previous
/// settings
void updateParametersFrom(const EffectPreset& preset);

private:
QString m_id;
EffectBackendType m_backendType;
Expand Down
8 changes: 5 additions & 3 deletions src/effects/presets/effectpresetmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ void EffectPresetManager::loadDefaultEffectPresets() {
file.close();
continue;
}
EffectPresetPointer pEffectPreset(new EffectPreset(doc.documentElement()));
if (!pEffectPreset->isEmpty()) {
auto presetFromFile = EffectPreset(doc.documentElement());
if (!presetFromFile.isEmpty()) {
EffectManifestPointer pManifest = m_pBackendManager->getManifest(
pEffectPreset->id(), pEffectPreset->backendType());
presetFromFile.id(), presetFromFile.backendType());
if (pManifest) {
auto pEffectPreset = EffectPresetPointer(new EffectPreset(pManifest));
pEffectPreset->updateParametersFrom(presetFromFile);
m_defaultPresets.insert(pManifest, pEffectPreset);
}
}
Expand Down