-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Use the gray color for all abstract classes #80184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of the usability team I really like the concept of greying out the images. Did not technical review.
cdff9be
to
be3a6dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed something. |
be3a6dc
to
9291b46
Compare
I'm a bit concerned about changing the base color of these icons, as the color is meant to reflect the inheritance (red icons all inherit from Node3D, etc.). I think it can make sense to modulate the icon for abstract classes programmatically for the Create New Node dialog, but I'd keep them in a shade that conveys the inheritance, and not modify the base SVGs, so it doesn't impact e.g. the icons visible in the Editor Help or Inspector. |
To add to that, we may sneakily use some of these icons for other visual elements where it can be important to preserve the color to convey the context/meaning. |
This was actually my initial idea. I'm not against this per-se, but to be devil's advocate:
So I'll resist it a little for now, so we can have some discussion about these pros and cons. @YuriSizov I don't get what you mean. |
These icons may be used for things other than node types in the editor. And in those cases it is very likely important that icons have their assigned color palette, as Akien mentions. |
Why have them programmatically? It's basically 3 colors. There is red (3D), green(Control), blue(2D) basically RGB. If the color isn't at least a shade of the aforementioned 3 colors, as Akien mentioned above, gray degrades the colorcoding intuitiveness. |
Idea: 3 non-programmatical 60% gray colors for GUI, 2D, and 3D. Gray for white icons and everything else |
I'm pretty sure we can find a modulate value that makes it look desaturated without having to change the icons. |
If we have a custom color for abstract 2D, 3D, and GUI nodes, then I'd prefer to have it in the icons so their appearance is consistent everywhere you see that icon. IMO the only good point was about their color communicating something about their inheritance, and this would solve it. |
Related: #794 We shouldn't be using solely color to convey inheritance |
I saw that proposal, but my personal conclusion was as follows:
So then we're left with 10-15 nodes that share the same icon between blue and red. It seems like an obvious conclusion that a different shape improves usability, but I beg to differ:
Because of this, I only appreciate a change of 3D icons when there's an important functional difference it tries to communicate (e.g. CollisionPolygon3D). NavigationObstacle and NavigationAgent use different shapes for their 2D and 3D versions' icons, even though they don't need to, and I think this shows the shortcomings of this whole idea. NavigationAgent3D is painful to recognize at small scales because it has this fancy 3D effect, whereas the simple flat NavigationAgent2D is insta recognizable. So yes, I think improving this for monochromia people makes it way too much worse for everyone else for it to be worth it IMO. And besides, just because colorblind people exist, doesn't mean we shouldn't use colors for usability. It rather means we shouldn't rely on them and consider them more of a last resort, when nothing else works - consider syntax highlighting. And I don't think anything else works here. |
The dimmed colors version looks nice, assuming it's done with modulation. |
No, it's not modulation, it's three new colors in the editor themes that are 60% gray and 40% green/red/blue. I don't really get why we'd want to use modulation, to me it seems better to be consistent everywhere with how we display these classes. It's not entirely gray, so it won't look disabled e.g if it's on a button - so I don't see a problem |
What I said, just in different words ;) |
Not really, because I'm saying essentially "we should use solely color to represent inheritance, because every other way is worse and blue and red are easy to tell apart for everyone" |
9291b46
to
9fcffb5
Compare
Next take: The colors used are a bit different, in a way that should be distinct enough from both plain gray and the node color. No modulation is used, I didn't get any nice colors from applying modulation indiscriminately. It would've also forced a bunch of light vs dark theme checks, so I think it's much nicer to just have 3 dedicated "abstract" colors that sit between gray and the node color (in my case, obtained by getting 1/3 gray, 2/3 node color, and darkening/lightening by 15%) I changed EditorNode's |
9fcffb5
to
7fe09a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the colors in the latest version of the PR. I do wonder if it should be slightly grayer, but now it is easy to tell what dimension these nodes are from.
7fe09a9
to
dfe4896
Compare
I just remembered I had already done something similar with the colors in Node1D: I used faded cyan for CollisionObject1D. So, yeah, I like the colors in this PR. |
I opted for a bit of desaturation because while cyan is a bright color, our 2D blue is darker even than the 50% gray, so abstract blue would have looked bad in some places (as most of our interface is blueish) We don't have abstract 2D classes icons currently, but it might become a thing 🤷 |
dfe4896
to
144f889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
@Calinou Was already in my plans, I just wanted to finally determine what'll happen with abstract node icons. |
144f889
to
6d2d0b6
Compare
This push should resolve the suggested changes. |
6d2d0b6
to
398ca4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks great now!
I've come around to the idea of hardcoding desaturated colors, so I think we can go ahead with this.
Thanks! |
Not entirely this PR's fault, but it did expose something. I'll make a fix. |
This is meant as a small quality of life thing and something that should be "obviously better", so I haven't made a proposal for it yet. The changes in this PR are as follows:
I think this gives a pretty good result overall:
(CollisionShape2D and CollisionShape3D currently appear with a white Object icon)