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

Highlight context menu items at the top of the 2D/3D viewports #35891

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 3, 2020

master version of #51087.

This makes it easier to notice that some menu items only appear when specific nodes are selected.

This change applies to both 2D and 3D editors, including both plugin-based menus and the hardcoded 2D layout/animation contextual menus.

This closes #11566 and possibly other issues/proposals.

Preview

image

Old versions

A blended version of the editor theme's accent color is used for highlighting (25% opacity).

image

Even older versions

Dark theme

3D editor

2D editor + animation

Light theme

2D editor (GUI)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 3, 2020

I'm quite sure this addresses an existing issue/proposal (if not several), but I can't find it anymore.

Could be this godotengine/godot-proposals#368 or this godotengine/godot-proposals#14 maybe.

@arkology
Copy link
Contributor

arkology commented Feb 4, 2020

Is it highlighted all the time? If so, i think blinking several times on change (and then back to usual color) would be better.

@groud
Copy link
Member

groud commented Feb 4, 2020

We discussed at the Godot sprint about that.
The color is fine, though I think I would prefer something more subtle, like an underline.

Also, a consensus was to change the way the top bar is organised. It would be split on two, with sticking to the left the whole list of tools, and sticking to the right the menus. This might look weird with color so it should probably be implemented first and we would see what happens.

@aaronfranke
Copy link
Member

See also: godotengine/godot-proposals#14

@Calinou
Copy link
Member Author

Calinou commented Feb 4, 2020

@arkology Blinking would probably be a bit too distracting. I don't remember seeing blinking being often at all in UIs, except to notify the user of a forbidden action (like clicking outside an exclusive modal window on Windows). It'd also complexify the implementation significantly for little gain 🙂

That said, I can make the background color a tad more subtle or even use an underline as @groud said.

@Calinou
Copy link
Member Author

Calinou commented Feb 4, 2020

Here are some alternative design proposals, with the current one for comparison:

Original

2020-02-04_21 41 03

Subtler background

2020-02-04_21 43 53

Accent color opacity is set to 24% instead of 30%.

Overline

2020-02-04_21 51 50

Underline

2020-02-04_21 54 24

(The Underline and Overline designs take slightly more space and make text no longer aligned. This is due to a StyleBoxFlat quirk I need to iron out.)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 4, 2020

Underline has potential, but could it be thicker?

@Calinou
Copy link
Member Author

Calinou commented Feb 4, 2020

@KoBeWi It could be, but it'd make the spacing problem I mentioned even worse until I find a way to fix it 🙂

I added the underline by calling draw_center(false), set_border_color(EditorNode::get_singleton()->get_gui_base()->get_color("accent_color", "Editor")) then set_border_width(MARGIN_BOTTOM, 2 * EDSCALE) on the StyleBox resource. I then decreased the bottom expand margin by 2 pixels to compensate.

@arkology
Copy link
Contributor

arkology commented Feb 5, 2020

@Calinou I think overline/underline looks much more better. As for me, i prefer overline, because it doesn't make this top panel look "heavy". But underline is also great.
Honestly, when i look at first version of highlight with full color, my eyes bleed :)

@GameDevLlama
Copy link
Contributor

I like the suggestions of @Calinou since in the light theme of the original suggestion, it looks more like it's being disabled instead of highlighted.

@Calinou Calinou added this to the 4.0 milestone Mar 12, 2020
@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from 4e37a4f to 24d49a9 Compare May 22, 2020 20:39
@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from 24d49a9 to 45e2d27 Compare March 4, 2021 03:50
@Calinou Calinou requested a review from a team as a code owner March 4, 2021 03:50
@Calinou
Copy link
Member Author

Calinou commented Mar 4, 2021

Rebased on the current master branch.

I decreased the highlight background opacity to 0.25 to make the text easier to read:

image

@KoBeWi
Copy link
Member

KoBeWi commented Mar 4, 2021

You missed _update_context_menu_stylebox() in CanvasItem editor (it's not called on accent color change).

@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from 45e2d27 to efcecb9 Compare March 4, 2021 23:17
@KoBeWi
Copy link
Member

KoBeWi commented Mar 15, 2021

So I just discovered something about styleboxes. If you set content_margin to zero, the border will be ignored for spacing. You could check if it would allow you to add the underline.

@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from efcecb9 to d10f191 Compare July 16, 2021 20:06
@Calinou
Copy link
Member Author

Calinou commented Jul 16, 2021

Rebased and tested again, it works successfully.

PS: Some contextual editors such as GPUParticles2D and CPUParticles2D add their own VSeparator at the beginning, but they probably shouldn't since this separator is already added by the CanvasItemEditorPlugin or Node3DEditorPlugin. This can be addressed in a future PR.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 16, 2021

So what happened to the underline idea? I just tried the solution I suggested previously and it does work:
image
(underline, no spacing problem)

Here's the stylebox (needs tweaks ofc):

Ref<StyleBoxFlat> context_menu_stylebox = memnew(StyleBoxFlat);
context_menu_stylebox->set_draw_center(false);
context_menu_stylebox->set_border_color(EditorNode::get_singleton()->get_gui_base()->get_theme_color("accent_color", "Editor"));
context_menu_stylebox->set_border_width(SIDE_BOTTOM, 4);
context_menu_stylebox->set_default_margin(SIDE_BOTTOM, 0);
context_menu_container->add_theme_style_override("panel", context_menu_stylebox);

@Calinou
Copy link
Member Author

Calinou commented Jul 16, 2021

@KoBeWi This works; maybe we can try a mix of both (with the background opacity lowered to 12.5%). What do you think?

image

@KoBeWi
Copy link
Member

KoBeWi commented Jul 16, 2021

Yeah, a very subtle background is fine. You could even try lowering it to 10%.

@Calinou
Copy link
Member Author

Calinou commented Jul 16, 2021

Yeah, a very subtle background is fine. You could even try lowering it to 10%.

Done, I pushed it.

@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from d10f191 to 64f5dbd Compare July 16, 2021 22:11
@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from 64f5dbd to 8219b6f Compare July 16, 2021 23:59
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Having the blue line be right next to the other blue line around the viewport does look a bit strange, but I can't think of any definitive ways to improve it. If we put the line at the top it might look confusing to have the same visual design as the dock tabs when this isn't a tab. Having just highlighting and no line is another option, but the line is more modern, so I'm not sure.

Anyway, I'll give this a ✔️, it can always be tweaked later.

EDIT: I suggest merging #48567 first.

@Calinou
Copy link
Member Author

Calinou commented Jul 17, 2021

Having the blue line be right next to the other blue line around the viewport does look a bit strange, but I can't think of any definitive ways to improve it.

#48567 will help with this a bit.

@akien-mga
Copy link
Member

Needs a rebase.

This makes it easier to notice that some menu items only appear when
specific nodes are selected.

This change applies to both 2D and 3D editors, including both plugin-based
menus and the hardcoded 2D layout/animation contextual menus.
@Calinou Calinou force-pushed the editor-viewport-highlight-context-menus branch from 8219b6f to 81d2d2b Compare July 27, 2021 14:45
@akien-mga akien-mga merged commit e95e33f into godotengine:master Jul 30, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3D Particles, move emission mesh creation menu to Inspector
7 participants