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

Refactor StyleBox's get_minimum_size method to make it virtual #5903

Closed
98teg opened this issue Dec 6, 2022 · 0 comments · Fixed by godotengine/godot#71686
Closed

Refactor StyleBox's get_minimum_size method to make it virtual #5903

98teg opened this issue Dec 6, 2022 · 0 comments · Fixed by godotengine/godot#71686
Assignees
Milestone

Comments

@98teg
Copy link

98teg commented Dec 6, 2022

Describe the project you are working on

A plugin to add more StyleBoxs to Godot.

Describe the problem or limitation you are having in your project

I can't set the minimum size of a control node via a new type of StyleBox. I'm creating a new StyleBox called StyleBoxBar with an attribute called thickness which should influence the control node's minimum size. For example, if a button has content with a height of 10px, but the thickness of its StyleBoxBar is 30px, the minimum height of the button should be 30px.

Right now, there are a couple of virtual methods to determine the minimum size of a control node based on the StyleBox, but none of them achieves this effect.

First, there is _get_style_margin. It won't work because I can only set a predetermined margin without knowing the content's height. In my previous example, if I set the margin to 15px, the minimum height would be 15+10+15=40px.

Then there is _get_center_size. It is only used in some nodes to determine the minimum size, such as ScrollBar or Separator. So it is ignored in the case of a button.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

After discussing this issue with @YuriSizov, we conclude that _get_center_size seems kind of redundant and a bit of a hack.

Internally, some nodes use this property to add it to the style box minimum size. There is no other internal use for it. And as such, it can be removed in favour of making get_minimum_size a virtual method. That would allow StyleBoxTexture; and other possible StyleBoxs, to customize how to determine its minimum size. It would also remove duplicated code since this would eliminate the need to add the center size to the minimum size in those nodes I mentioned earlier.

This change would break the public API, but I can not think of many use cases for get_minimum_size or get_center_size outside the engine itself.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The get_center_method would be removed from StyleBox, and get_minimum_size would be implemented as a virtual method. Every StyleBox would have the same get_minimum_size implementation as it is right now, except StyleBoxTexture, which would return its region_rect.size.

StyleBoxLine could also use this new get_minimum_size since right now it uses the get_style_margin method which don't seem appropriate.

Every node that previously used get_center_size in order to add it to get_minimum_size would now just need to call get_minimum_size.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This is a core change.

Is there a reason why this should be core and not an add-on in the asset library?

This is a core change.

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

Successfully merging a pull request may close this issue.

4 participants