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.

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

Control links are lost when duplicating automation clips #3781

Closed
unfa opened this issue Aug 27, 2017 · 21 comments · Fixed by #4723
Closed

Control links are lost when duplicating automation clips #3781

unfa opened this issue Aug 27, 2017 · 21 comments · Fixed by #4723
Labels
Milestone

Comments

@unfa
Copy link
Contributor

unfa commented Aug 27, 2017

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
Copy link
Member

tresf commented Aug 27, 2017

+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
Copy link
Contributor

karmux commented Aug 27, 2017

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
Copy link

musikBear commented Aug 28, 2017

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

@musikBear
Copy link

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
Copy link
Member

PhysSong commented Nov 30, 2017

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
Copy link

Well is it fixed?

@PhysSong
Copy link
Member

PhysSong commented Jun 8, 2018

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

@PhysSong
Copy link
Member

Here are some ideas:

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
Copy link
Member

Something like

... 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
Copy link

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
Copy link
Member

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
Copy link

@PhysSong

" LMMS will try to find objects by wrong IDs

Oh.. i understand. Good explanation 👍

@zonkmachine
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

DomClark commented Dec 8, 2018

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
Copy link
Member

PhysSong commented Dec 9, 2018

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.

@zonkmachine zonkmachine moved this from To do to In progress in Release LMMS 1.2.0-RC8 Dec 9, 2018
@musikBear
Copy link

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
Copy link
Member

@musikBear I think you're talking about #4632 (comment) .

@musikBear
Copy link

@musikBear I think you're talking about #4632 (comment) .
:: Whether to enable effect chains

@PhysSong
Oh yes -that is the one
I am next to sure, earlier versions has had on/off automation capabilities, for effect-chain, and its not that it cant be automated, because it can, but reloaded projects have lost this automation, even though the ID for controller is not lost:
´´´´
Its just not reacting (?)

@PhysSong
Copy link
Member

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.

Release LMMS 1.2.0-RC8 automation moved this from In progress to Done Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
8 participants