lmms
lmms copied to clipboard
LMMS
Adds feature to edit tangents of Cubic Hermite progressions
This PR enables the editing of a node's tangents on the Cubic Hermite progression. It works the following way:
- A node can have its tangents locked or unlocked. A node with unlocked tangents will have its own tangents recalculated when new nodes are added or nodes are moved around
- On the Edit Tangents mode, the user can edit the tangents of a particular node. To do so, just click on the pattern and the tangents of the closest node will start being edited.
- When a node has had it's tangents manually edited, they will become locked. That means they won't be recalculated when nodes are added/removed/moved.
- Right clicking on the Edit Tangents mode resets the tangents of the closest node. If you hold the RMB and drag it will reset the tangents of all the nodes on the path.
- When a node's tangent is reset, it will become unlocked again and will be recalculated according to the neighbor nodes.
- The tangent information of a node (and whether it's locked or unlocked) is now saved on the project file. It's not necessary to generate the tangents during load up anymore. For older projects compatibility it's checked whether any node doesn't have the tangent values saved, and if so the tangents are generated during load up.
- Color of the tangent line can be edited on the CSS (the tangent circle is just the same color but darkened).
- The tangents are only drawn on the Cubic Hermite mode. They are also only drawn for the nodes that had their tangents edited. That way the user can quickly know which nodes have the tangents locked and which doesn't.
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.
:robot: Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! :tophat:
Linux
- Linux (AppImage):
lmms-1.3.0-alpha.1.295+gee21e6bcb-linux-x86_64.AppImage(build link)
Windows
- Windows 32-bit:
lmms-1.3.0-alpha.1.295+gee21e6bcb-mingw-win32.exe(build link) - Windows 64-bit:
lmms-1.3.0-alpha.1.295+gee21e6bcb-mingw-win64.exe(build link) - Windows 32-bit:
lmms-1.3.0-alpha-msvc2017-win32.exe(build link) - Windows 64-bit:
lmms-1.3.0-alpha-msvc2017-win64.exe(build link)
macOS
:robot:
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/0fb21d25-2f64-416a-99b2-b198d63c9035/artifacts/0/lmms-1.3.0-alpha.1.295+gee21e6bcb-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17898?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://output.circle-artifacts.com/output/job/0ddb7d4a-37b4-4663-9a1e-766b4cd4d611/artifacts/0/lmms-1.3.0-alpha.1.295+gee21e6bcb-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17897?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d6c1da88-85af-4d84-8485-ae7fa8a19dda/artifacts/0/lmms-1.3.0-alpha.1.295+gee21e6bcb-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17901?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/sb5r544ajv61c8j8/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44344928"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/u5xc0e845xj66ch6/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44344928"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/bace2285-3749-4d06-a768-70cf6015b982/artifacts/0/lmms-1.3.0-alpha.1.295+gee21e6bcb-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17900?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "0dfb18c278a1bbfddd5d4106bc747497458ab8b2"}
Tested on Windows 10. Generally it works ok. The only weird thing that I found is that when a point have tangents and you are in "draw mode" and click over that point because you wanna move it, tangents disappear. I think that happens because in reality the action deletes the current point and creates a new one. I don't know if this can be solved, but otherwise I don't know how to move a point with tangents.
Other minor issues:
- The hotkey (Shitf-T) activates the Draw Outmodes tool. It's keyword (Shift-C) does nothing.
- Since edit tangents is only possible while in "Cubic Hermite progression", wouldn't be better to only allow to active the tool if you are using that mode? I mean, if you are on "linear" or "discrete" progression you can activate the edit tangents tool but do nothing. This may confuse the user.
- Tangent reacts to vertical mouse movements (ok) but also to horizontal moves and if you are moving, say, from left to right when you reach the point the tangent goes from totally down to totally up. I don't know if it would be better to only react to vertical movements.
Positive remarks:
- I don't know if that's intended, but if you edit tangents and then change to, say, linear progression, tangents disappear (ok). But if you return to "Cubit Hermite progression" the tangents appear again (super ok). And also that happens if you save and load the project again. Tangents are saved in the project file even if they are not visible in the automation editor. To me is ok!
Thanks for testing! Fixed a few of the points, I'll soon work on the remaining.
Tested on Windows 10. Generally it works ok. The only weird thing that I found is that when a point have tangents and you are in "draw mode" and click over that point because you wanna move it, tangents disappear. I think that happens because in reality the action deletes the current point and creates a new one. I don't know if this can be solved, but otherwise I don't know how to move a point with tangents.
The dragging of nodes involves creating new nodes objects, but I'll try to figure out a way to keep the tangents if we are dragging and not creating a new node. I thought I had done it but apparently forgot :grimacing:
- The hotkey (Shitf-T) activates the Draw Outmodes tool. It's keyword (Shift-C) does nothing.
That was a bug, I did a typo writing a variable's name and was setting the Edit Tangent mode on the Draw Out mode. It should be fixed now!
- Since edit tangents is only possible while in "Cubic Hermite progression", wouldn't be better to only allow to active the tool if you are using that mode? I mean, if you are on "linear" or "discrete" progression you can activate the edit tangents tool but do nothing. This may confuse the user.
Added logic for disabling the Edit Tangent mode button if you're in Discrete or Linear progressions.
- Tangent reacts to vertical mouse movements (ok) but also to horizontal moves and if you are moving, say, from left to right when you reach the point the tangent goes from totally down to totally up. I don't know if it would be better to only react to vertical movements.
The tangents are calculated by the tangent between the mouse position and the node position. When you go from one side to the other it ends up flipping because the X delta shifts sign. If you stop to think of it as a straight line that continues from one side to the other, the tangent is still accurate (the mouse sits on top of this imaginary line), however I can see how this can be confusing for the user. I'll try to add some logic later to account for switching sides on a node while editing the tangent so it mirrors it to compensate.
@superpaik The last 2 points were addressed. Dragging a node now keeps its tangents if they were locked (aka they are visible, meaning they were edited).
I also slightly changed the tangent calculation behavior, but I don't know if it improved over the previous one (I actually prefer the previous) because crossing the mouse through the node still causes changes, it just happens that the tangent is still pointing in the direction of the mouse but mirrored. I can revert it to the previous behavior. It was good that I had to tweak with it anyways because I fixed a bug with a possible division by 0 on the process.
Everything tested before works ok now.
I just notice that undo-redo do not work as expected. When undo all tangent points disappear with just one undo (and appear with redo) That do not happens with normal points. I know undo-redo needs a general refactoring but maybe you can double-check the code to see is something is amiss.
As for the new behaviour when moving the mouse, to me is more appealing , but I agree that is a matter of preference. Maybe some other can say what's best.
Good work!
Thanks man!
I just notice that undo-redo do not work as expected. When undo all tangent points disappear with just one undo (and appear with redo) That do not happens with normal points. I know undo-redo needs a general refactoring but maybe you can double-check the code to see is something is amiss.
My bad, I forgot to add a Journalling checkpoint so it was using whatever checkpoint was available before (which was before you added all the tangents, so they all disappeared). I just added the missing code, let me know if it works fine now!
As for the new behaviour when moving the mouse, to me is more appealing , but I agree that is a matter of preference. Maybe some other can say what's best.
Cool! I'm fine with either one to be honest, so if this new behavior feels better for you I'll keep it!
@IanCaio Needs update once more!
Testing now and it works great :+1:
One issue:
- After reopening a project with edited nodes I've managed to get into a state where the node edit button becomes unresponsive. Method to replicate issue: Create a project with an automation track and edit tangents in it. Save and reload lmms. Open the projet and press the 'Edit tangents' button. The button is now unresponsive. It will come to life again by selecting one of the other progression types and then switching back to Cubic Hermite. I tried 3016a295684c43fa1d86cac841a4c6af7fbafe91 which is the last change that is not 'merge master' and this behavior is already in that version.
Some quick observations:
- I think the tangent editing feels a bit 'nervous'. The mouse action results in big changes and I've even tried to edit my mouse settings to make more minute changes possible. Maybe an accelerator key that increases the resolution of the mouse actions would be motivated?
- After editing the tangent on one side, I often find myself moving the other one to the opposite position so they are aligned. Maybe an option to move the other node automagically?
@IanCaio Needs update once more!
Testing now and it works great 👍
One issue:
- After reopening a project with edited nodes I've managed to get into a state where the node edit button becomes unresponsive. Method to replicate issue: Create a project with an automation track and edit tangents in it. Save and reload lmms. Open the projet and press the 'Edit tangents' button. The button is now unresponsive. It will come to life again by selecting one of the other progression types and then switching back to Cubic Hermite. I tried 3016a29 which is the last change that is not 'merge master' and this behavior is already in that version.
Thanks man! I'd have to get a better look, but that's probably due to the button being enabled/disabled on setProgressionType and me forgetting to account for when a clip is loaded either by drag&drop or by project-loading, where the progression type is changed manually. If you drop an automation clip with a different progression type I imagine a similar bug would happen. That's just a guess looking quickly at the code, would have to confirm. There are a couple ways to fix it if that's the case, either having a progressionTypeChange signal that triggers the button change, or if I add the conditional inside the methods that will set a different clip.
Some quick observations:
I think the tangent editing feels a bit 'nervous'. The mouse action results in big changes and I've even tried to edit my mouse settings to make more minute changes possible. Maybe an accelerator key that increases the resolution of the mouse actions would be motivated?
After editing the tangent on one side, I often find myself moving the other one to the opposite position so they are aligned. Maybe an option to move the other node automagically?
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.
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.
I agree.
I saw lots of PRs being reviewed and merged (all way more recent than mine),
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 ~~BBTrack~~ Pattern Editor features. Update this one when you feel up to it.
@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. It seems to work fine, and I encountered no issues.
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.
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.
LGTM. We should probably refactor the big switch cases in the future though (e.g., putting them into functions).
Thanks man! Agreed, we could extend that to other mouse(Press/Move)Event methods as well, i.e.: PianoRoll has kind of a very big method for those too afair. I'm not sure how exactly to refactor them but I'm sure there's a way to improve those.
Now that there's 3 approvals, maybe we can merge in 24h if nobody comments wishing to review it further?