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

Incorporate DrumKeyboardScreen from alter-alter fork #112

Merged
merged 12 commits into from
Jun 30, 2023

Conversation

m-m-adams
Copy link
Collaborator

@m-m-adams m-m-adams commented Jun 27, 2023

To do - limit horizontal scroll behaviour to only scroll to pads that have drums mapped

based on discussion on discord changed goal to add a separate vertical scroll and move the existing behaviour to horizontal scroll instead

@jamiefaye
Copy link
Collaborator

Can you have a look at the merge conflicts here?

@m-m-adams
Copy link
Collaborator Author

Yeah, I'll have to figure out how to rebase this on the new folder structure

alter-alter and others added 3 commits June 28, 2023 16:14
fix InstrumentClipView

recording

Fixed Affect Entire button,  improved  rendering for drum pads

improved select knob on dyumkeyboad

Fixed DrumKeyboardScreen

Fixed a crash issue when turning the horizontal encoder.
@m-m-adams m-m-adams marked this pull request as ready for review June 29, 2023 22:57
@m-m-adams
Copy link
Collaborator Author

Still using the core logic from alter-alter's fork, scrolling horizontally now rotates the drum view like before while vertical scrolling jumps by 4 (thus appearing to go vertically)

Also removed the use of 1 as a magic number to indicate default velocity, replaced with a define outside of the 1-127 range of possible midi values to avoid future conflicts

Copy link
Collaborator

@sapphire-arches sapphire-arches left a comment

Choose a reason for hiding this comment

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

Overall looks fine. A few style nits.

The use of uiNeedsRendering seems to be a little excessive; I suspect most of them are redundant/not needed but don't really have a great way to prove that. If you want to spend time removing them we could probably get a few hundred cycles back but it might not be worth doing if stellar_aria is about to redo all the UI code anyway.

src/deluge/gui/ui/keyboard_screen.cpp Outdated Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Outdated Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Outdated Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Outdated Show resolved Hide resolved
src/deluge/gui/ui/keyboard_screen.cpp Outdated Show resolved Hide resolved
src/deluge/gui/views/instrument_clip_view.cpp Outdated Show resolved Hide resolved
src/deluge/gui/views/instrument_clip_view.cpp Outdated Show resolved Hide resolved
src/deluge/gui/views/view.cpp Show resolved Hide resolved
Copy link
Collaborator

@sapphire-arches sapphire-arches left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my fiddly nitpicks, I think this broadly looks OK now. Really excited to have it "upstream"!

@litui litui added the enhancement New feature or request label Jun 30, 2023
@litui litui merged commit 5d7ef08 into SynthstromAudible:community Jun 30, 2023
5 checks passed
@m-m-adams m-m-adams deleted the drum_velocities branch July 4, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants