persistent effects by Be-ing · Pull Request #1092 · mixxxdj/mixxx

Be-ing · 2016-12-27T08:29:41Z

Save state of effect chains to effects.xml in user settings directory on shutdown and reload state of effects chains from that file on startup. This paves the way for exporting & importing effect chain presets.

Should the state of effect metaknobs and/or chain superknobs be saved as well?

sblaisot · 2016-12-27T13:51:11Z

Can you please provide the XML schema used? How is it upward and backward compatible between mixxx versions?

Be-ing · 2016-12-27T16:17:18Z

I have not written a formal schema, but here is an example effects.xml file:

<?xml version='1.0' encoding='utf-8'?>
<MixxxEffects>
 <Rack>
  <Group>[EffectRack1]</Group>
  <Chains>
   <EffectChain>
    <Name></Name>
    <Description></Description>
    <InsertionType>0</InsertionType>
    <Effects>
     <Effect>
      <Id>org.mixxx.effects.filter</Id>
      <Version>1.0</Version>
      <Parameters>
       <KnobParameters>
        <KnobParameter>
         <Id>lpf</Id>
         <Value>1</Value>
         <LinkType>2</LinkType>
         <LinkInversion>1</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>q</Id>
         <Value>0.487611</Value>
         <LinkType>0</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>hpf</Id>
         <Value>0.322835</Value>
         <LinkType>3</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
       </KnobParameters>
       <ButtonParameters/>
      </Parameters>
     </Effect>
     <Effect/>
     <Effect>
      <Id>org.mixxx.effects.phaser</Id>
      <Version>1.0</Version>
      <Parameters>
       <KnobParameters>
        <KnobParameter>
         <Id>depth</Id>
         <Value>0</Value>
         <LinkType>1</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>range</Id>
         <Value>0.645669</Value>
         <LinkType>1</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>feedback</Id>
         <Value>0.716535</Value>
         <LinkType>0</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>stages</Id>
         <Value>0.311024</Value>
         <LinkType>0</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>lfo_frequency</Id>
         <Value>0.399213</Value>
         <LinkType>0</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
       </KnobParameters>
       <ButtonParameters>
        <ButtonParameter>
         <Id>stereo</Id>
         <Value>1</Value>
        </ButtonParameter>
       </ButtonParameters>
      </Parameters>
     </Effect>
     <Effect/>
    </Effects>
   </EffectChain>
   <EffectChain>
    <Name></Name>
    <Description></Description>
    <InsertionType>0</InsertionType>
    <Effects>
     <Effect/>
     <Effect>
      <Id>org.mixxx.effects.autopan</Id>
      <Version>1.0</Version>
      <Parameters>
       <KnobParameters>
        <KnobParameter>
         <Id>width</Id>
         <Value>0</Value>
         <LinkType>1</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>period</Id>
         <Value>1</Value>
         <LinkType>1</LinkType>
         <LinkInversion>1</LinkInversion>
        </KnobParameter>
        <KnobParameter>
         <Id>smoothing</Id>
         <Value>0.851646</Value>
         <LinkType>0</LinkType>
         <LinkInversion>0</LinkInversion>
        </KnobParameter>
       </KnobParameters>
       <ButtonParameters>
        <ButtonParameter>
         <Id>periodUnit</Id>
         <Value>1</Value>
        </ButtonParameter>
       </ButtonParameters>
      </Parameters>
     </Effect>
     <Effect/>
     <Effect/>
    </Effects>
   </EffectChain>
   <EffectChain>
    <Name></Name>
    <Description></Description>
    <InsertionType>0</InsertionType>
    <Effects>
     <Effect/>
     <Effect/>
     <Effect/>
     <Effect/>
    </Effects>
   </EffectChain>
   <EffectChain>
    <Name></Name>
    <Description></Description>
    <InsertionType>0</InsertionType>
    <Effects>
     <Effect/>
     <Effect/>
     <Effect/>
     <Effect/>
    </Effects>
   </EffectChain>
  </Chains>
 </Rack>
</MixxxEffects>

For backwards compatibility, if effects.xml is unreadable (or is not valid XML), blank effect chains are loaded like before. Also, if any element is empty or missing, a blank chain or effect or the default state of the parameter is loaded. Empty <Effect/> elements are intentionally used to keep the order of chains. Extra elements are ignored.

For forwards compatibility, it would be a good idea to indicate the Mixxx version with the <MixxxEffects> element, so I'll add that. I talked with @rryan on IRC and we agreed that a good way to handle storage of saved/imported effect chain presets in the future would be a similar XML file. I think it would be good to structure that like the <Chains> element above, with an arbitrary number of <EffectChain> child elements. Those would be saved in a different XML file, so case something goes wrong in effects.xml users won't lose their imported chains. I initially was thinking about saving one effect chain per file and relying on the file name to name them, but @rryan pointed out this would not be a good idea because of incompatibilities between filesystems and their limitations on valid file names. There is already a <Name> child element of <EffectChain>, but there is no UI to edit the names of chains currently this will be empty. If any other effect parameter types are added in the future (for example enum parameters), it would be easy to add another child element of <Parameters>.

Be-ing · 2016-12-28T22:33:12Z

+    } else {
+        m_pControlValue->setParameter(
+            XmlParse::selectNodeDouble(knobParameterElement, "Value"));
+        m_pEffectParameter->setValue(m_pControlValue->get());


I don't understand why this is necessary. m_pControlValue's valueChanged signal is connected to EffectParameterSlot::slotValueChanged in the EffectParameterSlot constructor, so the line above should already do this. Putting a qDebug() at the top of EffectParameterSlot::slotValueChanged shows that it is not triggered when the value is loaded from XML.

setParameter does not send value change event to the owner. Please try setParamerterFrom( .. , nullptr);
I think you have also to deal with soft takeover since controllers will be out of sync:
m_pSoftTakeover->ignore(m_pControlValue, parameter);

I tried m_pControlValue->set() and it had the same issue. I'll try setParameterFrom().

Soft takeover should not be active here. That would interfere with receiving initial positions of knobs from controllers. I have designed the Controls JS library to accommodate that.

setParameterFrom(..., nullptr) does work. I don't understand how. Could you explain? I looked through the ControlObject and ControlDoublePrivate source but it isn't clear to me. I see that ControlDoublePrivate::set() has the line emit(valueChanged(value, pSender));, but I don't understand how that doesn't trigger the connected slot when pSender is the ControlObject as set in ControlObject::setParameter().

daschuer · 2016-12-31T01:27:04Z

Saving the Mixxx version in the xml file will force us to migrate the file on every Mixxx verison change.
It also does not allow us to change the xml schema during development without changing Mixxx version.
I think we should version the schema itself and remove the Mixxx version.

daschuer · 2016-12-31T01:31:21Z

-        return false;
+        EffectChainPointer pEmptyChain;
+        QDomElement emptyChainElement = doc.createElement("EffectChain");
+        for (int i = 0; i < 4; ++i) {


the 4 should be a common definition

I agree. Where?

Is this the same 4 we have in StandardEffectRack::addEffectChainSlot?
Maybe it can become a const member of EffectChainManager,

daschuer · 2016-12-31T01:33:05Z

+        return chainElement;
+    }
+
+    XmlParse::addElement(*doc, chainElement, "Name",


Strings like "Name" should be placed in an anonymous namespace on top of this file.

Anonymous namespaces will not work well for this because these strings are reused in both the Slot (toXML & loadValuesFromXml methods) and non-Slot (createFromXml methods) classes in src/effects. Can you suggest a better place to define the strings?

Than you might use an independent header file that is included by both

daschuer · 2017-01-03T09:19:05Z

    bool saveEffectChains();
    QList<std::pair<EffectChainPointer, QDomElement>> loadEffectChains();

+    static const int kNumEffectsPerUnit = 4;


I am not sure if this works nice on all compilers.
I would prefer to wrap this into a getter and move this constant to the cpp file

daschuer · 2017-01-03T09:21:24Z


+// Version history:
+// 0 (Mixxx 2.1.0): initial support for saving state of effects
+const int EFFECT_XML_VERSION = 0;


kEffectXmlVersion

MIXXX_VERSION is a macro

Be-ing · 2017-01-09T23:48:55Z

@daschuer any more issues?

Be-ing · 2017-01-14T00:55:14Z

Whoops, I forgot to add the new header file in 61669a5. Added it in a new commit.

Be-ing · 2017-01-14T01:00:23Z

Let's not merge this until #1118 is merged to avoid having to increase the effect version numbers.

Be-ing · 2017-01-17T02:14:47Z

Okay, all merge conflicts and build issues fixed. Tests pass again. Ready for review.

Be-ing · 2017-04-15T20:52:06Z

ping @rryan: Is there anything left to do here? Ready for merge?

rryan

I did one more careful review and found a few minor things to fix. LGTM, nice work!

rryan · 2017-04-16T03:34:24Z

+        m_pControlValue->reset();
+    } else {
+        bool conversionWorked = false;
+        double value = XmlParse::selectNodeDouble(parameterElement,


Looks like you're missing a &conversionWorked 3rd parameter here so the value will never be set.

rryan · 2017-04-16T03:45:53Z

    QString itemPrefix = formatItemPrefix(iParameterSlotNumber);
    m_pControlLoaded = new ControlObject(
            ConfigKey(m_group, itemPrefix + QString("_loaded")));
+    m_pControlType = new ControlObject(


I know you just moved this, but I noticed m_pControlType and m_pControlLoaded are not deleted in the constructor. Could you add a delete please?

They are deleted in the parent EffectParameterSlotBase destructor. Deleting them here causes a segfault. I'll add a comment.

Oh oops, that's not confusing at all :) -- thanks.

rryan · 2017-04-16T03:55:06Z

            addEffectButtonParameterSlot();
        }

+        m_parametersById.clear();


If loadEffect is called with a null EffectPointer, we won't clear m_parametersById. Should this be at the top of the function?

rryan · 2017-04-16T04:01:17Z

+
+    QDomElement parametersElement = doc->createElement(EffectXml::ParametersRoot);
+
+    for (const auto& pParameter : m_parametersById) {


Since m_parametersById is only used in serializing/deserializing, I wonder if it makes sense to build it on the fly instead of storing at the class level (which requires keeping it in sync as new effects are loaded).

Also, why a QMap? did you want the items to be sorted by ID in the XML (instead of being in the parameter order in the manifest)?

Since m_parametersById is only used in serializing/deserializing, I wonder if it makes sense to build it on the fly instead of storing at the class level (which requires keeping it in sync as new effects are loaded).

EDIT: You're right, that does make sense.

Also, why a QMap? did you want the items to be sorted by ID in the XML (instead of being in the parameter order in the manifest)?

I used a QMap to pair the EffectParameterSlotBasePointers with the corresponding manifests' ID (which is a QString). EffectSlot::loadEffectSlotFromXml needs to lookup an EffectParameterSlotBasePointer from the ID string in the XML. It would be a good idea to serialize them in order when writing the XML though. Then when support for users rearranging parameters is implemented that could be represented in the XML file with the order of the <Parameter> elements.

Hmmm... I'll refactor this a bit to preserve parameter order.

rryan · 2017-04-16T04:10:30Z

+}
+
+void EffectSlot::loadEffectSlotFromXml(const QDomElement& effectElement) {
+    if (effectElement.text().isEmpty()) {


QDomElement::text() requires recursively building up a string of all the children's .text(). I think QDomNode::hasChildNodes does what you want here but will be far cheaper?

https://code.qt.io/cgit/qt/qt.git/tree/src/xml/dom/qdom.cpp#n4565

rryan · 2017-04-16T04:17:22Z

+}
+
+void EffectChainSlot::loadChainSlotFromXml(const QDomElement& effectChainElement) {
+    if (effectChainElement.text().isEmpty()) {


!element.hasChildNodes() instead?

(see my other comment)

rryan · 2017-04-16T04:17:27Z

+    if (m_pEffectParameter == nullptr) {
+        return;
+    }
+    if (parameterElement.text().isEmpty()) {


!element.hasChildNodes() instead?

(see my other comment)

rryan · 2017-04-16T04:17:38Z

+
+    QDomElement parametersElement = XmlParse::selectElement(effectElement,
+                                                            EffectXml::ParametersRoot);
+    if (parametersElement.text().isEmpty()) {


!element.hasChildNodes() instead?

(see my other comment)

rryan · 2017-04-16T04:32:57Z

+    if (effectElement.text().isEmpty()) {
+        return;
+    }
+


Since loading the effect chain into the effect chain slot and loading the chain slot's XML config happen separately, want to add a sanity check here that the XML effect ID matches the loaded effect ID and print a warning if it doesn't?

rryan · 2017-04-16T04:37:21Z

            addEffectButtonParameterSlot();
        }

+        m_parametersById.clear();


Please clear m_parametersById in EffectSlot::clear() as well

QDomNode::text recursively gets the text for each child node

In the future, it should be possible to rearrange effect parameters and store that in XML with the order of the <Parameter> elements.

rryan · 2017-04-18T16:07:25Z

Just did a final test and everything seems to work well. For fun, I changed Flanger's effect ID to the unicode poop symbol and I couldn't get it to load in an effect unit so I didn't get to test whether the UTF-8 encoded poop actually ended up in the XML file :P. I settled for adding accented characters to the EffectChain's description and verifying they were preserved across restarts. LGTM, thanks!

Be-ing · 2017-04-18T16:24:57Z

Thanks for review and testing! 2.1 will have a much better user experience for effects. :)

unicode poop symbol

I did not know such a thing existed. o.O

daschuer · 2017-04-18T22:03:14Z

In current Master, I cannot use prev_effect or prev_chain in Shade / LateNight.
Is this due to this PR?

daschuer · 2017-04-18T22:07:41Z

-    pStandardRack->addEffectChainSlot();
-    pStandardRack->addEffectChainSlot();
-    pStandardRack->addEffectChainSlot();
-    pStandardRack->addEffectChainSlot();


I think the regression is caused by the missing empty Slots in case of no stored effect are available.

daschuer · 2017-04-18T22:08:54Z

-    pStandardRack->addEffectChainSlot();
-    pStandardRack->addEffectChainSlot();
+    for (auto chain : savedChains) {
+        pStandardRack->addEffectChainSlot(chain.first, chain.second);


Please use here named members instead pointless "first" and "second"

This is used only once. I think creating a struct just for this one use would be more trouble than its worth.

It already causes trouble and it tends to spread out later.

Be-ing · 2017-04-18T22:18:33Z

In current Master, I cannot use prev_effect or prev_chain in Shade / LateNight.

next_chain is working. It looks broken at first because there are 4 chains created that do not have any effects loaded and LateNight has no way to load different effects in a chain yet. Something is wrong with prev_chain though. prev_chain works for the chains made with the premade chain hack, which should be removed as soon as Shade and LateNight are updated, but it isn't working for the apparently blank chains.

next/prev_effect are working fine in Shade.

daschuer · 2017-04-18T22:18:50Z

It looks like we have a conceptual issue here.

The content of the xml file also creates the COs of the effects. This breaks the original concept, that the COs are independent from the loaded effect chains.

Mixxx, or the loaded skin should set up the Effect slots and the stored chains from the XML is just responsible for the filling, loading the stored effects.
If there are ore or less stored effects in the XML file, it should be handled gracefully.

daschuer · 2017-04-18T22:20:31Z

next/prev_effect are working fine in Shade.

No, sorry. Not if there is no XML file.

Be-ing · 2017-04-18T22:24:33Z

No, sorry. Not if there is no XML file.

I cannot reproduce this.

daschuer · 2017-04-18T22:29:04Z

   for (auto chain : savedChains) {
        pStandardRack->addEffectChainSlot(chain.first, chain.second);
    }

Is skipped if there is no saved chain. And this instantiates and connects the prev_effect and next_effect COs. Without a connection nothing happens.

But I think that the COs are not connected before is the real issue.

Be-ing · 2017-04-18T22:33:59Z

Is skipped if there is no saved chain.

No it is not. EffectChainManager::loadEffectChains will return 4 empty EffectChainPointer, QDomElement pairs if the file cannot be read or parsed.

daschuer · 2017-04-18T22:46:53Z

For some reason I have found this file in my .mixxx folder

<?xml version='1.0' encoding='utf-8'?>
<MixxxEffects schemaVersion="0">
 <Rack>
  <Group>[EffectRack1]</Group>
  <Chains/>
 </Rack>
</MixxxEffects>

If I delete it, the issue is gone. What could be the reason for getting it?

daschuer · 2017-04-18T22:49:36Z

EffectChainManager::loadEffectChains will return 4 empty EffectChainPointer, QDomElement pairs if the file cannot be read or parsed.

IMHO this should not be in responds to loadEffectChains. Mixxx should setup the Chains before and load what is in the file. This would make the feature more robust against user edits.

Be-ing · 2017-04-19T01:33:01Z

If I delete it, the issue is gone. What could be the reason for getting it?

That was probably from testing an early version of this PR. Users should never have such a file.

The feature could be more robust against unusual XML files by ensuring that EffectChainManager::loadEffectChain always returns 4 EffectChainPointer, QDomElement pairs. The XML file you posted is a valid XML file, so it looked for effect chains to load, but ended up returning none (which is not the same as returning null EffectChainPointers).

Be-ing force-pushed the effect_xml branch from f15fa51 to 831b125 Compare December 27, 2016 09:04

Be-ing force-pushed the effect_xml branch 2 times, most recently from 4ba4cd9 to 3eefcd1 Compare December 27, 2016 19:35

Be-ing commented Dec 28, 2016

View reviewed changes

daschuer reviewed Dec 31, 2016

View reviewed changes

This was referenced Jan 2, 2017

Effect enable switches changes #1103

Merged

add per-effect meta knobs #1062

Merged

daschuer reviewed Jan 3, 2017

View reviewed changes

Be-ing force-pushed the effect_xml branch from 1a28e49 to 3edfe64 Compare January 7, 2017 22:10

rryan mentioned this pull request Jan 10, 2017

Fix the reverb effect #1113

Merged

Be-ing added 3 commits January 19, 2017 14:39

implement effect saving to/loading from XML

d7b45ea

save/load effect parameter state to XML on shutdown/startup

d826eca

add Mixxx version to effects.xml

50cc903

Be-ing force-pushed the effect_xml branch 3 times, most recently from 8499c99 to 7c3038e Compare January 19, 2017 23:53

Be-ing added 5 commits January 19, 2017 18:04

fix effects tests

3257996

make effect parameters loaded from XML actually change the sound

49c3d74

organize EffectParameterSlot contructor

211e875

small code cleanup

b7bfd32

define a constant for the number of effects

80a0e96

rryan approved these changes Apr 16, 2017

View reviewed changes

Be-ing added 6 commits April 17, 2017 22:54

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

6fe1268

add missing function parameter

94556ff

clarifying comment

7cd7a67

organize EffectParameterSlot constructor & destructor

5ee27df

efficiently check for empty XML elements

d132b18

QDomNode::text recursively gets the text for each child node

keep effect parameters in order in XML file

841d5ab

In the future, it should be possible to rearrange effect parameters and store that in XML with the order of the <Parameter> elements.

Be-ing mentioned this pull request Apr 18, 2017

Lv2 support2 #1240

Merged

rryan merged commit bf5e71f into mixxxdj:master Apr 18, 2017

Be-ing deleted the effect_xml branch April 18, 2017 16:17

daschuer reviewed Apr 18, 2017

View reviewed changes

Be-ing mentioned this pull request Apr 19, 2017

make sure effect chains are initialized properly #1241

Merged

Be-ing mentioned this pull request May 7, 2020

effect chain preset saving/loading, custom effect defaults, and parameter hiding/rearrangement #2618

Closed

15 tasks

This was referenced Aug 23, 2022

crackle when loading Linkwitz-Riley EQ #8714

Closed

Changing effect chain makes mixxx crash in branch master #8752

Closed

decouple effect slot setup from loading the defaults form XML #8851

Closed


		QDomElement parametersElement = doc->createElement(EffectXml::ParametersRoot);

		for (const auto& pParameter : m_parametersById) {

Uh oh!

Conversation

Be-ing commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sblaisot commented Dec 27, 2016

Uh oh!

Be-ing commented Dec 27, 2016

Uh oh!

Be-ing Dec 28, 2016 • 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!

Be-ing Dec 31, 2016 • 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 Dec 31, 2016 • 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!

daschuer commented Dec 31, 2016

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!

Be-ing Jan 3, 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!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Jan 9, 2017

Uh oh!

Be-ing commented Jan 14, 2017

Uh oh!

Be-ing commented Jan 14, 2017

Uh oh!

Be-ing commented Jan 17, 2017

Uh oh!

Be-ing commented Apr 15, 2017

Uh oh!

rryan left a comment

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!

rryan Apr 16, 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 Apr 18, 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!

Choose a reason for hiding this comment

Uh oh!

Be-ing commented Dec 27, 2016 •

edited

Loading

Be-ing Dec 28, 2016 •

edited

Loading

Be-ing Dec 31, 2016 •

edited

Loading

Be-ing Dec 31, 2016 •

edited

Loading

Be-ing Jan 3, 2017 •

edited

Loading

rryan Apr 16, 2017 •

edited

Loading

Be-ing Apr 18, 2017 •

edited

Loading

daschuer commented Apr 18, 2017 •

edited

Loading