InitHandle and NativeClass::new_instance() now refer to a global class registry to check if the class has been added.

[jacobsky]

This is related to issue #602 where and it allows the InitHandle struct and NativeClass trait to properly track whether a class has been added to the godot API.

This is just my first pass at a naming scheme, so if there is a preferred naming convention to go by or modifying the error messages, please let me know.

For the "class_registry", I decided to use the RwLock as there shouldn't be any need for locking after initialization. I thought this would be a performance be a minimal number of writes after initialization. This should lower the performance cost for reads after that point. The registry consists of a simple hash set as outlined in the Issue

For the NativeClass::new_instance() method, I chose to use a debug_assert!(), to give a more concrete error message when using a debug-build, but remove the additional check during instancing from Release builds.

If there are any other requests regarding this, I would be happy to make the necessary changes.

[Waridley]

Could this also make it easier to support inheriting from another Rust NativeClass type? The engine will check if you pass the name of a class owned by the same gdnative library: https://github.com/godotengine/godot/blob/3.x/modules/gdnative/nativescript/godot_nativescript.cpp#L66

[jacobsky]

@toasteater I went ahead and finished the changes that I was working on for the review in the latest commit 29fb8de please let me know if there's anything else that you'd like me to look into changing.

[jacobsky]

@Waridley while that is not exactly the intent for this commit, it might be possible to extend that functionality. The main purpose of this commit is to help making it a bit more obvious whether a type is registered multiple-times as well as making it more obvious when a NativeScript is not registered at all.

[ghost]

Looks good to me. Thanks!

bors r+

support inheriting from another Rust NativeClass type

I'm not entirely sure how this would work on the Rust side. We'll need to look into what setting the base field actually means before we can even start to consider whether it would be useful. In any case, this is out of the scope of this PR.

[bors]

Build failed:

• ci

[jacobsky]

@toasteater looks like I still have some additional work to go. It looks like the changes crashed in the godot tests. Sorry about that. I ran the main test suite, but I must have missed running the godot tests. That's embarassing...

I'll work on resolving those test failures.

[jacobsky]

I dug into the test cases a bit deeper and it looks like this is due to the test cases not actually calling an init callback. The easiest solution is likely to add a small section that allows it to bypass the check only in the test project. I tried by checking based on the feature "gd_test", but while this allows the checks in the test project to pass, the feature also appears to be active in the non-test projects.

[Waridley]

@jacobsky Right, I'm not saying you should implement it now. The only reason I even mentioned it here was I want to make sure whatever we do doesn't make it harder to implement this, and just in case it seemed trivial to add I wanted you to be aware of it.

[ghost]

I dug into the test cases a bit deeper and it looks like this is due to the test cases not actually calling an init callback.

That's weird. The functions sure do seem called in the code, and if we really didn't register the types, the tests should have fallen apart a long time ago. I think there's more to this problem that's worth investigation. We certainly don't want to hack around it by disabling the check in tests.

[ghost]

That's said it's also not immediately obvious to me how the code can go wrong. It's short, and it contains no new unsafes. This is really weird.

[jacobsky]

@toasteater thank you for the insight! I'll keep digging a bit more into the test cases and see what I can find.

[jacobsky]

I checked the various register functions as well as the lib.rs init function with print statements and it looks like it is never called. I was able to confirm this by also adding some panic!() macros into the init function and the panics never occurred. What is especially odd is that this should cause other failures.

I can also deduce that the macro is at least running since commenting it out causes the test to fail with the ERROR: initialize: Failed to obtain godot_gdnative_init symbol so I'm a bit stumped at the moment as to why the init() isn't being properly called in the tests.

After checking out the main difference, while it shouldn't cause any problems, it may be related to the gdscripts gdn.instance() call. I have a suspicion that it's not doing what we think it's supposed to do in that case. I tried modifying the GDNative library settings and none of them changed the behavior, the problem definitely looks like it's not doing exactly what we think it's doing.

The weirdest part is that not actually adding the classes should cause more issues in addition to what we're seeing. It's odd that it's only being revealed with this change.

[jacobsky]

For completeness beefd87 includes a change that should allow the test to pass now. The cause was due to the lazy initialization of the godot_nativescript_init this was resolved by moving the check in class.rs::maybe_emplace() to later in the function to allow for godot to properly lazily load the library while also being available in rust.

[ghost]

Let's hope that this is working now!

bors r+

[bors]

Build failed:

• ci

[ghost]

Well there are some new lints in clippy. I'll fix that one.

[ghost]

bors retry

[bors]

Build succeeded:

• ci

[jacobsky]

@toasteater thank you for all the help! It was fun to make this addition.