Adds feature to edit tangents of Cubic Hermite progressions #5924
Conversation
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.
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.
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.
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!
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.
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.
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.
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.
Adds doxygen comments explaining methods that were either introduced by this PR or which had parameters modified by this PR. Changes valueAt(timeMap::iterator, int offset) method, so it can handle offsets equal to 0 properly. This method is currently never used with an offset of 0 (because this case scenario is handled before this method is called), but it was a simply modification so I just added the conditional to make it possible to use an offset of 0.
AutomationPattern::flipX and AutomationPattern::flipY had some issues to the logic that caused UBs (from accessing an iterator past QMap::end()) and possibly misbehaviors when flipping an empty pattern. Both were refactored to fix those noted issues.
Adds another editing mode to Automation Editor (DRAW_OUTVALUES), specific to deal with node outValues. The Pixmap being used is the same as the DRAW edit mode for now. The way it works now: DRAW Mode (Shortcut SHIFT+D) Shift + Left click = Draws lines of nodes Left click = Draws/Drag node Right click = Remove nodes ERASE Mode (Shortcut SHIFT+E) Left click = Remove nodes Right click = Reset outValues DRAW_OUTVALUES Mode (Shortcut SHIFT+C) Left click = Drags outValue Right click = Reset outValues Now using a switch statement on the events to make things more organized.
Now, instead of only being able to change an outValue by clicking over the sphere representing it, the user can also click on any time on the pattern: If the quantized time of the place he clicked has a node, the outValue of its node will be set to the value where the mouse click happened and the outValue will start being dragged. Very similar to the way it works for the node itself on the draw mode.
Adds a recursive mutex to the AutomationPattern class and locks it on every method that access the member variables. Also rename the mutex from the AutomationEditor class and add locks to some methods that didn't have it before (except on methods that don't access member variables).
Applies changes requested by Veratil: - Replaces NULL with nullptr where necessary on AutomationEditor.cpp - Fixes spacing on the mutex commit (plus some other places) - Changes some if blocks to one liners - Replace while with do-while on some places, since the condition was already checked for earlier on the method. - Moves getNodeAt call a level up on the block, since it's called on both conditionals below. - Fixes identation on some code inside AutomationEditor::mousePressEvent. - Adds explicits blocks on a switch statement. Even though this was not necessary for that particular one (because there was no variable declaration inside it) it helps keeping it consistent with another switch statement that happened earlier. I also added a break statement to the last case of a switch (even though it was not needed, it's safer to avoid mistakes in the future with new cases being added).
The red sphere representing the outValue was drawed after the blue sphere representing the inValue. Because of that, if they had the same value the red sphere would be on top. For the user, it makes more sense to be able to see the blue sphere representing the input value on top instead. This commit changes the order of the drawing.
Fixes some comments pointed out on Spekular's review. Changes the AutomationNode's variable m_key to m_pos (leaving a comment on the header reminding that it matches the timeMap key). Removes comments related to removed code. Fixes code style on a pointer declaration.
Instead of creating a getter and setter for each QProperty, we use MEMBER instead and access those variables directly.
Overloads compound assignment operators +=, -=, *= and /= for AutomationNodes, making it so they affect the inValue and outValue of the node being assigned. Changes AutomationPattern::flipY() so it uses the new operators.
Makes the AutomationEditor::getNodeAt method more efficient, by exiting if the node we are checking is already past the position we given (since the nodes are ordered in the timeMap, all subsequent nodes will also be past the position). Now instead of returning the last node that is inside the coordinates, it returns the first. Also improves AutomationEditor::mousePressEvent to avoid getNodeAt being called twice unnecessarily.
Now, instead of keeping the offset between the inValue and outValue while dragging a node, setDragValue will either keep the current outValue intact, or move it together with the inValue if they are the same. The putValue method now doesn't have an offset parameter. If we want to put a node with an outValue different from the inValue we use putValues instead.
Creates a boolean before the m_editMode switch, that will be true if the action being processed affects outValues and false if it affects inValues. That way we can move the statement clickedNode=getNodeAt() before the switch-case, reducing repeated lines.
|
Anyone testing this ? |
Just go ahead! |
As per requested by PR review, instead of checking if the automation clip uses Cubic Hermite progression to allow changing tangents, we now instead use a method that will return true if the current progression type allows tangent editing. That way it's easier to allow tangent editing for new progression types that might be added in the future.
Fixes new conflicts
|
@IanCaio Needs update once more! Testing now and it works great 👍 One issue:
Some quick observations:
|
Thanks man! I'd have to get a better look, but that's probably due to the button being enabled/disabled on
Those are relatively simple improvements I think, could be done. IMO would be better in another PR, since the more I add to this one the less likely I believe this will actually get reviewed and merged. If I'm being transparent though, I got a bit demotivated with my PRs. I was not coding for a long time, got pinged and came back to fix issues on them, get them ready for merge and review, but they simply got left aside. All of them are over 2 years old and after I got back I saw lots of PRs being reviewed and merged (all way more recent than mine), more conflicts coming up and me having to keep fixing them while I was getting no reviewers to get them out of the way. Don't take me wrong, you're one of the few that showed interest in the PRs which I appreciate, also others that helped keeping them up to date after the major refactor such as @PhysSong , but it got a bit tiring to just be working on them while being left down in the queue. I'll still see if I find some time to take a look at this bug though. |
I agree.
Yeah, I know how that feels. There's unfortunately no easy way to deal with this. I was more or less away from the project while the code were being rearranged and we now have an insane backlog of 150 PR's. I just grabbed the ones that were the easiest to get started with to get them out of the way but there aren't that many PR's I can review as my coding skills are somewhat limited. I'll keep testing the rest of your stuff though. It looks pretty solid all of it. Next up is the |
|
@zonkmachine I think I managed to fix the conflict, now I have to think about how to approach the bugfix. Just another way to reproduce it more easily: you can have two automation clips, have one with linear progression and the other with cubic hermite progression. Then start editing the tangents of the latter, and double click the linear progression clip. You'll have the editor on the Tangent Edit mode with the linear progression automation clip (it won't work but it will be selected). The opposite happens too, if you are in the linear progression clip with the Tangent Edit button disabled, then double click the cubic hermite automation clip it will switch to that but with the button still disabled, you have to change the progression type for it to enable again. I'll see if I can work on that on the weekend and also check the review that was left on the BBTrack PR! |
|
I spent some time testing it with an ASan build. I really like the functionality this PR provides, and it is integrated well. If I had to find something to complain about, it would be the way tangent lines are edited. It seems to work by setting the tangent's slope based on the mouse Y position, and then the mouse X distance from the node sets the amount that changes in the mouse Y position affect the tangent slope. This works and I'm personally fine if we keep it this way, but it may be a bit unintuitive to users - at least at first. |
- The Edit Tangents button was not updating correctly when the clip was changed inside the automation editor. This was fixed by making the update routine a separate method and calling it on the places necessary (when changing progression modes and when changing clips). - Some suggestions from messmerd PR review were also included.
- Just adds 2 more review requested changes that were forgotten in the previous commit.
|
I believe I fixed the reported bug @zonkmachine , let me know if it worked, a quick test here seems to show it's gone. I made the button updating a separate method and now I call it both when the progression type is changed or when the clip is changed. I also addressed @messmerd review requests! The tangent lines are edited the way you described, looked like the simplest way back then. It could be changed in the future, zonkmachine also suggested something with speed sensitivity, but if possible I'd prefer to leave it for a separate PR. |
- Simplifies method that updates the edit tangent button on the automation editor
Thanks man! Agreed, we could extend that to other Now that there's 3 approvals, maybe we can merge in 24h if nobody comments wishing to review it further? |
This PR enables the editing of a node's tangents on the Cubic Hermite progression. It works the following way:
Again, sorry for the commit mess. This was being worked in my fork before #5712 getting merged and Github is making confusion on the commits that belong to this branch and the ones which belongs to the other.