WIP: Piano Roll mouse refactor + extras #6063

Veratil · 2021-06-19T14:42:59Z

This is part of my continued effort to clean up the Piano Roll code to make it easier to maintain and read (and be faster), with help from @IanCaio. I wanted to open this now since most of the major work is done, and I'd like to start getting some testing and reviews. I'm certain there are some edge cases from the refactor that myself and Ian haven't found. Also if there are any suggestions for duplicated code that we can pull out into their own function would be helpful.

Quick summary of changes:

organize code paths under switch statements depending on which duty or function is happening
formatting code to new conventions
pulling some code into their own functions
adding fixes for Piano Roll doesn't highlight chord notes properly when vertically scaled. #6020 Piano keys in the piano roll doesn't send aftertouch events #6028 Edit Mode(Move) Inconsitent Use Piano Roll vs Song Editor #6058

My known issues:

adjacent bug to Piano Roll doesn't highlight chord notes properly when vertically scaled. #6020 - when zooming from a higher to lower value, auto-marked semitones have an off by one error (which fixes itself on the next paintEvent)

Changes variable names from PianoRoll::mousePressEvent to camelCase and changes 2 comments.

Removes a loop that removed every note except one and replaced it with a clear() call plus appending the note. Removes a magic number used for finding the closest note.

* Changes mousePressEvent variables to camelCase Changes variable names from PianoRoll::mousePressEvent to camelCase and changes 2 comments. * Style fixes on mouseDoubleClickEvent * Removes magic number and costy loop Removes a loop that removed every note except one and replaced it with a clear() call plus appending the note. Removes a magic number used for finding the closest note. * Small fixes + Code style Fixes small bug with the vertical selection mode scrolling. Improves logic on the cursor selection of ActionNone. Adds update() + return statements for the actions that didn't have them, and removed the update() from outside the switch. Fixes the code style.

* Changes mousePressEvent variables to camelCase Changes variable names from PianoRoll::mousePressEvent to camelCase and changes 2 comments. * Style fixes on mouseDoubleClickEvent * Removes magic number and costy loop Removes a loop that removed every note except one and replaced it with a clear() call plus appending the note. Removes a magic number used for finding the closest note. * Small fixes + Code style Fixes small bug with the vertical selection mode scrolling. Improves logic on the cursor selection of ActionNone. Adds update() + return statements for the actions that didn't have them, and removed the update() from outside the switch. Fixes the code style. * Adds Remove Note action Adds a action to remove notes on the PianoRoll, and remove an unnecessary member variable that was only being used to handle that action (m_mouseDownRight). Also removes unnecessary handling of the play key action on the ActionNone case switch from the mouseMoveEvent. Adds a update() call on the ActionPlayKeys (so it updates the cursor) and a break; statement for consistency. Also fixes the calculation of the velocity.

LmmsBot · 2021-06-19T15:00:29Z

🤖 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.3.0-alpha.1.132%2Bg6ea5ae0-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-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

macOS : lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14048-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.132%2Bg6ea5ae0-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14048?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://14049-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14049?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14050-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14050?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/u78yqfavievs41po/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39669311"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/f7c0x8st2rj88220/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39669311"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14046-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.132%2Bg6ea5ae036-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14046?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "231e38d742551e739411613e88748c014af672fb"}

PhysSong

This is not a complete review yet. I have to read the remaining part later.

PhysSong · 2021-06-20T04:01:44Z

src/gui/editors/PianoRoll.cpp

 	const int visible_space = keyAreaBottom() - keyAreaTop();
 	m_totalKeysToScroll = qMax(0, NumKeys - 1 - visible_space / m_keyLineHeight);
-
+	


Suggested change

PhysSong · 2021-06-20T04:04:24Z

src/gui/editors/PianoRoll.cpp

@@ -449,6 +447,8 @@ PianoRoll::PianoRoll() :
 		this, SLOT(changeSnapMode()));

 	m_stepRecorder.initialize();
+
+	m_lastChord = nullptr;


Why do you have this here instead of the member initializer list?

That's a good question. I don't know what I was thinking!

PhysSong · 2021-06-20T04:05:35Z

src/gui/editors/PianoRoll.cpp

 		PR_TOP_MARGIN,
-		width() - m_whiteKeyWidth,
-		keyAreaBottom() - PR_TOP_MARGIN);
+		noteAreaWidth(),
+		keyAreaBottom() - keyAreaTop());


Why are you still using PR_TOP_MARGIN instead of keyAreaTop()?

I am using keyAreaTop()?

Oh nevermind you're talking about the line above the change. I just missed it is all.

PhysSong · 2021-06-26T07:56:59Z

src/gui/editors/PianoRoll.cpp

-	}
-
-	if( m_editMode == ModeEditDetuning && noteUnderMouse() )
+PianoRoll::PianoRollArea PianoRoll::getPianoRollAreaIn(const int x, const int y)


I would name it like coordToArea or pointToArea, but I'll leave the naming stuff up to you.

Hmm, yeah I think pointToPianoArea could work?

PhysSong · 2021-06-26T08:09:18Z

src/gui/editors/PianoRoll.cpp

+	// ticks per pixel = ticks per bar / pixels per bar
+	// const int tpp = static_cast<int>(static_cast<float>(TimePos::ticksPerBar()) / m_ppb);
+	// pixels per tick = pixels per bar / ticks per bar
+	const int ppt = static_cast<int>(static_cast<float>(m_ppb) / TimePos::ticksPerBar());


This will truncate the decimal part and affect ticks to position calculation.

For some reason that just didn't click in my head. 😂 I'll change to float.

PhysSong · 2021-06-26T08:10:37Z

src/gui/editors/PianoRoll.cpp

+	const int ppt = static_cast<int>(static_cast<float>(m_ppb) / TimePos::ticksPerBar());
+
+	// Area mouse clicked in
+	const PianoRollArea pra = getPianoRollAreaIn(mex, mey);


I prefer using this directly in switch statements. The same for the other occurrences.

That can be done. 👍

Veratil and others added 16 commits June 19, 2021 02:16

refactor mouse events

ba221dd

moved tail check

c0eb766

Changes mousePressEvent variables to camelCase

d6f1fa5

Changes variable names from PianoRoll::mousePressEvent to camelCase and changes 2 comments.

refactor doubleclick event

5a376a3

Style fixes on mouseDoubleClickEvent

df738ed

Removes magic number and costy loop

d1e6775

Removes a loop that removed every note except one and replaced it with a clear() call plus appending the note. Removes a magic number used for finding the closest note.

refactor mousemove event

00d2b5d

Some code cleanup

aa40f20

Cleanup on aisle mouseMoveEvent

c8714cd

Lots of cleanup

7571e81

Little changes

d6f008c

use aftertouch on piano keys

301fdbb

fit aftertouch and marked semitones on zoom

49b5789

add moving notes in select mode

231e38d

Veratil mentioned this pull request Jun 19, 2021

1.3 bug in piano-roll shortcut #6039

Closed

PhysSong reviewed Jun 26, 2021

View reviewed changes

allejok96 mentioned this pull request Aug 26, 2021

Demo: Piano-Roll chop, stamp, strum, bulldozer and mode switcher #6140

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Piano Roll mouse refactor + extras #6063

WIP: Piano Roll mouse refactor + extras #6063

Veratil commented Jun 19, 2021

LmmsBot commented Jun 19, 2021 •

edited

Loading

Linux

Windows

macOS

PhysSong left a comment

PhysSong Jun 20, 2021

PhysSong Jun 20, 2021

Veratil Jun 26, 2021

PhysSong Jun 20, 2021

Veratil Jun 26, 2021

Veratil Jun 26, 2021

PhysSong Jun 26, 2021

Veratil Jun 26, 2021

PhysSong Jun 26, 2021

Veratil Jun 26, 2021

PhysSong Jun 26, 2021

Veratil Jun 26, 2021

		const int visible_space = keyAreaBottom() - keyAreaTop();
		m_totalKeysToScroll = qMax(0, NumKeys - 1 - visible_space / m_keyLineHeight);

WIP: Piano Roll mouse refactor + extras #6063

Are you sure you want to change the base?

WIP: Piano Roll mouse refactor + extras #6063

Conversation

Veratil commented Jun 19, 2021

LmmsBot commented Jun 19, 2021 • edited Loading

Linux

Windows

macOS

PhysSong left a 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

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

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

LmmsBot commented Jun 19, 2021 •

edited

Loading