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

Changed bar lines to follow snap size #7034

Merged
merged 18 commits into from
May 20, 2024
Merged

Conversation

BoredGuy1
Copy link
Contributor

@BoredGuy1 BoredGuy1 commented Dec 29, 2023

Overview
Vertical lines now appear or disappear depending on quantization value.

Vertical lines between bars are 1 pixel wide with no emboss. Vertical lines at the end of bars are now 2 pixels wide with 1 pixel emboss to make them stand out. (MIN_PIXELS_PER_BAR has been increased from 2 to 4 to compensate.)

To account for small quantization values, vertical lines between bars will automatically disappear as the user zooms out. (Vertical lines at the end of bars will not disappear.)

Related Threads
#5111, #6420

Screenshots
2 bar quantization (big lines appear every other bar)
2 bar
1/2 bar quantization (big lines appear every bar, one small line appears within each bar)
half bar
1/16 bar quantization (big lines appear every bar, 15 small lines appear within each bar)
16th bar

@Rossmaxx
Copy link
Contributor

Good job. Nice to have this feature implemented but it would help if you could change the defaults too acc to #6420. IMO, 1/16 should be the default snapping with a bit of zoom in. That's what most daws follow these days.

@BoredGuy1
Copy link
Contributor Author

Alright, I'll do that.

@BoredGuy1
Copy link
Contributor Author

Changed the colors a bit to make the coarse grid stand out more. The coarse grid, fine grid, and horizontal line colors can all be individually configured as QSS properties. The line widths are defined as constants in TrackContentWidget.cpp.
image

@BoredGuy1
Copy link
Contributor Author

The coarse grid width, fine grid width, emboss width, and horizontal line width are all configurable now as QSS properties, as well as the offset of the emboss. The default themes have been updated accordingly.

This does break backwards compatibility with old 1.2 themes, but I assumed that that was going to happen sooner or later with 1.3.

image

@BoredGuy1 BoredGuy1 marked this pull request as draft February 5, 2024 15:32
@BoredGuy1 BoredGuy1 marked this pull request as ready for review February 5, 2024 15:33
@Rossmaxx
Copy link
Contributor

Requesting review from @michaelgregorius

src/gui/tracks/TrackContentWidget.cpp Show resolved Hide resolved
include/TrackContentWidget.h Outdated Show resolved Hide resolved
src/gui/tracks/TrackContentWidget.cpp Outdated Show resolved Hide resolved
src/gui/tracks/TrackContentWidget.cpp Outdated Show resolved Hide resolved
@michaelgregorius
Copy link
Contributor

Hi @BoredGuy1,

thanks for your PR! I really like that the Song Editor now defaults to a more zoomed in view!

I'd like to propose to use a default quantization of 1/4 for the Song Editor though because it makes it look less "busy". It would also coincide with the tempo which is BPM, i.e. 1/4 notes per minute.

Heres a visual comparison of the two options:

SongEditor-1-16
SongEditor-1-4

Using 1/4 is also the default in REAPER to compare against another DAW:

SongEditor-REAPER-Comparison

@BoredGuy1
Copy link
Contributor Author

BoredGuy1 commented May 6, 2024

Hey @michaelgregorius, thanks for the review. And sorry for the late response (been busy).

I agree, default 1/4 definitely looks cleaner than 1/16. That should be an easy change. I think I'll be able to add that (along with all the requested changes) in the coming week.

@BoredGuy1
Copy link
Contributor Author

OK, everything should be good now.

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

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

Just one comment on coding convention.

data/themes/classic/style.css Show resolved Hide resolved
@zonkmachine zonkmachine dismissed their stale review May 8, 2024 17:39

Already been commented on before.

include/ComboBoxModel.h Outdated Show resolved Hide resolved
include/TrackContentWidget.h Outdated Show resolved Hide resolved
src/gui/tracks/TrackContentWidget.cpp Outdated Show resolved Hide resolved
@BoredGuy1
Copy link
Contributor Author

Ready for review again.

@michaelgregorius
Copy link
Contributor

Ready for review again.

Still looks good to me. @DomClark, @zonkmachine and @Rossmaxx: Is it also good for you? There only seem to be resolved comments now.

@Rossmaxx
Copy link
Contributor

Yeah it's fine with me. Tbh 1/16 was too cluttered to begin with. Thanks to Michael to suggest the 1/4 snapping. I didn't know that 1/4 was supposed to be better.

LGTM

@michaelgregorius michaelgregorius merged commit a527427 into LMMS:master May 20, 2024
9 checks passed
@michaelgregorius
Copy link
Contributor

Pull request #7271 fixes some performance problems that got introduced by this pull request due to missing initialization of some variables.

Rossmaxx pushed a commit that referenced this pull request May 21, 2024
* Added lines in between bars
* Changed bar lines to follow snap size
* Changed default zoom and quantization value
* Added constants for line widths
* Added QSS configuration for new grid line colors
* Tied line widths to QSS properties
* Changed default quantization to 1/4
* Removed clear() from destructor model
* Removed destructor in ComboBoxModel.h
* Changed member set/get functions to pass by value
* Updated signal connection with newer syntax
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.

5 participants