Lmms: Control links are lost when duplicating automation clips

[unfa]

When using Copy/Paste or Ctrl+Drag to duplicate an automation clip, the link to a control is lost.

This makes it very tedious to duplicate automation clips, as one needs to restore the links manually for each clip.

LMMS 1.2 RC3 Linux.

[tresf]

+1, hit this all the time. This must be an order of operations problem because once you Ctrl + drag the control back it fixes it but the description copies just fine.

[karmux]

This doesn't always happen with me but from time to time. Right now I'm unable to reproduce.
Also related bug is that pasted automation clip will have duplicate connections when target automation clip already has same connection.

[musikBear]

This i reported earlier, and it is a renewed fail.
In 1.190 This worked
Automation could be copied by

• ctrl-drag
• using context-copy
• copy-tool
  This was iow (re)introduced between 1.1.90 and RC2
  The link was #3359

[musikBear]

Of cause you already knows this, but in case you do not..
The only thing that is missing for the connection to re-established, is the objectID
Here is the xml for only a automation TCO

<automationpattern tens="1" pos="0" prog="2" len="768" mute="0" name="TripleOscillator>Volume">
          <time pos="0" value="100"/>
          <time pos="168" value="165.7"/>
          <time pos="336" value="88.3"/>
          <time pos="456" value="11.7"/>
          <time pos="576" value="115.3"/>
          <object id="31716"/>
        </automationpattern>
        <automationpattern tens="1" pos="1536" prog="2" len="768" mute="0" name="TripleOscillator>Volume">
          <time pos="0" value="100"/>
          <time pos="168" value="165.7"/>
          <time pos="336" value="88.3"/>
          <time pos="456" value="11.7"/>
          <time pos="576" value="115.3"/>

        </automationpattern>

If i simply copy the objectID into the second block, like this

        <automationpattern tens="1" pos="0" prog="2" len="768" mute="0" name="TripleOscillator>Volume">
          <time pos="0" value="100"/>
          <time pos="168" value="165.7"/>
          <time pos="336" value="88.3"/>
          <time pos="456" value="11.7"/>
          <time pos="576" value="115.3"/>
          <object id="31716"/>
        </automationpattern>
        <automationpattern tens="1" pos="1536" prog="2" len="768" mute="0" name="TripleOscillator>Volume">
          <time pos="0" value="100"/>
          <time pos="168" value="165.7"/>
          <time pos="336" value="88.3"/>
          <time pos="456" value="11.7"/>
          <time pos="576" value="115.3"/>
          <object id="31716"/>
        </automationpattern>

The TCO get the curve shape back, and works on the automated control
<object id= /> is for some reason, not copied when a automation TCO is ctrl+drag- copied

[PhysSong]

This is a regression in 1.2, caused by #2875. When I reverted f97b431, the bug was gone.
However, We shouldn't simply revert that commit because it will reintroduce journaling ID collisions. I'm looking for good ideas...

[NETMANSKY]

Well is it fixed?

[PhysSong]

Not yet, but it will be likely to be fixed in RC7.

[PhysSong]

Here are some ideas:

• (temporary) Revert #2875. Fixing a major bug can be preferable to fixing a potential issue.
• (temporary) Use idToSave only if saving projects. The issue described in #2875 remains.
• Decouple automation from journaling. Maybe a master thing, but it will be much better if implemented correctly.

On a side note, #2875 doesn't fix the collision perfectly. For example, you can loading data/presets/Kicker/SnareMarch.xpf or data/presets/TripleOscillator/DetunedGhost.xpf from sidebar.

[zonkmachine]

Something like

• (temporary) Revert #2875. Fixing a major bug can be preferable to fixing a potential issue.

... for rc7 and

• Decouple automation from journaling. Maybe a master thing, but it will be much better if implemented correctly.

... for the final 1.2 release?

[musikBear]

May i ask why the<object id="...."/>is lost from the datastructure?
Is that understood, and not repairable?
-why not reuse of a string concat ... idk.. :| should propl shut up.. 🙊

[PhysSong]

May i ask why the <object id="...."/> is lost from the datastructure?

@musikBear After #2875, new objects and saved objects use different ranges of ID. When saving, new objects' IDs are converted by idToSave function.
When you copy an automation pattern, LMMS saves the pattern's data and reloads it. While the process, the ID conversion occurs and LMMS will try to find objects by wrong IDs. So the connection can't be re-established and the missing block is the consequence of it.

[musikBear]

@PhysSong

" LMMS will try to find objects by wrong IDs

Oh.. i understand. Good explanation 👍

[zonkmachine]

• (temporary) Revert #2875. Fixing a major bug can be preferable to fixing a potential issue.

Yeah, I think this is a good idea. Revert #2875 and remove this issue from RC7. Unless you have a better fix in the works off course.

[PhysSong]

Unless you have a better fix in the works off course.

I have a fix, but I think it needs some changes. I'll remove this from RC7 without doing anything, but keep this in 1.2.0 milestone.

[PhysSong]

How about hacking AutomationPattern::resolveAllIDs? I think we can have a simple fallback logic which tries adding EO_ID_MSB to the ID. It fixes the symptom, but not the cause.
Note that some structural changes are needed to fix the underlying bug completely.

diff --git a/src/core/AutomationPattern.cpp b/src/core/AutomationPattern.cpp
index 25da6defb..c14b83104 100644
--- a/src/core/AutomationPattern.cpp
+++ b/src/core/AutomationPattern.cpp
@@ -787,6 +787,14 @@ void AutomationPattern::resolveAllIDs()
                        {
                            a->addObject( dynamic_cast<AutomatableModel *>( o ), false );
                        }
+                       else
+                       {
+                           o = Engine::projectJournal()->journallingObject( *k + ( 1 << 23 ) );
+                           if( o && dynamic_cast<AutomatableModel *>( o ) )
+                           {
+                               a->addObject( dynamic_cast<AutomatableModel *>( o ), false );
+                           }
+                       }
                    }
                    a->m_idsToResolve.clear();
                    a->dataChanged();

[DomClark]

It fixes the symptom, but not the cause.
Note that some structural changes are needed to fix the underlying bug completely.

I'm not too familiar with the journalling code, so I can't comment on whether it's a good fix or not, but if it works, you're happy with it, and we'd like to minimise the number of structural changes in stable-1.2, then I'd say go for it. If I'm following things correctly, this is one of the main blockers for 1.2, so fixing the just the symptoms would be better than no fix at all. Structural changes can always be done in master.

[PhysSong]

and we'd like to minimise the number of structural changes in stable-1.2

Agreed. Since I don't have a better fix for stable-1.2, I've decided to use the fix above.

[musikBear]

The 'lost connection' is unfortunately also a fact for on/off controllers, and not just for copies, but also after reopen of a saved project.
Recreate :
Automate any FX-chain On/Off
Replay - FX-chain follows automation
Save project
Reload project
Replay -FX-chain will not follow automation
( Redrag of FX-chain On/Off into automation, Restores functionality -But thats rather obvious..

[PhysSong]

@musikBear I think you're talking about https://github.com/LMMS/lmms/issues/4632#issuecomment-427544062 .

[PhysSong]

I didn't explain that in #4632: IDs are saved correctly in automation patterns, but not in connected models(the green LED). If any of them doesn't contain its ID, the automation will be broken.