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

4 new note modification tools #5857

Merged
merged 12 commits into from Mar 5, 2021
Merged

4 new note modification tools #5857

merged 12 commits into from Mar 5, 2021

Conversation

allejok96
Copy link
Contributor

Add 4 new tools to the Piano roll note tools menu:

bild

@LmmsBot
Copy link

LmmsBot commented Dec 23, 2020

🤖 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://12645-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bg1302cb4-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12645?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://12642-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bg1302cb431-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12642?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12644-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bg1302cb431-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12644?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/ojw289xn6olj0nel/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37987275"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/07qd7pb339m4rwcp/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37987275"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12641-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.82%2Bg1302cb431-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12641?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "d5dcac2c0f4932fc60bcfc0465e6af8bc3f025c0"}

@Iron-Squid
Copy link

These are pretty good tools, but I suggest latest be changed to Last note to make it more consistent with the Note Length drop-down menu.

@allejok96
Copy link
Contributor Author

Thanks, I wanted an English speaker's input on that one. I was afraid "last" could be interpreted as "rightmost".

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! Minor code style suggestion.

We also need some testing to check this approach fits all case scenarios, from reading the logic it seems it does.

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
@PhysSong
Copy link
Member

Looks good. Merge?

@IanCaio
Copy link
Contributor

IanCaio commented Jan 12, 2021

My approval still stands, can merge it IMO!

@superpaik
Copy link
Contributor

Can you explain what is the expected behaviour for each new tool so I can test them based on that?
Suggestion: add a short-cut to the last two new funcionalities, as the other have

@allejok96
Copy link
Contributor Author

@superpaik

  • Fill: grow notes/chords to the next note/chord
  • Cut overlaps: reduce notes/chords to stop where the next note/chord begins
  • Min length: grow notes to be no shorter than the last used (last clicked) note
  • Max length: reduce note lengths to be no longer than last used

I don't think we should add shortcuts to the last two since they are more of a "use once" action, and we shouldn't waste shortcuts.

@Iron-Squid
Copy link

Iron-Squid commented Jan 13, 2021

What if instead of having separate shortcuts for different tools, we had one shortcut for the current selected tool, and could switch between them with shortcut + up/down arrow keys? With the current shortcut displayed where possible, of course.
Just a suggestion.

@allejok96
Copy link
Contributor Author

Interesting suggestion @Iron-Squid ... speaking for myself, I'd rather keep the shortcuts for faster access to the more common tools

@superpaik
Copy link
Contributor

Thanks for explaining how it should work.
Some issues found:
Fill: sometimes notes/chord grow to end of next beat, sometimes no (test 1)
Cut overlaps: somehow have the same problem (test 2)
If the videos aren't clear, please don't hesitate to ask. In the videos, actions are executed via short-cut.
I tested it with the "no duration" notes (the one that are created inside the "beat+bassline editor") and they work well. It's a perfect feature to transform those notes into regular notes, something that right now it's almost impossible. Well done!

As for the short-cut for the other options, I don't know how internally short-cut works, but if they are only local to the piano-roll (so they can be used on other windows with other features) I don't see them as a waste. To me the two options aren't a "one shot" because they can be usefull when you are writing a melody and you want to test different note durations (like 1/4, 1/8, ...) so you change one and apply it to the others. I know it can be done via selecting all notes and changing them all at once, but I find that with this option it would be quicker and easier.
test1
test2

@allejok96
Copy link
Contributor Author

Good find @superpaik. I designed Fill to grow the notes to the end of the bar. But it's a bug that Cut cuts the last note spanning a bar line...

@allejok96
Copy link
Contributor Author

Test 2 bug is fixed.

Test 1 bug remains. It happens when the last note ends before the bar line, but a previous note spans across the line... I don't think it's worth adding a check for that to the algorithm. The computer still does 99% of it right. Or am I wrong?

@superpaik I added min/max as a lazy way to fix extremely short/long notes created by fill/cut. I understand it can be used in many different ways, but when editing and trying different note lengths, wouldn't it work just as good using ctrl+A + resize + shift+C like shown below?

(And to be honest I think the design of min/max is backwards when the user has to click a note of the desired length first, then click the tool... But it was the easiest solution.)

rec2.mp4

@superpaik
Copy link
Contributor

Sorry. I couldn't retest it because somehow there isn't the functionality on the installables generated by the bot (checked both mingw and MSVC)

@IanCaio
Copy link
Contributor

IanCaio commented Jan 27, 2021

As you mentioned, the bug 1 is actually the behavior of the algorithm, but it might be a little surprising to the user. What if the behavior for the last chord was: Stretch to the next full bar OR to end position of the note that reaches further, whichever is bigger? Then in that case the small note would fill up to the end of the previous one. The changes to the code would look a bit like this:

			TimePos lastChordEndPos(0);
// ...
			// Update the end position for the last chord, which will stretch
			// to the position where the furthest chord ends or to the
			// next full bar, whichever is bigger
			lastChordEndPos = qMax(notes[i].endPos(), lastChordEndPos);

			// Stretch last chord to end of bar or to the calculated position
			// notes[i] will always be the longest note in the chord
			if (i+1 == notes.count())
			{
				TimePos lastBarPos(notes[i]->endPos().nextFullBar() * TimePos::ticksPerBar())
				lastChordEndPos = qMax(lastBarPos, lastChordEndPos);
				chordLength = lastChordEndPos - notes[i]->pos();
			}

I'm not sure that behavior is more user friendly, but at least you wouldn't unexpectedly have the last chord ending before the previous chord. Let me know if I didn't explain it well enough, can throw some images to give some examples.

@allejok96
Copy link
Contributor Author

Thanks @IanCaio, now both bugs @superpaik mentioned are fixed, and your approach made the code simpler too.

Current implementation: Fill stretches the last chord to the end of the last bar (the end of the clip/tco in song editor).

@allejok96 allejok96 mentioned this pull request Jan 30, 2021
@superpaik
Copy link
Contributor

superpaik commented Feb 1, 2021

HI! For the fill, I don't know if the behaviour attached is working as expected or no. To me is weird. Why A#4 is not "filled" to "F#4"?
As for the cutOverlaps, I think still there are some issues. Some overlaps are not treated.
test-fill
test-cutOverlaps

@IanCaio
Copy link
Contributor

IanCaio commented Feb 1, 2021

HI! For the fill, I don't know if the behaviour attached is working as expected or no. To me is weird. Why A#4 is not "filled" to "F#4"?
As for the cutOverlaps, I think still there are some issues. Some overlaps are not treated.

Thanks for the regularly testing @superpaik, been really helping out specially now we are getting ready for the refactoring! I was about to post that one in #testing after the latest changes, but you beat me to it.

From what I see on the gifs, the behavior is what is expected from the code. I'm not sure if in terms of UX it's the best and most expected behavior, but what you see is not a bug in the code.

First of all, any of the actions treat the notes in the patterns as chords. If multiple notes are in the same position in the timeline they are considered part of a chord (even if their sizes are different).

Situation from GIF 1)
The behavior of "Fill" is that it will stretch every Chord to the beginning of the next one in the sequence. The last chord will stretch to the last bar or to the end of the last note, whichever is furthest away. Another behavior is that it will never shrink a chord: it only increases notes size, never decreases. What happens in that example is that the "Chord" (in this case, a single note) that is after A#4 is E4. So it would stretch until it reaches it, but the end of A#4 is already farther than the beginning of E4. Since fill only grows notes, it doesn't shrink A#4 so it touches the beginning of E4, but it also doesn't grow it to F#4 (because the "chord" behind this one is the D#4 single note).

It's possible to come up with a logic that stretches the end of a note to the beginning of a note closest to it (the way you were expecting A#4 to stretch to F#4). But that's a completely different logic from the current one. I think for now this approach is simple and useful for most scenarios where it's designed to be used (which is basically filling gaps between chords after MIDI recording afaik). In the future we can rethink it and account for that situation too, or maybe make a different action that works that way.

Situation from GIF 2)
Since notes that are in the same position are considered part of the same chord, on the first time you trigger "Cut Overlaps", B4 is one "chord" and D#4 is another "chord". It then shrinks B4 to be at the beginning of the next chord. When you move D#4 to the same position as B4, they are now part of the same chord. Maybe we could make it so the "Cut Overlaps" also shrink the notes of the last chord so they are all the same size.

@superpaik
Copy link
Contributor

Okey!
I understand there are so many different scenarios so it's very difficult to make it work as the user wants/expect. If the functionality is clear and is working as expected, then is ok. The only thing I'd do is write the current behavior in the help so people know what to do with those tools, because that concept of "chords" could be misleading from a user point of view (not always all the notes of a chord start at the same position -for example, if you are adding swing or want to sound more "human")
Anyway, I'm aware of the refactoring phase and all the work behind it. So to me, these tools are ok.
Thanks for the quick and detalied explanation!

@allejok96
Copy link
Contributor Author

@superpaik you've brought good feedback. There will always be edge cases. The more weird scenarios we try to account for, the more complex the logic becomes and the more unpredictable the tool is. So we need to come up with a simple and straightforward rule for how the tool should work.

GIF 1:
Maybe Fill should extend the END position of notes to the closest start position to the right? The more I think about it, the better it seems.

GIF 2:
I think this is exactly what the user can expect, because...

  1. It shouldn't cut the end of B4 because that part doesn't overlap anything.
  2. It shouldn't cut the beginning of B4 because that would significantly alter the melody.
  3. It shouldn't remove D#4 because that would remove all harmony in a song.
  4. It shouldn't extend D#4 to the length of B4, because the tool is called "cut".

If a user is writing swingy chords, that user won't use Cut overlaps, because it will destroy everything, and that's it.

@IanCaio
Copy link
Contributor

IanCaio commented Feb 1, 2021

Maybe Fill should extend the END position of notes to the closest start position to the right? The more I think about it, the better it seems.

I don't think that would be a bad idea. It would work for both the chord situations you were originally making it for (from Avicii video) and on the situations superpaik was expecting. It would be slightly less efficient because for each end position you'd have to go through all notes following it until you found one that has a beginning position higher or equal to the end position and then stretch the single note. I could sketch some code if you want help with it, might have some free time tomorrow to work on something.

I agree the cut actions seems to be good as it is, I can't think of a better behavior right now at least.

@superpaik
Copy link
Contributor

Yes, I agree with you both. Maybe I'm overthinking things because I'm in "testing" mode ;-) Maybe we should follow the Pareto Principle and focus on the 80% of most used and normal scenarios. Thanks for the effort.

@IanCaio
Copy link
Contributor

IanCaio commented Feb 2, 2021

Yes, I agree with you both. Maybe I'm overthinking things because I'm in "testing" mode ;-) Maybe we should follow the Pareto Principle and focus on the 80% of most used and normal scenarios. Thanks for the effort.

The UX perspective is very important too, so keep bringing those concerns up! I think both the author and myself agree with you on the first case, I think the fill behavior could be improved to fill up to the closest note beginning (even if it's 2 or 3 notes away).

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor code style fixes and one small possible fix on the logic. Besides that the new logic looks good, without testing (just reading) it seems to fit what @superpaik was expecting as the behavior.

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay, after the last fixes it LGTM!

@PhysSong
Copy link
Member

@allejok96 Could you resolve the conflict with the piano-roll knife PR? I think it can be merged after conflict resolution.

@IanCaio
Copy link
Contributor

IanCaio commented Mar 4, 2021

Also merging this one tomorrow if no one objects!

@IanCaio IanCaio mentioned this pull request Mar 4, 2021
@IanCaio IanCaio merged commit 5d366ef into LMMS:master Mar 5, 2021
@allejok96 allejok96 deleted the note-tools branch March 5, 2021 18:38
IanCaio added a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
* Add new note length modification tools

* "Last" instead of "latest"

* Code formatting

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>

* Fix sort

* Fix incorrect cut of last note

* Fix Fill not stretching last note to end

* Fill from end positions

* Rename limitNoteLengths

* Compare with non-selected notes when filling

* Style changes by IanCaio + bugfix

* break instead of continue

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
* Add new note length modification tools

* "Last" instead of "latest"

* Code formatting

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>

* Fix sort

* Fix incorrect cut of last note

* Fix Fill not stretching last note to end

* Fill from end positions

* Rename limitNoteLengths

* Compare with non-selected notes when filling

* Style changes by IanCaio + bugfix

* break instead of continue

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Add new note length modification tools

* "Last" instead of "latest"

* Code formatting

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>

* Fix sort

* Fix incorrect cut of last note

* Fix Fill not stretching last note to end

* Fill from end positions

* Rename limitNoteLengths

* Compare with non-selected notes when filling

* Style changes by IanCaio + bugfix

* break instead of continue

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants