Automation clips smaller than one bar resize to a bar when the automation nodes are changed

bratpeki commented 1 month ago

System Information

Linux

LMMS Version(s)

Master

Most Recent Working Version

No response

Bug Summary

This happens when the nodes are not outside the size of the automation clip (eg. add a note to a quarter, the clip is a half long, it will resize to a bar).

Expected Behaviour

Stays the same size, unless you create a node outside the clip length.

Steps To Reproduce

Make a clip, shrink it, add a node anywhere in that space.

Logs

No response

Screenshots / Minimum Reproducible Project

No response

Please search the issue tracker for existing bug reports before submitting your own.

[X] I have searched all existing issues and confirmed that this is not a duplicate.

regulus79 commented 1 month ago

Interesting, I can replicate the issue but it only happens when I place the first node; if I then resize the clip back to smaller than 1 bar, any subsequent nodes placed behave correctly.

regulus79 commented 4 weeks ago

Right, so I've found a fix by commenting out this line: https://github.com/LMMS/lmms/blob/ff8c47062f4480a6252b09cd4b1075780d5e5678/src/core/AutomationClip.cpp#L356

When you add an automation node, it calls removeNode() before it adds the node so that it can clear whatever was there before. And since removeNode() calls updateLength(), when you add the very first node, it deletes the node before adding it(?) which means that the clip is empty, which means that updateLength() defaults to 1 bar, which causes the bug.

It.... seems that there was a reason for this, given that it was added/moved/kept in #5469? I personally don't understand why calling updateLength() is necessary when removing a node, since the clip can't ever shrink. updateLength() always returns the max of the current length and the last node position, so afaik it should be impossible to change the length of the clip by removing a node.

bratpeki commented 4 weeks ago

Wonderful, @regulus79! I'll investigate this further. In the meantime, maybe we can ask @michaelgregorius to take a look as well.

regulus79 commented 4 weeks ago

I did a bit of testing, and it seems that the fix I gave doesn't work when the node is placed at the exact start of the clip. Interestingly, the code in timeMapLength() which checks if the node is at 0 has a comment before it which says that the check is to prevent the clip from disappearing, so that code might be important.

michaelgregorius commented 4 weeks ago

I can reproduce the problem:

LMMS-ResizingAutomationClip.webm

The main problem seems to be that calling AutomationClip::removeNode does more than it should. It does not simply update the data structure, it also always updates the clip length which is a completely different thing. Both things should not be mixed.

I also wonder if the current data handling is another problem. If I understand correctly adding a new point is interpreted like dragging a point which in turn seems to be implemented as removing a node at the previous position and adding a new node at the new position. This then leads to the remove code being triggered with an update in the first place..

I see the following problems here:

Dragging a node does not manipulate an existing node, i.e. change its properties and keep it, but instead nodes are removed and added over and over (?). So it's not like I drag one instance that has a lifetime until it is explicitly deleted by the user.
Why should the update of a data structure in AutomationClip::removeNode decide that the length must also be updated? This is something that should be done in two separate calls. That way clients could decide if they want to implement the behavior of a clip length update after the removal of a node or not. With the current implementation this is always done which leads to the bug.

michaelgregorius commented 4 weeks ago

I have implemented the separation of concerns described in my last comment with pull request https://github.com/LMMS/lmms/pull/7472 which should fix this issue. Feel free to review it.

LMMS / lmms