Changes AutomationPattern to use nodes instead of raw float values #5712
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces the use of AutomationNodes instead of just floats for keeping the automation values. The AutomationNodes will give more flexibility when it comes to storing extra information about the values (and make it possible to have multiple progression types in the future). Now the tangents are stored on the node, requiring one less timeMap on the automation pattern. Some methods had to be changed for that, since before they used const_iterator, which wouldn't allow us to change the tangent values. TODO: - Check TODO comments that were added in places of the code I had some doubts about. - Maybe change the timeMap to hold pointers to AutomationNodes and not actual instances. - In some pieces of the code, I check if the AutomationNode already exists before setting its value or creating another node. I think that's unnecessary since if I create another node and assign it to the current existing one it could just clone its value. (That would not be valid for pointers to AutomationNodes, so that's something to consider when deciding between the two). - I still didn't find a good solution for renaming QMap::iterator method from "value()" to something else, so now we have lines that look a bit odd like "it.value().getValue()", because value() is actually returning the node, and getValue() is getting the node's automation value.
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12332-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bgf26435b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12332?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://12333-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bgf26435b21-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12333?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12335-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bgf26435b21-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12335?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/jf8dv5fe8nywcliy/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37526603"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/9t06t01pffd9vx89/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37526603"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12334-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.129%2Bgf26435b21-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12334?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "5a66667b70894852112327a3cbde66bec6ab1ed1"} |
Veratil
reviewed
Oct 13, 2020
Spekular
reviewed
Oct 13, 2020
This commit is a big change in comparison to the previous one. Automation nodes now have a inValue and outValue instead of a single value. The inValue represents the core value of the node, which is used for incoming progressions. The outValue represents the value of the node for progressions starting from that node on. In practice, the true value of the node is the inValue and outValue represents a way of creating discrete jumps in a node's position. By default inValue and outValue are the same. The user will then be allowed to change the outValue to create those discrete jumps. Because their values might be different, we now need two tangents for a single node: One for the curve coming before the node and one for the curve coming after the node. So instead of a single tangent variable, we now have inTangent and outTangent. If inValue and outValue are the same, so are inTangent and outTangent, but if they are different both are calculated according to the curve before and after the node. On the Automation Editor, the inValue of the node is represented by our default blue circle, while the outValue is represented by a red circle with 80% alpha (we should add a variable to the Automation Editor class to hold the color of this second circle in the future). Lots of comments were added on the modified files to explain the existing methods where changes were required (not only explaining the logic of the methods but the reason behind using inValue or outValue on them). Lots of TODO comments were also added as placeholders for changes that could or should be done before the finishing of this PR (or after it). For now only the logic for the nodes was added, but there's still no way for the user to change outValues (on the next commit a small placeholder shortcut will be added to do that for testing purposes). The drawing of the notes detuning on the piano roll was updated to account for the discrete jumps. The drawing of the pattern TCO was updated to account for the discrete jumps. The drawing of the pattern on the AutomationEditor was updated to account for the discrete jumps. IMPORTANT: - There were changes to the loadSettings and saveSettings of AutomationPatterns, to account for inValues and outValues, but I didn't create an upgrade routine yet. Some behavior that is important to mention: - Most operations on nodes (drawing, moving, X/Y flipping, and even selection copy/paste, apparently not fully finished) ignore the outValue, basically reseting it to the inValue. So when an user moves a node with a discrete jump, for example, that discrete jump will be lost and the user will need to set it again. Obviously in the future we might want to keep that information, but that isn't a critical issue, just a behavior that can be improved later by upgrading the code. - Later on we might want to connect a signal to the AutomationNode class, so it calls generateTangents when node data is changed. - When an object is disconnected from an automation pattern and it has to rescale the values so they fit the new range of values (max and min) the outValue is reset, meaning all discrete jumps are lost. This behavior will be changed in the future. Things that I think are also important noting: - Mainly in the src/gui/editor/AutomationEditor.cpp file there were lots of codes that apparently are related to a feature that is not yet finished (moving/cutting/copying/pasting selections of automation values). This doesn't sound good unless it's currently being worked on. I tried my best to update the current code to comply to the use of AutomationNodes so their developing can be picked up from a unbroken state. As with other operations involving AutomationNodes, they only account for the inValue discarding any discrete jumps that were present. - I added comments on some logic that seemed flawed in the src/gui/editor/AutomationEditor.cpp file so it can be reviewed. It's beyond the scope of this PR, but since I had to read and change a lot on that file I thought it was pertinent to at least comment those observations.
|
Updated main comment of this draft PR to explain the latest commit. I pushed the changes since most of the core changes were done, in case the reviewers want to take a peak at the code as it is now. Be aware it's not entirely finished though, it's being shared just for the sake of keeping everyone up to date to the work. I'll soon push a new commit with a way to change |
IanCaio
commented
Oct 15, 2020
While implementing the automation nodes, I noticed AutomatinEditor.cpp had some issues regarding flawed logic, code style convention, code that could comply to DRY paradigm, conditions that resulted in Undefined Behavior and unused legacy code. That's probably due to how old some changes are, they probably reflect a much different state of LMMS's code base. To make the transition to automation nodes a better one and avoid having to rework everything later I'm using the fact I have to get in touch with most of this code to try to fix some things. This commit starts refactoring AutomationEditor::mousePressEvent and AutomationEditor::mouseMoveEvent. There are still things to be improved on both but I'll slowly commit them so I can have better versioning control of the PR. Some changes worth noting: - A new action was created in the AutomationEditor class for drawing lines, since its logic was too mixed up with the logic of drawing and dragging a single node. - Changed most variable names to fit the current code style (just very few left to change). - Improved comments explaining the code. - Created a separate method for checking if the mouse position is sitting over an existing node (previously this code was repeated inside the event method and it had flaws on its logic, most of the times returning that the user didn't click a node even when he/she clicked one). Method is called getNodeAt(x,y,r), r being the "radius" to be considered (not actually a radius, the area is actually a square). Some changes that are still planned: - Removing legacy code for features that weren't finished (select and move selection) if it's agreed. - Adding some logic to the DRAW_LINE action so it can be even improved. - Not forgetting the main focus of the PR, adding a way for the user to edit the outValue of nodes.
When adding a value to a particular time, instead of checking if there's a node there already and manually setting its value, we just assign it with a new AutomationNode. QMap silently removes the existing node and adds the new one.
|
Updated draft PR with some of the refactoring and fixing I've been making to |
Adds an upgrade method for the change in the automation nodes settings. The "value" attribute of the <time> element is deleted and assigned to the "inValue" and "outValue" attributes. Now older project files can be loaded properly.
This commit introduces a way for the user to drag outValues on the Automation Editor, by Alt+clicking the outValue red node and dragging up and down. A new action was added for that purpose, called MOVE_OUTVALUE. When the user clicks a outValue sphere with the Alt modifier, m_action is set to MOVE_OUTVALUE, and the time position of the node being affected is stored on m_draggedOutValueKey. That is later used on the mouseMoveEvent to update the outValue of the node. Removed repeated code on the mouseReleaseEvent and removed excessive blank lines after a method. Things to keep in mind when testing through this commit's build: - Creating/Moving an automation node resets the outValue - When the outValue is changed, generateTangents isn't called automatically. So you'll notice that after adding another automation node the curves change (that's because after the new node being added the tangents are then recalculated). Still lots of work ahead!
|
Last two commits:
TODO:
|
Unnecessary loop was removed with call to appropriate method.
Creates a separate header and source file for the AutomationNode class.
Adds 2 new members to the AutomationNode class: m_pattern - A pointer to the pattern this node belongs to m_key - The time position (timeMap key) of this node The constructors (and places they were used) were updated accordingly. AutomationNode was also made a friend class of AutomationPattern so it can access private/protected methods from its pointer. This will be later used to allow AutomationNode's to call generateTangents once their values are updated. Small fix on a code block related to moving selections inside the mouseMoveEvent from AutomationEditor.
This commit removes code that was not currently used in the AutomationEditor. Most of it was related to a feature I believe was once functional but broke along the way, but the code was not cleaned up in an effort to fix it later. The feature allowed selecting and moving/cutting/copying/pasting/removing values from the automation pattern. It added up to lots of lines of code, which so far I was keeping up to date to the changes. However, I believe (and others devs agree) that rewritting this code later might be a better approach than trying to fix what we currently have, so I'm removing the obsolete code. The git history will allow us to reference back to it when implementing the feature again and this will make it harder for this PR to introduce bugs because a certain affected feature couldn't be tested. It also makes reviewing easier, for there are less affected code to cover. For reviewing purposes: I used a single commit for removing the mentioned code, so its diff in relation to the previous commit should give a good idea of everything that was removed.
The methods that change the inValue and outValue of nodes now also generate new tangents for the previous, current and next nodes (the ones affected), so now when the user drags the node outValue the curve is updated accordingly.
Veratil
requested changes
Oct 16, 2020
Veratil
reviewed
Oct 16, 2020
Veratil
requested changes
Oct 16, 2020
Adds a method to the automation node that returns the valueOffset between inValue and outValue. AutomationPattern::putValue now has an extra parameter called outValueOffset (defaults to 0), so when it adds a node to the timeMap the outValue is set according to this offset. flipY() will calculate the offset and invert it, so the discrete jumps are kept but flipped vertically as well. flipX() doesn't need to calculate this offset because it uses the outValue itself. Obs: I believe cleanObjects() was meant to be called everytime AutomationPattern::flipX() is called, but it was inside a conditional that would only call it on some situations. I moved it outside of the conditional.
User can now reset the outValue of nodes on a very analogous way to removing nodes but with the Alt key pressed. Alt + right clicking over the node (or dragging over an area), or Alt + clicking on it on Erase mode (or dragging over an area) will reset the outValues of the nodes. To do that, two new actions were created: ERASE_VALUES (for the regular node removing) and RESET_OUTVALUES (for the outValues reseting). Those are checked for in the moveMouseEvent, which acts accordingly. The removePoints() method was removed and two new methods were added instead: removeNodes() and resetNodes(), which will remove nodes on a tick range or reset them respectivately. The AutomationNode.h and AutomationNode.cpp files were fixed to fit the current code style conventions. The other files were kept as is, so they can be changed all at once at the end of the PR. Change requests made by Veratil were done.
|
Update: Flipping patterns now accounts for the discrete jumps (flipping them too if necessary). The discrete jump values can now be reset by using the same logic as removing the nodes, when the AutomationNode files were fixed to match code style conventions, all others are still not compliant but I figured it would be better to fix it all at once than to have parts of it complying and parts not. |
|
From the TODO list, basically the only thing left to do is allow discrete jumps to be kept when dragging a node. I have an idea on how to do it and might finish it later today. I think after that the PR will basically be ready to go from "Draft" to "Ready To Review"! |
This commit makes a small change to setDragValue. When starting the drag (m_dragging == false), it checks if the time position being dragged already has a node. If it does, then the offset between inValue and offValue is stored on m_dragOutValueOffset so it can be used on the putValue calls, keeping the offset. If there isn't a node in the time position, m_dragOutValueOffset is set to 0.
Fixes the modified code to comply to current code style convention. I tried to keep the changes exclusive to the lines modified by this PR (to keep the diff cleaner), but I might have fixed a couple of other because either they were hard to differentiate on the current diff or because they were too close in context. But still, tried to keep changes mostly to the lines actually changed by the PR.
|
I'm pretty sure alt+click+drag is invalid because on some OSes it will drag the application window. I think we may need to rely more on the different modes. Some ideas:
The last one is a bit hard to describe, but visually speaking: |
|
@Spekular Oh damn, I was testing on Linux so it worked fine but I didn't think other OSs would have trouble with that. It's a shame. From the testing time I had I think from the options you suggested I'm inclined to the first or second. If the second is chose, people used to the current pattern editing workflow might complain to have to switch to erase mode (I think I barely use it myself tbh), but we are kind of limited in number of modifiers, so we have to lose somewhere to give space to that tool. I don't know, I think I'll just go with the new editing mode and we can figure out some shortcut to it to improve workflow later. |
Adds a CSS property for the outValue color and renames the one used for the inValue color so they are consistent. Colors were added to classic and default themes. The original inValue colors were kept, but to fit with the outValue node they had a little bit of transparency added.
To truly flip a pattern horizontally, we also needed to swap the inValues and outValues of the nodes being flipped. This commit fixes that behavior by including the swap when necessary. Also adds a TODO comment regarding the behavior of the generateTangents method when the difference between inValue and outValue is very small.
|
@PhysSong just fixed the |
|
We're already breaking some projects with the new relative path handling, but if we can make new projects work better on old versions, that might be worth doing. |
Changes the inValue attribute name on the XML from "inValue" to "value" to make projects created with the new automation pattern partially backwards compatible (outValues will still not be loaded obviously, but the inValues will be loaded as the regular pattern values we had before).
Fixes bug on AutomationPattern::resetNodes: If it was called with tick0 == tick1, m_timeMap.find() return value was being used to call resetOutValue. When QMap::find() doesn't find a value it returns QMap::end() though, resulting in a SegFault. Fixed by checking if the return value is QMap::end() first.
|
@PhysSong you mean from #5735 ? True, I think that would break projects for older versions if the local prefix is used. Since the change is small here I just went ahead and committed it. If we decide that a more accurate XML attribute name would be better I can reverse it. I also committed a small bugfix on |
|
I haven't fully reviewed this, but I think I reviewed almost everything in many previous reviews. I think now it's the time to test this. |
|
Besides myself, PR has been tested by @superpaik (not very recently though) and recently by Discord user Iron Squid. Unfortunately we haven't had many volunteer testers lately (or ever? 😬 ). I was wondering if @DomClark still wants to give this a review. If not, can we begin a countdown for merging this one? Obviously it can wait if he wishes to review! |
Adds a note about the default copy-assignment from AutomationNode being used on the AutomationPattern constructor. If any dynamic allocated resources are added to the class, an user-defined copy-assignment constructor should be used instead. Also changes the position of an arithmetics operator on a multiline statement for readability.
|
I'll try to do a final review this weekend to make sure it's not breaking anything. |
PhysSong
reviewed
Jan 30, 2021
Comment on lines
+484
to
+487
| // TODO: Currently when rescaling the timeMap values to fit the new range of values (newMin and newMax) | ||
| // only the inValue is being considered and the outValue is being reset to the inValue (so discrete jumps | ||
| // are discarded). Possibly later we will want discrete jumps to be maintained so we will need to upgrade | ||
| // the logic to account for them. |
There was a problem hiding this comment.
@IanCaio So what should we do with this for this PR?
|
This PR fixes some potential race conditions between the UI thread and rendering threads, but now RT threads may be blocked. We need a better mechanism, but fixing it may be out of scope. |
This commit addresses PhysSong review: - An if-statement was replaced with a ternary operator on AutomationEditor::paintEvent(). - One comment was improved on PianoRow::drawDetuningInfo() to make it clearer that drawing cubic hermit curves as straight lines is just a temporary thing. - While applying the requested changes on the AutomationPattern::flipY() method, I noticed that even though it accepted any integer values as min and max, the current logic (both on master and on the previous commit from this PR) would only work if the range was [-max,+max] or [0,+max]. If min was -5 and max 10 for example, the flipping could return us a pattern with values out of range. This commit replaces the logic with an improved one that now relies on the distance between the node and the edges of the range instead, thus working for any range instead of those two mentioned earlier. The new logic also attend to the request of using a for-loop and also using in-place operations.
|
There are some TODOs which are not addressed:
However, I don't consider them as blocking issues. |
Awesome, could we merge tomorrow if no one objects? |
|
When is the tomorrow? 😄 |
Tomorrow. 😏 |
|
Merging it now then! Hadn't done it before because only Veratil approved, so I thought you could still want to review something else |
This was referenced Feb 28, 2021
IanCaio
added a commit
to IanCaio/lmms
that referenced
this pull request
Mar 28, 2021
devnexen
pushed a commit
to devnexen/lmms
that referenced
this pull request
Apr 10, 2021
sdasda7777
pushed a commit
to sdasda7777/lmms
that referenced
this pull request
Jun 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original goal of this PR was to introduce the use of Automation Nodes instead of Automation Values to give more flexibility in terms of adjusting the automation shapes. During the process of writing some issues were noticed in affected files (mostly on
AutomationEditor.cpp), and since they would have to be changed to the introduction of this new feature they were fixed as those issues were found, adding up to the final size of the PR. I believe the automation editor code was improved in comparison to what we had (there were conditions that caused UB, obsolete legacy code and repeated routines for example) and refactoring it created a clear ground for introducing the automation nodes.Automation Pattern behavior
Current behavior
At the moment, each automation pattern is basically a sequence of values in a timeline. Every point on the automation editor represents the value of the automated parameter at that particular time. The progression type will tell LMMS how it should transition between each of those values.The problem with that approach is that we can't store any more information regarding that point in time, so we are very limited in terms of adjustments that can be done to the shape of the automation pattern.
New behavior
With the changes proposed by this PR, instead of values in a timeline we have Automation Nodes. Nodes can store basically any information we want it to, not just a value. Right now, it stores an input value, an output value, the input and output tangents for curve progressions, the position in the timeline and the pattern that owns it.
The input value (
inValue) represents the value of the node for the progression on the left side of it. It can be considered the real value of that node, since it represents the value that the parameter should have exactly at that time. It's represented by a blue circle.The output value (
outValue) represents the value of the node immediately after the time it's situated in. That value will be used for the progression to the right side of the node. It allows the user to create a discrete jump on the exact position of that node. By default, the output value matches the input value.The input tangent of the curve represents the tangent of the curve (for Cubic Hermite progressions) to the left of the node and the output tangent the same, but for the right side of the curve. They are currently automatically calculated using the same logic as before, but having them stored at the automation node opens the possibility of we allowing them to be manually changed in the future.
Another idea that was discussed (and was the original motivation for this transition) is having the nodes storing a progression type, so we can have different transition types on the same automation pattern. This PR creates the foundation for that to be implemented, but considering the current size of it already I believe that should be left for another PR, after this one is merged.
Automation Editor controls
Currently, the automation editor have 2 edit modes: Draw mode and Erase mode. With the changes on this PR, a third mode is added ("Draw outValue mode"). The icon representing it is to be changed later, right now we just reuse the one from Draw mode. The controls for each mode are:
Draw mode (Shift + D):
Draw outValue mode (Shift + C):
Erase mode (Shift + E):
Flipping horizontally and vertically keeps the discrete jumps on the nodes (flipping them as well).
Things to be tested
Since this PR affects the Automation Editor and Automation Pattern files deeply, they should be intensely tested. There's not a particular thing to be focused on but basically the whole functionality of the Automation Editor (including Piano Roll detuning) should be tested.