Limit note length to quantization value #3998

zonkmachine · 2017-11-22T01:30:03Z

Draging a note to it's minimum value of 1 will add this new length to
the note if you later choose to stretch it which will not be clearly
visible in the Piano Roll unless you zoom in a bit. Limit the note
length to the quantization value and use key to set a smaller
value.

PR explained in depth here: #3989 (comment)

Fixes #3989

zonkmachine · 2017-11-27T08:45:26Z

Closing this PR for now. It's not clear to me how to improve this and it's not important for 1.2 anyway.

LmmsBot · 2020-05-09T21:36:58Z

🤖 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

Linux (AppImage): lmms-1.2.1.676-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.1.676-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.2.1.676-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.2.1-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.2.1-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.2.1.676-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://6375-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.676-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6375?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://6372-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.676-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6372?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://6376-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.676-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6376?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/wpn024tn0a7ek51h/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/32859425"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/7ibrqfit5c3984sd/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/32859425"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://6373-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.676-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6373?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "cb01991211606db8780eed53f14d61d0136b51ad"}

Draging a note to it's minimum value of 1 will add this new length to the note if you later choose to stretch it which will not be clearly visible in the Piano Roll unless you zoom in a bit. Limit the note length to the quantization value and use <Alt> key to override and set a smaller value.

Spekular · 2020-05-10T09:24:45Z

Isn't this made reduntant by #4357?

We also have a quantize button in piano roll. Isn't it better to fix that so it affects lengths as well?

I ask because this reminds me of tresf asking for fine offsets to be removed when clips are moved (until he changed his mind). If I want to extend the length of a weird-length note by 1/4, it's much easier to do that at 1/4 Q than to do it by hand at 1/64th Q.

zonkmachine · 2020-05-10T12:24:06Z

Isn't this made reduntant by #4357?

Maybe, I'll look into it.

We also have a quantize button in piano roll. Isn't it better to fix that so it affects lengths as well?

Again maybe, but the issue here are tiny stretches that users aren't aware of that causes the tco to come out the wrong length. If you don't see the note length as being the culprit you're not going to try and change it neither by stretching nor by quantization.

Spekular · 2020-05-10T12:41:24Z

Sorry, it seems I misunderstood what the PR did. I read "limit note length to quantization value" as "do not allow resized notes to be sizes other than n * Q, where Q is the quantization level, n is an integer". I should have looked at the detailed explanation/code instead.

Setting the minimum length to the current Q is better, but still overkill IMO. I think we should only prevent resizing down if the resize would lead to a length of zero. That way a 3/8ths note can be resized to 1/8th even if you're at 1/4th Q, which is a totally valid operation IMO.

Spekular · 2020-05-10T12:45:28Z

src/gui/editors/PianoRoll.cpp

@@ -2811,8 +2811,8 @@ void PianoRoll::dragNotes( int x, int y, bool alt, bool shift, bool ctrl )
 				if (note->selected())
 				{
 					int newLength = note->oldLength() + off_ticks;
-					newLength = qMax(1, newLength);
-					note->setLength( MidiTime(newLength) );
+					newLength = qMax(alt ? 1 : quantization(), newLength);


Implementation for my preferred behavior, since code might be easier to interpret than my explanation 😅

Suggested change

newLength = qMax(alt ? 1 : quantization(), newLength);

if (newLength <= 0) { newLength = quantization(); }

Or if you prefer

Suggested change

newLength = qMax(alt ? 1 : quantization(), newLength);

newLength = (newLength > 0) ? newLength : quantization();

That's elegant! I prefer the <Alt> key in there somewhere. You can skip quantization when changing the length of the note with the <Alt> key so I would prefer:

if (newLength <= 0){ newLength = alt ? 1 : quantization(); }

If you change that I'll commit it.

Like so? :)

Suggested change

newLength = qMax(alt ? 1 : quantization(), newLength);

if (newLength <= 0) { newLength = alt ? 1 : quantization(); }

Reminder that newLength = alt is not a boolean operation, but an assignment one. 😉

Yes. We're assigning either the quantized value or if overriding with Alt key, 1. Then you can still set the note to any length if you want.

Okay! Wanted to make sure since tertiary operator can be confusing sometimes.
EDIT: To quick glance that is. 😁 Looked like newLength == alt ? ... when I saw it.

Co-authored-by: Spekular <Spekular@users.noreply.github.com>

Spekular

LGTM, and feels safe enough that I wouldn't even mind merging it on 1.2.

zonkmachine · 2020-05-11T20:25:31Z

It has some quirks though. Select many different notes that start at different times and change their length. It looks a bit odd when you shorten them and they take turn to first become shorter and then jump up again and become the quantized length.

Spekular · 2020-05-11T20:37:10Z

Oh right, you could end up scaling 3/8->1/8->2/8...

I didn't think it through enough huh 🙃

Spekular · 2020-05-11T21:01:10Z

I wonder if using note->oldLength() % quantization() as a minimum instead of quantization() would work, or if oldLength is updated too frequently. That could also become zero, so then you'd need two checks for zero or something clever.

Spekular · 2020-05-12T16:32:01Z

src/gui/editors/PianoRoll.cpp

+					if (newLength <= 0) { newLength = alt ? 1 : quantization(); }
+					note->setLength(MidiTime(newLength));


This works the way I wanted, judging from an admittedly quick test.

Suggested change

if (newLength <= 0) { newLength = alt ? 1 : quantization(); }

note->setLength(MidiTime(newLength));

int minLength = alt ? 1 : (note->oldLength() % quantization());

if (minLength == 0) { minLength = quantization(); }

note->setLength(MidiTime(max(newLength, minLength)));

Would love to figure out a shorter way to get
x % y if x % y > 0
y otherwise

It works nice but if I set the length of notes of varying length I would want them to end up at the same min value. Example two notes starting at the same time. Lenths, 2/16 and 3/16. Set quantization to 1/8, hit Ctrl + A and set their length to min by dragging. Not the same value.

In that case I think we need to iterate over the selction once before getting here to find the smallest minLength of any selected note, then use that for all of them.

I had to test my latest research so the PR is for now back at my branch.

zonkmachine · 2020-05-12T19:11:28Z

Back to my original commit. I'm experimenting with letting notes shorter than quantization be remembered and kept as minimum value().

int oldLength = note->oldLength();
int newLength = oldLength + off_ticks;
int minLength = qMin(oldLength,quantization());
newLength = qMax(alt ? 1 : minLength, newLength);
...

zonkmachine · 2020-05-15T22:18:22Z

New idea. Maybe just limit to the shortest note that isn't just a tick? It'll still give a short extra length if you drag it out with quantization but one that is more easy to spot.

newLength = qMax(MidiTime::ticksPerTact()/64, newLength);

In following order.
A whole note with a 64th note
A whole note with a tick
A whole note

Spekular · 2020-05-20T20:11:02Z

I'm not a fan of this version, it doesn't allow 3/4 -> 1/4 @ 1/2 Q.

Spekular · 2020-05-20T20:51:03Z

Here's a branch that meets both my criteria and yours:

Notes can be resized to length % quantization, or quantization if the previous math leads to zero
All notes can be resized to be as small as the shortest note's minimum length

master...Spekular:noteLen

The minimum length is stored in a member variable on the initial mouse press in order to avoid recalculating it on every drag event.

zonkmachine · 2020-05-21T17:03:12Z

Good solution. I've played around with it for quite a while and can't seem to find any obvious weak spots. I think you should issue a separate PR for your solution and I'll close this one.

On another but related note. If you have a group of notes with beat notes among them, select them all, and proceeds to extend them by dragging, the beat note won't react until after a whole measure has been covered. It looks like it's the full -192 ticks that has been covered first before there is a visible change. If I manually change the beat note length to -10 in the mmp file it will just stop for a brief moment before being extended. I don't know what a preferable behavior would be here.

Spekular · 2020-05-21T17:15:21Z

I think you should issue a separate PR for your solution

Done!

On another but related note. If you have a group of notes with beat notes among them, select them all, and proceeds to extend them by dragging, the beat note won't react until after a whole measure has been covered.

Confirmed, but just to clarify this doesn't seem to be a regression as it occurs in my master build and 1.2.1 as well. New issue for that problem once this one is closed?

zonkmachine · 2020-05-21T17:19:35Z

Confirmed, but just to clarify this doesn't seem to be a regression as it occurs in my master build and 1.2.1 as well.

Yes, this is not new. It's there on 1.1.3. already.

New issue for that problem once this one is closed?

Yes.

zonkmachine · 2020-05-21T17:27:17Z

Closed in favor of #5512

zonkmachine · 2020-05-21T21:30:35Z

Adding this fixes the resize issue I think. At least it demos the issue.

if (note->selected())
{
	if(note->oldLength() <= 0){note->setOldLength(1);}
	int newLength = qMax(minLength, note->oldLength() + off_ticks);
	note->setLength(MidiTime(newLength));

	m_lenOfNewNotes = note->length();
}

zonkmachine mentioned this pull request Nov 22, 2017

Some patterns in "Song-Editor" and "Beat+Bassline editor" are displayed larger than they are #3989

Closed

zonkmachine closed this Nov 27, 2017

zonkmachine reopened this May 9, 2020

zonkmachine changed the base branch from stable-1.2 to master May 9, 2020 20:54

zonkmachine force-pushed the minimumnotelength branch from 9776788 to 5b47419 Compare May 9, 2020 21:32

zonkmachine force-pushed the minimumnotelength branch from 5b47419 to aa08c52 Compare May 9, 2020 22:18

Spekular reviewed May 10, 2020

View reviewed changes

Update src/gui/editors/PianoRoll.cpp

54e4bc5

Co-authored-by: Spekular <Spekular@users.noreply.github.com>

Spekular approved these changes May 11, 2020

View reviewed changes

Spekular reviewed May 12, 2020

View reviewed changes

Remember min note length if shorter than quantization()

cb01991

Spekular mentioned this pull request May 21, 2020

Better minimum length when resizing notes #5512

Merged

zonkmachine closed this May 21, 2020

zonkmachine deleted the minimumnotelength branch June 1, 2020 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit note length to quantization value #3998

Limit note length to quantization value #3998

zonkmachine commented Nov 22, 2017 •

edited

Loading

zonkmachine commented Nov 27, 2017

LmmsBot commented May 9, 2020 •

edited

Loading

Linux

Windows

macOS

Spekular commented May 10, 2020

zonkmachine commented May 10, 2020

Spekular commented May 10, 2020

Spekular May 10, 2020

zonkmachine May 11, 2020 •

edited

Loading

Spekular May 11, 2020

zonkmachine May 11, 2020

Veratil May 11, 2020

zonkmachine May 11, 2020

Veratil May 11, 2020 •

edited

Loading

Spekular left a comment

zonkmachine commented May 11, 2020

Spekular commented May 11, 2020

Spekular commented May 11, 2020

Spekular May 12, 2020

zonkmachine May 12, 2020

Spekular May 12, 2020

zonkmachine May 13, 2020 •

edited

Loading

zonkmachine commented May 12, 2020

zonkmachine commented May 15, 2020

Spekular commented May 20, 2020

Spekular commented May 20, 2020 •

edited

Loading

zonkmachine commented May 21, 2020

Spekular commented May 21, 2020

zonkmachine commented May 21, 2020

zonkmachine commented May 21, 2020

zonkmachine commented May 21, 2020 •

edited

Loading

	newLength = qMax(alt ? 1 : quantization(), newLength);
	if (newLength <= 0) { newLength = quantization(); }

	newLength = qMax(alt ? 1 : quantization(), newLength);
	newLength = (newLength > 0) ? newLength : quantization();

	newLength = qMax(alt ? 1 : quantization(), newLength);
	if (newLength <= 0) { newLength = alt ? 1 : quantization(); }

		if (newLength <= 0) { newLength = alt ? 1 : quantization(); }
		note->setLength(MidiTime(newLength));

-					if (newLength <= 0) { newLength = alt ? 1 : quantization(); }
-					note->setLength(MidiTime(newLength));
+					int minLength = alt ? 1 : (note->oldLength() % quantization());
+					if (minLength == 0) { minLength = quantization(); }
+					note->setLength(MidiTime(max(newLength, minLength)));

Limit note length to quantization value #3998

Limit note length to quantization value #3998

Conversation

zonkmachine commented Nov 22, 2017 • edited Loading

zonkmachine commented Nov 27, 2017

LmmsBot commented May 9, 2020 • edited Loading

Linux

Windows

macOS

Spekular commented May 10, 2020

zonkmachine commented May 10, 2020

Spekular commented May 10, 2020

Choose a reason for hiding this comment

zonkmachine May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veratil May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Spekular left a comment

Choose a reason for hiding this comment

zonkmachine commented May 11, 2020

Spekular commented May 11, 2020

Spekular commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine May 13, 2020 • edited Loading

Choose a reason for hiding this comment

zonkmachine commented May 12, 2020

zonkmachine commented May 15, 2020

Spekular commented May 20, 2020

Spekular commented May 20, 2020 • edited Loading

zonkmachine commented May 21, 2020

Spekular commented May 21, 2020

zonkmachine commented May 21, 2020

zonkmachine commented May 21, 2020

zonkmachine commented May 21, 2020 • edited Loading

zonkmachine commented Nov 22, 2017 •

edited

Loading

LmmsBot commented May 9, 2020 •

edited

Loading

zonkmachine May 11, 2020 •

edited

Loading

Veratil May 11, 2020 •

edited

Loading

zonkmachine May 13, 2020 •

edited

Loading

Spekular commented May 20, 2020 •

edited

Loading

zonkmachine commented May 21, 2020 •

edited

Loading