Skip to content

Conversation

@charjr
Copy link

@charjr charjr commented Oct 28, 2025

Close #11414

Tried this locally, my example still works.

@AThousandShips Interesting to note, I also tested the extension with no destruction whatsoever with Godot running verbose stdout. I've left it running for over 10 minutes, No warnings of any kind, Godot's built-in monitors show static memory has not changed at all. 🤷

@dsnopek
Copy link
Contributor

dsnopek commented Oct 28, 2025

The added variant_destroy() call won't actually do anything on the Godot side, because Variant::needs_deinit is false for VECTOR2

However, it also won't cause any issues, and so maybe it's better to demonstrate always calling variant_destroy() rather than using internal knowledge to skip it?

Looking at godot-cpp's implementation of Variant, it's always calling variant_destroy() as well, and not trying to be clever about only calling it for certain types

@charjr
Copy link
Author

charjr commented Oct 29, 2025

@dsnopek

The added variant_destroy() call won't actually do anything on the Godot side, because Variant::needs_deinit is false for VECTOR2

@AThousandShips showed me this as well and I just want to say I truly appreciate both of you choosing to point me at the right bit of Godot's internal code. It's not the first time the Godot community have done this for me and each time I come out more confident in my understanding. So thank you. 🙏

However, it also won't cause any issues, and so maybe it's better to demonstrate always calling variant_destroy() rather than using internal knowledge to skip it?

I think in this instance we should demonstrate it to match the text below that code snippet:

At the end we need to destruct the Variants we created. While technically the
Vector2 one does not require destructing, it is clearer to cleanup everything.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Sounds good to me!

I'm not sure why this PR is failing CI - perhaps try rebasing from the latest master branch? There's nothing in the changes here that I would expect to cause issues.

@charjr
Copy link
Author

charjr commented Oct 29, 2025

@dsnopek Rebased and seems fine now. 👍

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

GDExtension C Example - Vector2 misleadingly not cleaned up

3 participants