Skip to content

Free nodes from instantiating if it is not a Control in the theme editor #107632

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

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

gongpha
Copy link
Contributor

@gongpha gongpha commented Jun 17, 2025

Minor fix on the theme editor preview.

Fix a rarely-happening minor memory leak from not freeing the instantiated node inside the if clause that checks if a node is Control. Also added more error messages

@AThousandShips AThousandShips requested a review from a team June 17, 2025 13:33
@AThousandShips AThousandShips added this to the 4.5 milestone Jun 17, 2025
@@ -510,8 +510,15 @@ bool SceneThemeEditorPreview::set_preview_scene(const String &p_path) {
}

Node *instance = loaded_scene->instantiate();
if (!instance || !Object::cast_to<Control>(instance)) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Useless newline

Copy link
Member

@akien-mga akien-mga Jun 17, 2025

Choose a reason for hiding this comment

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

I think it's fine either way here, but yeah it could be removed as the pattern:

Object *obj = get_some_object();
if (!obj) {

is pretty common in Godot.

But here there's also a later if check that uses instance and so it can make sense indeed to have them separate:

Node *instance;

if (!instance) {}

if (other thing with instance) {}

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Repiteo Repiteo merged commit 3f99c09 into godotengine:master Jun 18, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jun 18, 2025

Thanks!

@gongpha gongpha deleted the potentially-memory-leak branch June 19, 2025 05:17
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.

5 participants