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

Expose Window's _get_contents_minimum_size() to scripting #80178

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

YeldhamDev
Copy link
Member

The _get_contents_minimum_size() method is used by Godot's Accept/ConfirmationDialog, and it would be very useful for users that intend to make dialogs with custom behavior.

@YeldhamDev YeldhamDev added this to the 4.2 milestone Aug 2, 2023
@YeldhamDev YeldhamDev requested review from a team as code owners August 2, 2023 17:04
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.

Well, makes sense since half of the built-in dialogs and popups implements it.

Comment on lines +1751 to +1752
Vector2 ms = _get_contents_minimum_size();
GDVIRTUAL_CALL(_get_contents_minimum_size, ms);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it going to be wasteful to always run the default _get_contents_minimum_size() before the virtual call? How heavy is that call?

Not tested, but this might be better for performance:

Suggested change
Vector2 ms = _get_contents_minimum_size();
GDVIRTUAL_CALL(_get_contents_minimum_size, ms);
Vector2 ms;
if (GDVIRTUAL_CALL(_get_contents_minimum_size, ms) {
return ms;
}
return _get_contents_minimum_size();

@akien-mga akien-mga merged commit f9d960c into godotengine:master Aug 3, 2023
@akien-mga
Copy link
Member

Thanks!

Didn't intend to merge it just yet, made a mistake in my batch... oh well.

@YuriSizov
Copy link
Contributor

A follow-up would be good still, the change you suggested does make sense.

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