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

Fix drawNoteRect misalignments #5881

Merged
merged 2 commits into from Mar 12, 2021
Merged

Fix drawNoteRect misalignments #5881

merged 2 commits into from Mar 12, 2021

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Jan 17, 2021

Partially Fixes #5863 (Notes misaligning, not keys visual glitch)

Tested with ghost notes, detuning, scrolling vertical and horizontal, and resizing the note edit area. Everything seems to be working as normal now for notes.

@LmmsBot
Copy link

LmmsBot commented Jan 17, 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! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12854-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bg4dfeae4-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12854?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://12855-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bg4dfeae488-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12855?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12851-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bg4dfeae488-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12851?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/5wtslsi4tgj8mnep/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38103737"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/cm9gpe5v1v59p2r3/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38103737"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12852-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bg4dfeae488-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12852?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "3869e30e3374462002e0c0ddc267f88dcac53849"}

@PhysSong
Copy link
Member

Code looks good. @RoxasKH Could you test this and confirm the fix?

@JohannesLorenz JohannesLorenz added the needs testing This pull request needs more testing label Jan 23, 2021
@RoxasKH
Copy link
Contributor

RoxasKH commented Jan 23, 2021

Code looks good. @RoxasKH Could you test this and confirm the fix?

Just tested it and it seems it's still buggy to me. It'd be better if someone else could test it, too, tho.

https://streamable.com/uvtzh6

@kalinushkin
Copy link

Code looks good. @RoxasKH Could you test this and confirm the fix?

Just tested it and it seems it's still buggy to me. It'd be better if someone else could test it, too, tho.

https://streamable.com/uvtzh6

Well, i tested it, and i find it correct working. "As by design" 👍

@RoxasKH
Copy link
Contributor

RoxasKH commented Jan 23, 2021

Code looks good. @RoxasKH Could you test this and confirm the fix?

Just tested it and it seems it's still buggy to me. It'd be better if someone else could test it, too, tho.
https://streamable.com/uvtzh6

Well, i tested it, and i find it correct working. "As by design" 👍

No idea why it still happens to me, but i gotta admit i don't install properly LMMS versions to test them.
Maybe could also be related to resolution as i'm still on a 720p screen, but probably not.

@Veratil
Copy link
Contributor Author

Veratil commented Jan 23, 2021

Code looks good. @RoxasKH Could you test this and confirm the fix?

Just tested it and it seems it's still buggy to me. It'd be better if someone else could test it, too, tho.

https://streamable.com/uvtzh6

This is for the notes, the artifacting on the piano keys is another issue I noticed that I'll look into later.

@RoxasKH
Copy link
Contributor

RoxasKH commented Jan 23, 2021

Code looks good. @RoxasKH Could you test this and confirm the fix?

Just tested it and it seems it's still buggy to me. It'd be better if someone else could test it, too, tho.
https://streamable.com/uvtzh6

This is for the notes, the artifacting on the piano keys is another issue I noticed that I'll look into later.

Oh, thanks to point this out, i actually created the issue for the artifacts of the pianoroll keys so i never really noticed the notes bug while resizing.

Seems still a little buggy, but mostly unnoticeable.

https://streamable.com/d625ie

@superpaik
Copy link
Contributor

In windows it doesn't work properly. When resizing the keys movement aren't smooth and sometimes they are keep unaligned . They get aligned again when the cursor enters the piano roll window.
imatge

@Veratil
Copy link
Contributor Author

Veratil commented Jan 25, 2021

In windows it doesn't work properly. When resizing the keys movement aren't smooth and sometimes they are keep unaligned . They get aligned again when the cursor enters the piano roll window.
imatge

This PR is for the notes themselves, the piano keys I have to look into separately.

@PhysSong
Copy link
Member

@Veratil Do you want to fix piano keys in this PR or in a separate PR?

@Veratil
Copy link
Contributor Author

Veratil commented Jan 29, 2021

@Veratil Do you want to fix piano keys in this PR or in a separate PR?

I haven't figured out the reason for the piano keys glitch yet, but that will be a separate PR.

@PhysSong
Copy link
Member

PhysSong commented Feb 20, 2021

@Veratil Isn't the drawDetuningInfo call at line 3224 supposed to be fixed as well? Also, could you extract (topKey - note->key()) * m_keyLineHeight + keyAreaTop() - 1 to a variable to avoid repetitions and potential mistakes?

@Veratil
Copy link
Contributor Author

Veratil commented Feb 20, 2021

@Veratil Isn't the drawDetuningInfo call at line 3224 supposed to be fixed as well?

I didn't seem to have any issues with detuning draw, but I can double check it again.

Also, could you extract (topKey - note->key()) * m_keyLineHeight + keyAreaTop() - 1 to a variable to avoid repetitions and potential mistakes?

Yeah that's reasonable. It'd have to be a lambda though since note->key() changes within the for loop.

@PhysSong
Copy link
Member

This can potentially conflict with #5928, FYI.

@Veratil
Copy link
Contributor Author

Veratil commented Mar 11, 2021

This can potentially conflict with #5928, FYI.

Double checking the diff for 5928, I don't see it touching these lines. It also still calculates the midpoint of the note based on the Y position given, so afaict we're safe on both. 👍

@Spekular
Copy link
Member

Spekular commented Mar 11, 2021

I can confirm the issue Roxas encountered, but I think it may be the octave lines that are bugged, since they stay aligned with the buggy keyboard. Dragging the bottom of the piano roll window down exposes more and more of the next lowest key until a full key is visible, at which point the view jumps to expose the next higher key instead. Regardless of whether this is contributing to the octave line/note desync, it's rather unpleasant. The same occurs when dragging the top of the piano roll up.

In 1.2, the new notes at the top are smoothly revealed.

Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

Code wise changes LGTM. As for testing, piano roll resizing is still buggy, but this is an improvement over the existing behavior on master.

@Veratil
Copy link
Contributor Author

Veratil commented Mar 11, 2021

I can confirm the issue Roxas encountered, but I think it may be the octave lines that are bugged, since they stay aligned with the buggy keyboard. Dragging the bottom of the piano roll window down exposes more and more of the next lowest key until a full key is visible, at which point the view jumps to expose the next higher key instead. Regardless of whether this is contributing to the octave line/note desync, it's rather unpleasant. The same occurs when dragging the top of the piano roll up.

I'll see what I can debug to prevent this state.

In 1.2, the new notes at the top are smoothly revealed.

I agree smooth reveal is better, and I'll aim to get back to that. 👍 For now we're slowly just fixing and cleaning the old, slow, and poorly maintained code. One step back, two steps forward. 😄

@Veratil Veratil merged commit 266ad8d into LMMS:master Mar 12, 2021
@Veratil Veratil removed the needs testing This pull request needs more testing label Mar 12, 2021
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.

Pianoroll resizing is graphically broken in LMMS 1.3
8 participants