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

Improves the PianoRoll detuning drawing #5928

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

IanCaio
Copy link
Contributor

@IanCaio IanCaio commented Mar 2, 2021

This PR rewrites the detuning drawing method of the PianoRoll to behave similar to the AutomationEditor. It now draws Cubic Hermite progressions as well, and the detuning is displayed as a shape instead of a line.

The color of the detuning shape is the same as m_noteColor, except it's lighter and has some alpha.

Detuning

There's one visual glitch left to fix, which is also present in master: Changes to the automation node's outValues don't immediately trigger a repaint on the PianoRoll, because they are done directly on the AutomationNode class which doesn't emit the dataChanged() signal on the Automation Pattern. This is an easy fix, once it's decided how to approach it. Waiting for some opinions on the Discord channel before working on the fix.

	This PR rewrites the detuning drawing method of the PianoRoll to
behave similar to the AutomationEditor. It now draws Cubic Hermite
progressions as well, and the detuning is displayed as a shape instead
of a line.

	The color of the detuning shape is the same as m_noteColor,
except it's lighter and has some alpha.
@LmmsBot
Copy link

LmmsBot commented Mar 2, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://12717-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg0215ce709-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12717?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12716-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg0215ce709-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12716?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/08r51o3s4fump0eo/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38031731"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/gq3bv6g3o7qwnmfr/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38031731"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12718-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg0215ce7-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12718?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12719-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg0215ce709-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12719?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "d6f9f9dc41e765704c726449637f094f30b2eaae"}

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@cyber-bridge
Copy link
Contributor

I've tried to compile it but it failed (Qt version 5.15.2):

/home/unknown/lmms/detuningDrawing/src/gui/editors/PianoRoll.cpp:1046:16: error: aggregate 'QPainterPath path' has incomplete type and cannot be defined
 1046 |   QPainterPath path;
      |                ^~~~

After adding #include <QPainterPath> to src/gui/editors/PianoRoll.cpp it did compile.

Looks great! :-D

	Moves lambda functions up, uses keyAreaTop() instead of
PR_TOP_MARGIN, and include missing header (<QPainterPath>).
@IanCaio
Copy link
Contributor Author

IanCaio commented Mar 2, 2021

I've tried to compile it but it failed (Qt version 5.15.2):

/home/unknown/lmms/detuningDrawing/src/gui/editors/PianoRoll.cpp:1046:16: error: aggregate 'QPainterPath path' has incomplete type and cannot be defined
 1046 |   QPainterPath path;
      |                ^~~~

After adding #include <QPainterPath> to src/gui/editors/PianoRoll.cpp it did compile.

Looks great! :-D

Added the missing header! I've no idea how it compiled without complaints here

@cyber-bridge
Copy link
Contributor

Added the missing header! I've no idea how it compiled without complaints here

I can only guess that your version of Qt is below 5.15. Probably this commit is responsible: https://code.qt.io/cgit/qt/qtbase.git/commit/?id=50d2acdc93b4de2ba56eb67787e2bdcb21dd4bea

@AaronRecord
Copy link

AaronRecord commented Mar 7, 2021

I personally like lines better than the shapes but other than that it looks good. I guess the shapes might make it easier to tell which note the detune belongs to

@superpaik
Copy link
Contributor

A part from the little glitch thing you commented, I think that if we use shapes (I like it better than lines, because is in line with the automation detuning editor), we have to modify how it is drawing because it can lead to user confusion. If I'm not wrong, both detuning automations (see attached images) have the same shape in the piano roll editor. One solution would be having in the piano roll the same shape as the automation editor or another, to highlight the shape contour according the automation (second image. Don't take into account the bad quality of the mock-up, it's just an idea ;-)
imatge
imatge

@IanCaio
Copy link
Contributor Author

IanCaio commented Mar 8, 2021

A part from the little glitch thing you commented, I think that if we use shapes (I like it better than lines, because is in line with the automation detuning editor), we have to modify how it is drawing because it can lead to user confusion. If I'm not wrong, both detuning automations (see attached images) have the same shape in the piano roll editor. One solution would be having in the piano roll the same shape as the automation editor or another, to highlight the shape contour according the automation (second image. Don't take into account the bad quality of the mock-up, it's just an idea ;-)

True, well noticed! Highlighting the contour might be a little tricky because even though it looks like one single shape, the drawings are actually many shapes close together, but I'd have to look into it to be sure. Maybe there's a way to add that contour that is not really complicated.

But I thought of an alternative too, which would be extending the shape until the end of the note. Then the first one would continue at C5 until the end and the second one would have that discrete jump and continue on top of the note. What do you think?

@superpaik
Copy link
Contributor

Yes, that'd be great! But have in mind that user can change the note duration once automation has been established, so it would have to change accordingly, and maybe that is not that easy. But if possible, I think it's a good solution.

@IanCaio
Copy link
Contributor Author

IanCaio commented Mar 8, 2021

True, once I get some time to work on it I'll check which one will add less complexity: Reworking the path generation so I can have a single path and then use the outline or drawing the detuning all the way to the end of note. The latter might not even require that I worry about the note resizing because the Piano Roll redrawing is connected to the Pattern dataChanged signal, so every time a note is resized the Piano Roll will redraw anyways, and the detuning shape will be redrawn too (now accounting to the resized note)

@DomClark DomClark added the after-refactor PRs that will need to be rebased after the planned code refactor (#5592) label Mar 12, 2021
@zonkmachine
Copy link
Member

detune

Bloody brilliant, want this! Glitches that are already in LMMS doesn't have to be fixed here for a merge. That's my opinion anyway.

	Fixes a new conflict on PianoRoll.cpp
@zonkmachine
Copy link
Member

or drawing the detuning all the way to the end of note.

Maybe extend the detuning to the end of the note release and let the drawing fade too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-refactor PRs that will need to be rebased after the planned code refactor (#5592)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants