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

Take h_separation into account for label and icon positions (item_rect) #74301

Closed
wants to merge 0 commits into from

Conversation

joao-pedro-braz
Copy link
Contributor

@joao-pedro-braz joao-pedro-braz commented Mar 3, 2023

Description

This PR makes it so that when drawing an item_rect the h_separation is taken into account when computing the icon and label positions. The immediate effect is that TreeItem icons in the Editor are no longer drawn "hogging" the item's border, which was specially perceptible when using a Corner Radius greater than 3.

Comparisons

image
Default with Corner Radius 3

image
PR with Corner Radius 3

image
Default (RTL) with Corner Radius 3

image
PR (RTL) with Corner Radius 3

image
Default with Corner Radius 6

image
PR with Corner Radius 6

@joao-pedro-braz joao-pedro-braz requested a review from a team as a code owner March 3, 2023 13:58
@joao-pedro-braz
Copy link
Contributor Author

Also, is there any particular reason on why antialiasing is not enabled by default in the Editor Theme?

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 3, 2023

h_separation is a constant for the distance between the icon and the label. It is not supposed to be applied to icon's initial position. So this is not taking it into account, this is adding it as an extra to fix the issue. I'm not sure this is a correct solution.

Instead, it's probably the stylebox that needs to be adjusted to expand further to the side.

Also, is there any particular reason on why antialiasing is not enabled by default in the Editor Theme?

Performance, it's not necessary but it's expensive, and Godot aims to be friendly to lower end hardware.

@YuriSizov YuriSizov added this to the 4.x milestone Mar 3, 2023
@joao-pedro-braz
Copy link
Contributor Author

joao-pedro-braz commented Mar 3, 2023

I guess the root problem then is that the TreeItem themselves don't have a StyleBox, right?
They only get one if they're focused and/or selected.

If we were to add a default StyleBox to them, then I guess we could solve this problem through the Content Margins.
What do you think?

@YuriSizov
Copy link
Contributor

Yeah, that sounds like a good idea. Just need to make sure that in the default theme that stylebox doesn't do anything (so existing themes work as expected).

@joao-pedro-braz
Copy link
Contributor Author

Yeah, that's a good idea.
Working on it right now!

@joao-pedro-braz
Copy link
Contributor Author

Closing due to #74321, which implements this feature as discussed.

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.

3 participants