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

Improve material and mesh preview buttons #78858

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Jun 29, 2023

A few small improvements to the mesh/material preview, notably adding a new theme variation for buttons of this sort.

Before Now
image image
  • Tweaked the icons a bit, improving the numbers, the cube's colors (as one of them translates poorly in light theme), and an artifact in the cube icon.
  • Deleted the on/off variations of these icons, as a single one is now enough.
  • Added hover indicators to the toggled off state of these buttons (there's no theme color for when a button is hovered while toggled on.
  • The new theme looks more discernible I think, especially on light theme.
  • Applied this theme also to StyleBox preview, somewhat reverting Improve StyleBox preview a little #77418 where I introduced a small issue, where the grid button would get highlighted when it's focused even though there's no visual indicator of being focused. The new theme variation doesn't have a focus color, but it has a hover color, so it's perfect.

@fire

This comment was marked as outdated.

@fire
Copy link
Member

fire commented Jun 30, 2023

I don't see any obvious problems with the others.

@fire
Copy link
Member

fire commented Jun 30, 2023

Sorry, I forgot this was a sphere. It's hard. Let me see how it should look.

PBR materials can be coloured with this chart.

image

But I agree with the later comment.

https://docs.quixel.com/mixer/1/ja/topic/pbr-physically-based-rendering

@AThousandShips
Copy link
Member

AThousandShips commented Jun 30, 2023

The correctness of the icon to the actual physical object aside it's much less clear with the new design imo, there's no obvious way that the original design doesn't represent the highlight on a sphere

@MewPurPur
Copy link
Contributor Author

I think it's because the rest of the sphere is too bright, so the highlight isn't as obvious.

Now it looks like this:

image

The hole always irked me, but it's not that important and maybe it's the lesser evil if you don't like this one either.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 30, 2023

I don't see any problems with the original? Can you please clarify what the issue is to justify the change, is there some confusion or issue raised over it?

Icons should be as simple as possible

If it should be changed I'd suggest turning it into a wireframe sphere like Blender does, otherwise leave it as it is

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jun 30, 2023

I don't see any problems with the original

Well like I said, I found the hole weird and it's irked me, it's like if something is misaligned. Too small to open an issue and whine about, that's why I didn't only do the PR to change this. Likewise for the old 2 being drawn like this:

image

So my reason is "I found it annoying" but of course, this is subjective, so I'll just strip it out if people do like the old one better. Also the old icon is equally complex before, so I don't get that remark. The cube uses shading, so why would the sphere have a hole?

@AThousandShips
Copy link
Member

AThousandShips commented Jun 30, 2023

The new icon is much less clear to me, especially in low contrast, and it's not wrong or unclear in any way, it conveys the idea of a highlight on a sphere, bright on half-bright is much less visible the screenshot you provided shows that to me at least, so I have an actual accessibility concern about the design

The new design is more "busy", that's what I mean with simple

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Jun 30, 2023

In terms of simplicity, a transparent background is at least as busy as a solid color :P

This doesn't matter though, getting no consensus on that icon means it's best to not change it. So I've reverted it.

@Calinou
Copy link
Member

Calinou commented Jun 30, 2023

I suggest adding an outline to icons that are drawn on a mixed-color background. See editor/icons/ViewportSpeed.svg for an example of this.

@MewPurPur
Copy link
Contributor Author

@Calinou Good idea. Here's take 4:

image

@MewPurPur MewPurPur force-pushed the tweak-mesh-preview branch 2 times, most recently from 0755349 to 05652a0 Compare June 30, 2023 19:43
@AThousandShips
Copy link
Member

Those buttons are very clear

@MewPurPur
Copy link
Contributor Author

image

Minor tweak, reducing the opacity of the outline to make it less jarring.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

I tested this PR locally. Some comments:

  • The outline is a bit too subtle (or too thin) now, considering the background. It doesn't help that much with visibility, given the bright checkerboard pattern in the background.
    • An alternative is to darken the checkerboard background (and reduce the contrast between light and dark squares), which dark theme enjoyers will surely appreciate too 🙂
  • Icons look broken when using light theme, as the background color doesn't change. I suggest adding the icons to the theme conversion exclusion in editor_themes.cpp.

Dark theme

Screenshot_20230803_040708

Light theme

Screenshot_20230803_040720

@MewPurPur
Copy link
Contributor Author

Adding the icons there sounds like a great idea!

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Aug 3, 2023

It looks good to me, but still I brought opacity up from 60% to 80%. And added them as exceptions.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 3, 2023
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I like this improvement a lot!

@MewPurPur MewPurPur force-pushed the tweak-mesh-preview branch 2 times, most recently from 3a85a14 to 2b962fc Compare August 3, 2023 21:40
@MewPurPur
Copy link
Contributor Author

OK doned. Did a few other code style + micro optimizations in the push too, but nothing that affects appearance or behavior.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Behavior on light theme is good now, including after restarting the editor:

Screenshot_20230805_022124

Screenshot_20230805_022136

@akien-mga akien-mga merged commit 02709d5 into godotengine:master Aug 7, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

@MewPurPur MewPurPur deleted the tweak-mesh-preview branch August 7, 2023 16:21
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.

6 participants