Effects: store deck & main EQ in effects.xml #11695

ronso0 · 2023-06-30T23:29:00Z

... to have all effects settings in one place.

The migration from mixxx.cfg is supposed to be done only once, hence the read lines are removed from mixxx.cfg. This means if someone decides to go back to 2.3, the deck EQs and the Main EQ will be reset to default. (IIRC downgrading has more implications and we don't guarantee that's trouble-free anyway)

Since cfg strings are removed during migration the only possibility to loose the main EQ config is if Mixxx crashes before/during shutdown (when the cleaned config is written but not the effects config).

Deck EQ effect uids:

 <EqualizerEffects>
  <Id group="[Channel3]">org.mixxx.effects.biquadfullkilleq Built-In</Id>
  <Id group="[Channel2]">---</Id> <!-- exactly as with QuickEffect chains --- clears the EQ slot -->
  <Id group="[Channel1]"></Id>    <!-- while this (or absent Id element) loads the default EQ -->
  <Id group="[Channel4]">org.mixxx.effects.bessel8lvmixeq Built-In</Id>
 </EqualizerEffects>

Main EQ effect uid + parameters (stored like the standard effect chains)

 <MainEQ>
  <EffectChain>
   <Name></Name> <!-- always empty since we just load an effect into the only slot #1 -->
   <MixMode>DRY/WET</MixMode>
   <SuperParameterValue>0</SuperParameterValue>
   <Effects>
    <Effect>
     <MetaParameterValue>0</MetaParameterValue>
     <Id>org.mixxx.effects.parametriceq</Id>
     <BackendType>Built-In</BackendType>
     <Parameters>
      ...
     </Parameters>
    </Effect>
   </Effects>
  </EffectChain>
 </MainEQ>

Preferences > Mixer is adjusted, though there's now one small step back in terms of UX IMO (compared to #11527, not 2.4):
main EQ changes can not be undone with Cancel -- updateMainEQ() always reads the state from EffectsManager. Though that's what the 'Reset Parameter' button is for.

ronso0 · 2023-06-30T23:31:39Z

If there are review ressources for #11527 for merging that into 2.4 it makes absolute sense to have this, too, to complement the effect system changes.

ronso0 · 2023-07-02T15:50:18Z

I have no clue why so many loop / cue / engine buffer tests fail with this branch, I didn't touch related files 🤷

ronso0 · 2023-07-14T00:01:56Z

I have no clue why so many loop / cue / engine buffer tests fail with this branch, I didn't touch related files shrug

I do now: apparently the output effect chain is not created for tests, thus with null check on the output effect chain pointer all tests pass.

ronso0 · 2023-08-14T14:52:54Z

Transition from mixxx.cfg works flawlessly.
Ready for review (last 4 commits), but still draft until #11527 has been merged.

ronso0 · 2023-10-16T22:35:48Z

#11527 has been merged, rebased onto 2.4

daschuer · 2023-10-17T05:10:01Z

Can you remove the unrelated logo update?

ronso0 · 2023-10-17T08:08:17Z

Whoops, cleaned it up locally already but push...
Done.

Swiftb0y

some low-level codestyle comments, didn't manage to analyze the highlevel changes yet.

Swiftb0y · 2023-11-16T15:56:13Z

src/effects/presets/effectchainpresetmanager.cpp

+    EffectManifestPointer pDefaultEqEffect = m_pBackendManager->getManifest(
+            BiquadFullKillEQEffect::getId(), EffectBackendType::BuiltIn);


isn't the default effect defined somewhere else?

it was set in DlgPrefMixer

Swiftb0y · 2023-11-16T15:57:14Z

src/effects/presets/effectchainpresetmanager.cpp

+    VERIFY_OR_DEBUG_ASSERT(!pDefaultEqEffect.isNull()) {
+    }


this is a branch without any statement, should this just be DEBUG_ASSERT or are you missing an early return?

Oh.. Yeah, just DEBUG_ASSERT will do

Swiftb0y · 2023-11-16T16:17:01Z

src/effects/presets/effectchainpresetmanager.cpp

+        mainEqPreset = EffectChainPresetPointer(
+                new EffectChainPreset(mainEqChainElement));


prefer over the same reason to prefer std::make_shared

Suggested change

mainEqPreset = EffectChainPresetPointer(

new EffectChainPreset(mainEqChainElement));

mainEqPreset = EffectChainPresetPointer::create(mainEqChainElement);

src/effects/effectsmanager.cpp

Swiftb0y · 2023-11-16T16:23:16Z

src/effects/presets/effectchainpresetmanager.cpp

+    QDomElement mainEqElement = XmlParse::selectElement(root, EffectXml::kMainEq);
+    QDomNodeList mainEqs = mainEqElement.elementsByTagName(EffectXml::kChain);
+    QDomNode mainEqChainNode = mainEqs.at(0);
+    EffectChainPresetPointer mainEqPreset;


functionally equivalent, but this makes it obvious that the pointer is initialized.

Suggested change

EffectChainPresetPointer mainEqPreset;

EffectChainPresetPointer mainEqPreset = nullptr;

Also, shouldn't this also probably be pDefaultEqEffect instead of nullptr?

the default for Main is none

Swiftb0y · 2023-11-16T16:37:55Z

src/effects/effectsmanager.cpp

+    for (auto it = m_equalizerEffectChains.begin();
+            it != m_equalizerEffectChains.end();
+            it++) {


gotta love Qt's associative containers for the ease-of-use when wanting to iterate over them...

I just copied the style present in this file, could as well be a QHashIterator.

No its fine, I just wish we could use structured-bindings instead, but we can't...
https://stackoverflow.com/questions/13087028/can-i-easily-iterate-over-the-values-of-a-map-using-a-range-based-for-loop

~~I have an Idea on how to improve this though.~~ Nevermind, not much of an improvement.

Okay.
I already switched to QHashIterator, even if it's just about making it a bit more compact.

src/preferences/dialog/dlgprefmixer.cpp

ronso0 · 2023-11-16T23:08:56Z

Thanks @Swiftb0y for taking a look. I'll consider your proposals and check each of the commits to make sure everything is still as intended.

Swiftb0y · 2023-11-16T23:13:23Z

Thank you. I just picked this PR by chance during the little free time I had. So I don't know when I'll be able to finish this review.

ronso0 · 2023-11-16T23:47:53Z

didn't manage to analyze the highlevel changes yet

I simply adopted the routines of the QuickEffect chains for the Equalizers, and those of the standard effect units for the Main effect unit, maybe that helps 🤷‍♂️ Nothing I'm particularly proud of, it just had to be done.

…ects.xml

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

ronso0 · 2023-12-03T22:07:18Z

ping @Swiftb0y do you see a chance to continue with your review?
I'd love to get this into 2.4, IMO it's the final move to finsh the effects refactoring.

daschuer

It works already good. Thank you.
I have added some comment for code improvement

daschuer · 2024-01-12T07:52:47Z

src/effects/effectsmanager.cpp

+    QHashIterator<QString, QuickEffectChainPointer> qeIt(m_quickEffectChains);
+    while (qeIt.hasNext()) {
+        auto pChainSlot = qeIt.next().value();


Suggested change

QHashIterator<QString, QuickEffectChainPointer> qeIt(m_quickEffectChains);

while (qeIt.hasNext()) {

auto pChainSlot = qeIt.next().value();

for (auto pChainSlot : m_quickEffectChains) {

I could have sworn I tried that loop earlier...
Thanks!

daschuer · 2024-01-12T07:55:03Z

src/effects/effectsmanager.cpp

+    QHashIterator<QString, EqualizerEffectChainPointer> eqIt(m_equalizerEffectChains);
+    while (eqIt.hasNext()) {
+        auto pEqChainSlot = eqIt.next().value();


Suggested change

QHashIterator<QString, EqualizerEffectChainPointer> eqIt(m_equalizerEffectChains);

while (eqIt.hasNext()) {

auto pEqChainSlot = eqIt.next().value();

for (auto pEqChainSlot : m_equalizerEffectChains) {

daschuer · 2024-01-12T07:58:53Z

src/effects/effectsmanager.cpp

+    QHashIterator<QString, QuickEffectChainPointer> qeIt(m_quickEffectChains);
+    while (qeIt.hasNext()) {
+        deckStrings << qeIt.next().key();
    }


Suggested change

QHashIterator<QString, QuickEffectChainPointer> qeIt(m_quickEffectChains);

while (qeIt.hasNext()) {

deckStrings << qeIt.next().key();

}

deckStrings = m_quickEffectChains.keys();

daschuer · 2024-01-12T08:29:01Z

src/effects/effectsmanager.cpp

+    QHashIterator<QString, EqualizerEffectChainPointer> eqIt(m_equalizerEffectChains);
+    while (eqIt.hasNext()) {
+        eqIt.next();
+        const auto pEffectSlot = eqIt.value().data()->getEffectSlot(0);


Suggested change

const auto pEffectSlot = eqIt.value().data()->getEffectSlot(0);

const auto pEffectSlot = eqIt.value()->getEffectSlot(0);

daschuer · 2024-01-12T08:31:40Z

src/effects/effectsmanager.cpp

@@ -241,10 +292,24 @@ void EffectsManager::saveEffectsXml() {
            "schemaVersion", QString::number(EffectXml::kXmlSchemaVersion));
    doc.appendChild(rootElement);

+    QHash<QString, EffectManifestPointer> eqEffectManifests;


Suggested change

QHash<QString, EffectManifestPointer> eqEffectManifests;

QHash<QString, EffectManifestPointer> eqEffectManifests;

eqEffectManifests.reserve(m_equalizerEffectChains.size());

daschuer · 2024-01-12T08:33:29Z

src/effects/effectsmanager.cpp

+        auto pManifest = pEffectSlot->getManifest();
+        eqEffectManifests.insert(eqIt.key(), pManifest);
+    }
+
    QHash<QString, EffectChainPresetPointer> quickEffectChainPresets;


Suggested change

QHash<QString, EffectChainPresetPointer> quickEffectChainPresets;

QHash<QString, EffectChainPresetPointer> quickEffectChainPresets;

quickEffectChainPresets.reserve(m_quickEffectChains.size());

daschuer · 2024-01-12T08:43:33Z

src/effects/effectsmanager.cpp

+    QHashIterator<QString, QuickEffectChainPointer> qeIt(m_quickEffectChains);
+    while (qeIt.hasNext()) {
+        qeIt.next();
+        auto pPreset = EffectChainPresetPointer::create(qeIt.value().data());


I stumbled over this because the ownership was not instantly obvious. Maybe a comment helps or:

Suggested change

auto pPreset = EffectChainPresetPointer::create(qeIt.value().data());

auto* pQuickEffectChain = qeIt.value().data();

auto pPreset = EffectChainPresetPointer::create(pQuickEffectChain);

src/effects/presets/effectchainpresetmanager.cpp

daschuer · 2024-01-12T09:00:49Z

src/effects/presets/effectchainpresetmanager.cpp

+    // Read id of last loaded EQ effect
+    QDomElement eqEffectsElement =
+            XmlParse::selectElement(root, EffectXml::kEqualizerEffects);
+    QDomNodeList eqEffectNodeList =


Suggested change

QDomNodeList eqEffectNodeList =

const QDomNodeList eqEffectNodeList =

daschuer · 2024-01-12T09:20:57Z

Did did still consider this +350 -141 PR a 2.4 candidate? I think yes for sake of completeness

ronso0 · 2024-01-12T13:01:48Z

Yes, this should be in 2.4 now that we polished it.

The tricky part is the migration in DlgPrefMixer:

The migration from mixxx.cfg is supposed to be done only once, hence the read lines are removed from mixxx.cfg. This means if someone decides to go back to 2.3, the deck EQs and the Main EQ will be reset to default. (IIRC downgrading has more implications and we don't guarantee that's trouble-free anyway)

Since cfg strings are removed during migration the only possibility to loose the main EQ config is if Mixxx crashes before/during shutdown (when the cleaned config is written but not the effects config).

daschuer · 2024-01-12T16:12:41Z

clazy is complaining:

Error: /home/runner/work/mixxx/mixxx/src/effects/effectsmanager.cpp:177:10: error: Missing reference in range-for with non trivial type (QSharedPointer<EqualizerEffectChain>) [-Wclazy-range-loop-reference]
    for (auto pEqChainSlot : std::as_const(m_equalizerEffectChains)) {
         ^
         const  &
1 error generated.

The rest looks good. Thank you. Please confirm when it is ready for merge.

ronso0 · 2024-01-12T18:11:51Z

Fixed.

daschuer

All Green. Thank you. LGTM.

ronso0 · 2024-01-12T20:01:16Z

Glad this got done, thanks for your reviews @Swiftb0y and @daschuer !!

github-actions bot added the ui label Jun 30, 2023

ronso0 changed the base branch from main to 2.4 June 30, 2023 23:29

ronso0 added effects and removed ui labels Jun 30, 2023

ronso0 added this to the 2.4.0 milestone Jun 30, 2023

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 80cb584 to c17af3d Compare July 1, 2023 11:37

github-actions bot added the ui label Jul 1, 2023

ronso0 mentioned this pull request Jul 1, 2023

Pref > Mixer: apply & save settings only in slotApply(), fix bugs, improve UX #11527

Merged

ronso0 marked this pull request as draft July 2, 2023 15:38

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from c17af3d to 22d5dda Compare July 2, 2023 15:50

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 22d5dda to 786b9df Compare July 12, 2023 21:21

github-actions bot added the code quality label Jul 12, 2023

ronso0 force-pushed the effects-store-eq-in-effects.xml branch 2 times, most recently from c0a46b8 to 20b68e6 Compare July 13, 2023 23:56

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 20b68e6 to c672756 Compare August 13, 2023 23:44

ronso0 added preferences and removed code quality ui labels Aug 14, 2023

ronso0 mentioned this pull request Aug 24, 2023

Library sidebar: allow moving focused index with Ctrl + Up/Down #11262

Closed

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from c672756 to ad73a65 Compare October 16, 2023 22:34

ronso0 marked this pull request as ready for review October 16, 2023 22:35

github-actions bot added the ui label Oct 16, 2023

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from ad73a65 to 5c12ebd Compare October 17, 2023 08:08

Effects: center Super knob when loading empty (QuickEffect) chain preset

3fffc29

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 7b23d47 to 3fffc29 Compare November 15, 2023 15:28

Swiftb0y reviewed Nov 16, 2023

View reviewed changes

ronso0 and others added 4 commits November 17, 2023 14:37

Merge remote-tracking branch 'mixxx/2.4' into effects-store-eq-in-eff…

272452c

…ects.xml

DlgPrefMixer: revert deck label change

0e85fb5

Effects(Preset)Manager: use QHashIterator

c93e5d8

Effects(Preset)Mananger: improve pointer init/creation, add comments

c5c8895

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 1db748e to c5c8895 Compare November 17, 2023 13:39

Effects(Preset)Manager: use 'create' for various Effect...Pointer

4c330a3

ronso0 mentioned this pull request Nov 20, 2023

Effects: center Super knob when loading empty (QuickEffect) chain preset #12320

Merged

JoergAtGithub added the needs review label Dec 16, 2023

daschuer requested changes Jan 12, 2024

View reviewed changes

Effects: more const

892f5e5

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from 7134440 to dfb7c1c Compare January 12, 2024 14:02

ronso0 added 3 commits January 12, 2024 19:09

EffectsManager: explicit interators to range-based loops

d7bab9a

EffectsManager: pre-allocate memory for QLists

03f843b

EffectsManager: clarify ownership/type with explicit assignment

2e9aa1d

ronso0 force-pushed the effects-store-eq-in-effects.xml branch from dfb7c1c to 2e9aa1d Compare January 12, 2024 18:11

daschuer approved these changes Jan 12, 2024

View reviewed changes

daschuer merged commit a4ce3c9 into mixxxdj:2.4 Jan 12, 2024
11 checks passed

ronso0 deleted the effects-store-eq-in-effects.xml branch January 12, 2024 20:00

		EffectManifestPointer pDefaultEqEffect = m_pBackendManager->getManifest(
		BiquadFullKillEQEffect::getId(), EffectBackendType::BuiltIn);

		mainEqPreset = EffectChainPresetPointer(
		new EffectChainPreset(mainEqChainElement));

	mainEqPreset = EffectChainPresetPointer(
	new EffectChainPreset(mainEqChainElement));
	mainEqPreset = EffectChainPresetPointer::create(mainEqChainElement);

	EffectChainPresetPointer mainEqPreset;
	EffectChainPresetPointer mainEqPreset = nullptr;

	const auto pEffectSlot = eqIt.value().data()->getEffectSlot(0);
	const auto pEffectSlot = eqIt.value()->getEffectSlot(0);

	QHash<QString, EffectManifestPointer> eqEffectManifests;
	QHash<QString, EffectManifestPointer> eqEffectManifests;
	eqEffectManifests.reserve(m_equalizerEffectChains.size());

	QHash<QString, EffectChainPresetPointer> quickEffectChainPresets;
	QHash<QString, EffectChainPresetPointer> quickEffectChainPresets;
	quickEffectChainPresets.reserve(m_quickEffectChains.size());

	auto pPreset = EffectChainPresetPointer::create(qeIt.value().data());
	auto* pQuickEffectChain = qeIt.value().data();
	auto pPreset = EffectChainPresetPointer::create(pQuickEffectChain);

	QDomNodeList eqEffectNodeList =
	const QDomNodeList eqEffectNodeList =

Effects: store deck & main EQ in effects.xml #11695

Effects: store deck & main EQ in effects.xml #11695

Conversation

ronso0 commented Jun 30, 2023 • edited Loading

ronso0 commented Jun 30, 2023

ronso0 commented Jul 2, 2023

ronso0 commented Jul 14, 2023

ronso0 commented Aug 14, 2023

ronso0 commented Oct 16, 2023

daschuer commented Oct 17, 2023

ronso0 commented Oct 17, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Nov 16, 2023

Swiftb0y commented Nov 16, 2023

ronso0 commented Nov 16, 2023

ronso0 commented Dec 3, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jan 12, 2024

ronso0 commented Jan 12, 2024

daschuer commented Jan 12, 2024

ronso0 commented Jan 12, 2024

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Jan 12, 2024

ronso0 commented Jun 30, 2023 •

edited

Loading

Swiftb0y Nov 17, 2023 •

edited

Loading