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

Piano Roll and Automation editor grid redesign (w/ @BaraMGB) #3062

Merged
merged 16 commits into from
Feb 22, 2017

Conversation

Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Oct 3, 2016

Fixes #3019 and #2308

Thank you very much @BaraMGB for collaborating on this Pull Request with me and helping me out with my problems.

screenshot from 2016-10-03 20 34 37
screenshot from 2016-10-03 20 34 18

The grid is ligher, the background alternates shades. I also did a general CSS cleanup, fixed the indentation and stuff.

Feedback and testing welcome.

@Umcaruje Umcaruje force-pushed the lightergrid branch 2 times, most recently from d4a4ea8 to ac4c44c Compare October 3, 2016 18:51
@BaraMGB
Copy link
Contributor

BaraMGB commented Oct 3, 2016

That looks really great, now.

@simonvanderveldt
Copy link
Contributor

Could it also solve this one (or include a specific fix for it)? #2832

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 4, 2016

Could it also solve this one (or include a specific fix for it)? #2832

To allow seperate colors for different lines? seems like a overkill to me.

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 4, 2016

Conflicts resolved, rebased.

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 5, 2016

@musikBear your feedback would be valuable on this.

@BaraMGB
Copy link
Contributor

BaraMGB commented Oct 6, 2016

Could it be worth to shade horizontal lines which represent a black key? I noticed that in other programs.

@musikBear
Copy link

@Umcaruje Very good! Also in respect to the vertical grid-lines. I think this will be pleasant to work with
🍾 👍

{
if ( (barCount + leftBars) % 2 != 0 )
{
p.drawRect( x - m_currentPosition, PR_TOP_MARGIN, m_ppt,
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I've got a green line on top of the shading rect:
2016-10-06-143342_1920x1080_scrot

What helps is:

p.fillRect( x - m_currentPosition, PR_TOP_MARGIN, m_ppt,
                    height() - ( PR_BOTTOM_MARGIN + PR_TOP_MARGIN ), backgroundShade() );

by removing the : p.setBrush( backgroundShade() ); of course.

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 6, 2016

Good catch @BaraMGB, will fix asap.

Glad you like it @musikBear. Can we close out the issue where its asked
that the tick lines have a different color? Or do you think this still does
not help in that sense

On 6 Oct 2016 14:38, "BaraMGB" [email protected] wrote:

@BaraMGB requested changes on this pull request.

In src/gui/editors/PianoRoll.cpp
#3062 (review):

@@ -2863,6 +2870,22 @@ void PianoRoll::paintEvent(QPaintEvent * pe )

bool show32nds = ( m_zoomingModel.value() > 3 );

  • // alternating shades for better contrast
  • // count the bars which disappear on left by scrolling
  • int barCount = m_currentPosition / MidiTime::ticksPerTact();
  • int leftBars = m_currentPosition / m_ppt;
    +
  • p.setBrush( backgroundShade() );
    +
  • for ( int x = WHITE_KEY_WIDTH; x < width() + m_currentPosition; x += m_ppt, ++barCount )
  • {
  • if ( (barCount + leftBars)  % 2 != 0 )
    
  • {
    
  •         p.drawRect( x - m_currentPosition, PR_TOP_MARGIN, m_ppt,
    

Okay, I've got a green line on top of the shading rect:
[image: 2016-10-06-143342_1920x1080_scrot]
https://cloud.githubusercontent.com/assets/6502580/19152379/00306bf0-8bd2-11e6-852c-cfeaa2835b45.png

What helps is:

p.fillRect( x - m_currentPosition, PR_TOP_MARGIN, m_ppt,
height() - ( PR_BOTTOM_MARGIN + PR_TOP_MARGIN ), backgroundShade() );

by removing the : p.setBrush( backgroundShade() ); of course.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3062 (review), or mute
the thread
https:/notifications/unsubscribe-auth/AF_bPYXWjzNtR4tni3EAROMiV6-gzjgPks5qxOvhgaJpZM4KM5wg
.

@musikBear
Copy link

@Umcaruje afaik, you have implemented css option for grid, and my poor vision can not be a hallmark for the design. I can fiddle with the css, and the current design you have here is clearly much better. The vertical bar-dividers are still a bit difficult to see, but yes. If noone else feel the same about bar-dividers, then this is a fine design 👍
[ I am not sure i ever really got to explain the triplet-line idea for bar-dividers, so i will just try with one more drawing, because there was never different colors in that idea, just two additional lines on each side of the current bar-divider-line, so in total 3 lines

I have tried to explain the tripple-line bar-divider WITH color, but that just to explain the idea
tripletdivider

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 7, 2016

@BaraMGB fixed it.

@Umcaruje
Copy link
Member Author

Umcaruje commented Oct 7, 2016

@musikBear here's how the grid looks with your proposal. Seems like an overkill to me.
screenshot from 2016-10-07 21 43 25

@simonvanderveldt
Copy link
Contributor

Hmm, the 3 lines might be a bit excessive.

But on the other hand, especially in the automation editor the distinction between the bar and and tick lines is pretty minimal (like in your screenshot https://cloud.githubusercontent.com/assets/6282045/19049224/d129f1c0-89a9-11e6-82cb-1d2cb7766374.png).
It would be nice of those lines were a bit easier to distinguish from eachother.

@BaraMGB
Copy link
Contributor

BaraMGB commented Oct 8, 2016

If the discussion is over, this one can be merged. @musikBear @simonvanderveldt ?

@simonvanderveldt
Copy link
Contributor

@BaraMGB I just tried this branch locally and for me the extra shading of every 2nd bar makes LMMS feel a bit "busy" (hope you know what I mean). I definitely prefer it without the shading.

I do think a better visible grid is a necessity though. This goes for both the normal horizontal (notes) and vertical (ticks) lines and especially for the "major" ticks.
For me the bar start/end is clearly visible, but the major ticks like 1.2, 1.3, etc aren't.

Best example of what works really well for me/I really like is this:
https://www.image-line.com/support/FLHelp/html/img_shot/pianoroll_draghandle.png

@Umcaruje Any clue if this will clash with darker horizontal lines for the black keys vs lighter horizontal lines for the white keys?

@Umcaruje
Copy link
Member Author

I'm going to merge this in a couple of hours if there are no objections.

@simonvanderveldt
Copy link
Contributor

simonvanderveldt commented Oct 11, 2016

I'm going to merge this in a couple of hours if there are no objections.

I think we should address the root cause that currently makes it difficult to distinguish bars (and ticks) from each other, which IMHO is that the grid's visibility isn't good enough. This is pretty much as posted in #3019

I'm not a fan of adding extra stuff (in this case visual overhead) to fix issues whilst the root issue isn't solved. I do see that #2308 specifically requests highlighting for odd bars, but no one asked why that request was made.

@Umcaruje
Copy link
Member Author

Ok then, I'll work on this more 👍

@musikBear
Copy link

@Umcaruje Sorry i have been 'Off' for a week, so i have not been following your work. I have to say that i kind of like the image with the triplet line, but i also understand that you feel it does not suit your overall design. Design and 'taste' is darn difficult :) Very good work, though 👍

@Umcaruje
Copy link
Member Author

Update: Automation editor has the same grid now, and shading is more visible.

Waiting for color guidelines from @RebeccaDeField and this can be tested and merged.
image
image

@musikBear
Copy link

musikBear commented Feb 12, 2017

Update: Automation editor has the same grid now, and shading is more visible.

@Umcaruje it is visible now, but inside the blue area in automation, i cant see it ( -It is ofcause also because of the graduated alpha, which i know is easy to disable :)
Is the intensity of the bar shading also modifiable in css?
The me and other bats, can just do that, and i will not comment the faint default grid again :)

@Umcaruje
Copy link
Member Author

Is the intensity of the bar shading also modifiable in css?

Yes, absolutely everything is modifiable in css. You can easily change the opacity of the shading part.

@tresf
Copy link
Member

tresf commented Feb 12, 2017

@musikBear has a good point. This may be worthy running through a colorblind test. Here's some numbers...

an all-boys school in the Home Counties of England with 1000 pupils would have approximately 100 colour deficient students. 12-13 would be deuteranopes, 12-13 would be protanopes, 12-13 would have a form of protanomaly and 62 would have a form of deuteranomaly. About half of those with an anomalous condition would have a moderate to severe form of deficiency.

@tresf
Copy link
Member

tresf commented Feb 12, 2017

That said, I love it. :)

@Umcaruje
Copy link
Member Author

After some help from @RebeccaDeField this is the current look:
screenshot from 2017-02-13 00 37 53
screenshot from 2017-02-13 00 37 39

This PR is finally ready for merging and review.

@Umcaruje
Copy link
Member Author

@tresf @musikBear @vlad1777d If this is looking good, I'll merge.

@musikBear
Copy link

@Umcaruje When you told me that i can work with it in CSS, im absolutely satisfied! Great job! 🎆

@tresf
Copy link
Member

tresf commented Feb 14, 2017

@Umcaruje I have not reviewed the code, but the UI without a doubt fixes #3019. 👍

@Umcaruje Umcaruje merged commit de2e164 into LMMS:master Feb 22, 2017
@vlad0337187
Copy link

@Umcaruje , It looks great, especially violent automation line =)

@vlad0337187
Copy link

vlad0337187 commented Mar 9, 2018

@Umcaruje , sorry that I write not in appropriate place, but I didn't found any IRC channels or something like this to ask question,
can I change colors of automation editor's grid without rebuilding LMMS ?

(also I'd like to try adjusting colors of automation graph)

Seems, it's not so clear as Piano Roll's grid.

@zonkmachine
Copy link
Member

I didn't found any IRC channels or something like this to ask question,

Here: https://discord.gg/3sc5su7
Especially the channel named #support

@musikBear
Copy link

Here:

Well yes, but any explanation done in discord, is lost after the discord session. So the time used in discord to explain a feature, benefits exactly one person.
Written down, it could benefit n persons. 🙊

@Spekular
Copy link
Member

@musikBear nope, Discord has persistent chat history. Besides, mentioning "consensus on Discord is ___" is writing it down for the benefit of those on github.

@Umcaruje
Copy link
Member Author

@Umcaruje , sorry that I write not in appropriate place, but I didn't found any IRC channels or something like this to ask question,
can I change colors of automation editor's grid without rebuilding LMMS ?

@vlad1777d yeah, you can. Just edit the qproperty-lineColor, qproperty-beatLineColor, qproperty-barLineColor css rules under AutomationEditor.

If you come up with a better colored grid, don't be hesitant to share, I'm always open for improvement on the gui, and your mockups made this PR happen :)

@musikBear
Copy link

Discord has persistent chat history

Searchable? That would render my thought to void, but is it? ..anywitch oftopic so ..

@Spekular
Copy link
Member

Spekular commented Mar 11, 2018 via email

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Make dem grid ndasd

* grid BG

* fix bug in scroll behavior

* debugging scrolling

* Add CSS property, port to automation editor

* Fix a fail

* Spaces to tabs

* Use fillRect rather than drawRect

* Implement @vlad1777d's idea

* Seperate loops and stuff

* Finish up Piano Roll Grid

* Cleanup

* Redesign the grid for the Automation Editor

* Update colors

* Formatting changes

* formatting changes II
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.

8 participants