Skip to content

Rework the way custom class instances are created from C++#408

Merged
Zylann merged 1 commit intogodotengine:masterfrom
Zylann:custom_ref_rework
Jun 5, 2020
Merged

Rework the way custom class instances are created from C++#408
Zylann merged 1 commit intogodotengine:masterfrom
Zylann:custom_ref_rework

Conversation

@Zylann
Copy link
Collaborator

@Zylann Zylann commented May 30, 2020

This is a new take that can replace #401 and #346 (it goes a long way at this point).

Should fix #397, #343, #215.
PR #350 is no longer needed, as well as godotengine/godot#33532 and godotengine/godot-headers#60, so it should also be easier to merge.
PR #346 might also be no longer needed, if it's about the same issue? Though I don't really understand the change it does.

To understand what's going on and why this is yet another PR, I wrote a summary here: godotengine/godot#33532 (comment)

This PR avoids the original source of the leak by directly creating the new instance, without going through reference initialization (which is then done by us down the road), so it behaves consistently like in engine and workarounds of the previous PRs should not be needed.

  • Custom references no longer leak
  • The script instance is created using Object::set_script() instead of NativeScript::new(), as is done in the D and Rust bindings (thanks @toasteater and @sheepandshepherd for the help).
  • The use of GDNativeLibrary no longer causes a leak (worked around with C API, see Instance binding data on GDNativeLibrary objects never gets freed godot#39181)
  • Extracted code from GODOT_CLASS so it's not written twice
  • Formatted a bit some long lines to fit in the width of Github review
  • Wrapped related code in a detail namespace, as it's meant for internal use
  • Added comments about what's going on (I can remove those about failed attempts if you think they aren't needed)
  • Renamed as() to its true meaning (not 100% sure on that. I did this when looking at the only places it was used, please confirm)
  • Fixed a few things which were forcing to wrap custom classes inside the godot namespace

Note: the only downside of set_script is that we can't have this fix and have constructor arguments at the same time (as proposed in #293). However constructor arguments were already not supported so this doesn't break any compatibility and should be investigated in a future PR (which could involve PR to Godot), as I think the fix has much higher priority.

@Zylann Zylann changed the title Rework the way custom class instances are created from C++ [WIP] Rework the way custom class instances are created from C++ May 30, 2020
@Zylann Zylann force-pushed the custom_ref_rework branch 2 times, most recently from e89ee90 to a493928 Compare May 31, 2020 00:42
@Zylann Zylann changed the title [WIP] Rework the way custom class instances are created from C++ Rework the way custom class instances are created from C++ May 31, 2020
@Zylann
Copy link
Collaborator Author

Zylann commented May 31, 2020

Worked around the last leak, things should work fine now.
Formatting check fails, not sure why since I use clang-format

@Zylann Zylann mentioned this pull request May 31, 2020
@BastiaanOlij BastiaanOlij added the bug This has been identified as a bug label Jun 2, 2020
Copy link
Collaborator

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

I'm happy to try these changes merged into master. Do fix up the clang-format issues first though

@akien-mga
Copy link
Member

Try to rebase after #409.

@Zylann Zylann force-pushed the custom_ref_rework branch from a493928 to 616aad8 Compare June 2, 2020 17:25
@Zylann Zylann force-pushed the custom_ref_rework branch from 616aad8 to 09c8bf9 Compare June 2, 2020 18:34
@Zylann
Copy link
Collaborator Author

Zylann commented Jun 2, 2020

Fixed formatting.

@dancullen
Copy link

I know I'm a bit late to the party, but I wanted to affirm that this indeed fixes #215 for me. Many thanks @Zylann !

@kb173
Copy link

kb173 commented Sep 25, 2020

I can also confirm that this fixes #215. Thank you @Zylann, this feels much more stable now!

Just for reference since that wasn't immediately clear to me: The proper way to instance with no workarounds is now:

Ref<CustomClass> object;
object.instance();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This has been identified as a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(Windows) _new() creates invalid Null instance of custom class / crashes in debug build

5 participants