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

[Equalizer] Bright analyzer colors, opacity increased #4772

Merged
merged 7 commits into from Apr 6, 2019

Conversation

Projects
None yet
6 participants
@GingkathFox
Copy link
Contributor

commented Jan 14, 2019

I dont really like how dark and transparent the analyzer colors are. For me, it's hard to actually see the difference between the input and the output. This pull changes the in color and makes both colors a bit more opaque.

GingkathFox added some commits Jan 14, 2019

[Equalizer] Bright analyzer colors, opacity incr
Brightened spectrum analyzer colors and increased opacity a tad to make more visible
@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

@GingkathFox GingkathFox reopened this Jan 15, 2019

@tresf

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

With any UI-only changes, a before and after helps.

Before:
screen shot 2019-01-14 at 10 34 33 pm

After:
screen shot 2019-01-14 at 10 38 10 pm

@PhysSong PhysSong requested review from RebeccaDeField and Umcaruje Jan 15, 2019

@Spekular

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

@tresf

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Let's please focus on accepting or denying this PR. If the OP's concerns are valid, focus on on them. Enhancements described above are out of scope of our stable branch.

@Spekular

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

@tresf

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Enhancements described above are out of scope of our stable branch.

Which part of this isn't relevant to the PR?

Out of scope does not negate relevance, but establishing scope forces decision.

  • Do you agree the current colors are too transparent
  • Do you agree the current colors are hard to differentiate

The PR touches two lines of code and makes no mention of making things colorless, making the bands affect color, etc. The points are extremely relevant, but they're out of scope, mostly because they lead to "endless" discussion.

Let's focus on @GingkathFox's PR and how he can make it better please.

@RoxasKH

This comment has been minimized.

Copy link

commented Jan 15, 2019

In my personal opinion, i really like the actual visualizer colors. I think the new ones are a bit more confusionary, also they don't fit the UI so much. So i'd maintain the actual ones, but it's not only my decision.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@Spekular Oh crap, i think i accidentally made both blue. The inputs supposed to be a green color, lemmie change that.
@tresf cmake and make on MacOS hate me, absolutely nothing will build without a fatal error of some sort. Ill see if i can build on Ubuntu.
@RoxasKH see above xp

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Lemmie try to build on Ubuntu, ill get back to yall in a bit.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

Wasnt able to build, ill try something tomorrow.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

I cant get make to install on ubuntu 18.04, sorry. Can someone check the color change for me?

GingkathFox added some commits Jan 27, 2019

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

In:
In
Out:
Out
Both:
Both

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

@GingkathFox thank you for your work on this. I do not object to brightening the colors or changing to blue/green. Can you update these colors to be the same hue of green and blue that we use in the theme? You can find the palette here and brighten the colors however you see fit so long as the hue and saturation stays the same.

Note that blue and green aren't that far apart on the color wheel so if you're looking for a lot of contrast other color combinations might be better, but both of these colors will display brighter than purple.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@RebeccaDeField i dunno, i didnt really like the blue&green combo. ill try it.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

screen shot 2019-01-31 at 9 01 19 pm
eeh, doesnt look any different. ew.

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@GingkathFox I didn't mean to use the old colors, just the same hues as any in the existing palette. Blue and green should still be alright as long as it has enough contrast (one is lighter than the other) and is brighter to fulfill the purpose of this pr 👍

Edit: there is a plugin which uses the orange/red combination which is brighter already implemented as an option for a different color combination.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

im colorblind, so they both look red to me XD
how do i change the hue of a rgb value? @RebeccaDeField

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@GingkathFox would be a little more complicated to change the hue using RGB. In inkscape, try switching to HSL in fill and stroke.
dialog_fillstroke_hsl

Then only edit the Lightness value, leave the hue/saturation and you should be good to go.

I think our best bet in a color combination that will work for everyone is going to be a brighter blue/green but I am open to other combinations if it works.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

im running MacOS >w<
ive been using this, maybe i can just change the hue of the HSL value and copy the RGB? @RebeccaDeField

@tresf

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

In inkscape, try switching to HSL in fill and stroke.

im running MacOS >w<

Translation: macOS can't run Inkscape (they claim it works with X11 server, but it doesn't). @GingkathFox you have a Linux VM, muscle up. 💪 😄

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

@tresf i dont wanna reboot pc to switch thoooo XDDD

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

ive been using this, maybe i can just change the hue of the HSL value and copy the RGB?

@GingkathFox Any color program using HSL will work fine, only assumed inkscape because that's where the source files for the colors are contained but the program choice is arbitrary as long as you can grab the colors you need.

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

@RebeccaDeField ok cool!

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

In:
in
Out:
out
In&Out:
in out
I dont like the in hue >w<

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

@GingkathFox This looks good, but I am thinking that the In color should be brighter given the purpose fo the PR?

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

XD now i gotta mess with the in color again! Time to fire up Colorizer and ill push the changes later today @RebeccaDeField

@GingkathFox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

In:
in
Pushing commit now

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

Given that this matches our existing colors and it looks more visible than the previous version of the equalizer, I think we've accomplished our goal here and this has my approval from the design dept.

@RebeccaDeField

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Some checks not successful. Anything we can do to fix that or is this ok to merge? Hoping to get this cleared out of the pr requests as it is a pretty minor change.

@PhysSong

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Some checks not successful. Anything we can do to fix that or is this ok to merge?

The failure looks unrelated to changes in this PR. I restarted the Travis-CI build and it should pass in 30 minutes.

@RebeccaDeField RebeccaDeField merged commit 82e3ba7 into LMMS:stable-1.2 Apr 6, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.