Register server classes to fix GDNative binding issues

[aqnuep]

Due to ClassDB not containing information about the server specific classes
the JSON API file didn't contain those either which caused the GDNative
binding tools to not recognize objects of such classes.

This is needed to fix godotengine/godot-cpp#175.

[karroffel]

I don't know what reasons were there to not expose those in the first place, so this might not be a good change, but if I assume there was no reason for that then this PR looks good!

[akien-mga]

clang-format doesn't like the empty line after public: https://travis-ci.org/godotengine/godot/jobs/523390022

[neikeq]

Don't merge this. I'll review in an hour.

[neikeq]

We do not want to expose these classes to the scripting API. The same way we don't expose OS_Unix (I meant IP_Unix).
IINW, the issue with godotengine/godot-cpp#175 is the same I had some time ago with C#, which I fixed doing the following:

godot/modules/mono/csharp_script.cpp

Lines 1126 to 1130 in b0da7b6

	const ClassDB::ClassInfo *classinfo = ClassDB::classes.getptr(type_name);
	while (classinfo && !classinfo->exposed)
	classinfo = classinfo->inherits_ptr;
	ERR_FAIL_NULL_V(classinfo, false);
	type_name = classinfo->name;

Not sure how that could be applied to NativeScript.

Maybe we could have a way to register private classes in ClassDB.
Or even better, perhaps this is already fixable from bindings generators. Just make an additional check, if the class is not exposed but it derives an exposed class, generate it (in C# such a class would be generated as internal).

[aqnuep]

Not sure what's the ground for not wanting to expose them, after all they are nothing but implementations of an already exposed interface.

In the end it's your call, I just intended to contribute back the fix I did on my fork, considering that apparently other people ran into the same issue.

Feel free to close the PR if you don't want to take this fix.

[neikeq]

First of all, I'm not talking for all contributors, as it can be seen in the two other contributors who approved this PR.

One reason I don't like this is because all the reason to expose these classes is to fix the crash. There is no other point for these classes to be exposed, and the user is not meant to use them directly. This wouldn't be too much of a problem is we could somehow mark these classes so the C# bindings generator, for example, could generate them as internal.

The other reason, and the most important, is the case of IP_Unix (I said OS_Unix by mistake in the other comment). If I understand the godotengine/godot-cpp#175 issue correctly, the same crash should be reproducible when trying to access any IP instance on a unix platform (something like godot::Engine::get_singleton_object("IP") or maybe just godot::IP from godot-cpp).
If that's the case, we cannot just apply the same fix and expose IP_Unix, because that would make the ClassDB API non-portable as IP_Unix only exists on unix platforms.

[AndreaCatania]

It could be interesting if we would allow godot to support different features depending on the implementation chosen. Taking the physics as example we could have additional APIs depending on the physics engine, (For example the kinematic APIs that get rejected #21838), opening a lot of new possibilities.

On the other hand, the down side is that it could mean start to break the base Godot architecture, since in this moment everything is unified.

To say a rough opinion, in this moment, I'm at 51% on reject this PR, because I can add the extra features that I need elsewhere...

[aqnuep]

One reason I don't like this is because all the reason to expose these classes is to fix the crash.

That's an oversimplification. In practice what happens is that simply the binding is not aware of the object's type (as the inheritance relationship is unknown to it as it's not present in the API json), which then causes the crash.

The other reason, and the most important, is the case of IP_Unix (I said OS_Unix by mistake in the other comment). If I understand the GodotNativeTools/godot-cpp#175 issue correctly, the same crash should be reproducible when trying to access any IP instance on a unix platform (something like godot::Engine::get_singleton_object("IP") or maybe just godot::IP from godot-cpp).

I didn't look into that one, but if it's the same principle where there is an OS-specific subclass implementing a common interface, then I'm afraid the issue is the same.

Really, there are two options as I see it:

• Either register the implementation classes of the interfaces with the ClassDB too, so that the API json has information about them and thus bindings can understand what they are (possibly even if an actual implementation won't exist on any given platform),
• Or change the way how GDNative works on the engine side so that it treats the implementation classes as if they were identical to the parent interface class

IMO the API json itself should contain all interfaces though, even if some of the implementation classes only ever going to exist on a subset of the platforms/configurations.

Again, it's up to you how you go about, as far as I'm concerned I have a working solution for the problem I encountered, so I won't have hard feelings about you closing the PR :)

[neikeq]

IMO the API json itself should contain all interfaces though, even if some of the implementation classes only ever going to exist on a subset of the platforms/configurations.

This will not happen. The ClassDB API has to be the same on all platforms and for all targets (debug/release).

Maybe we could use the pimpl idiom to solve the IP_Unix issue, or something like what is done with the OS class (which is basically the same as pimpl).

[neikeq]

The thing is this PR is missing more types that are not exposed. This is the full list:

BulletPhysicsDirectBodyState, BulletPhysicsServer, GDNativeLibraryResourceLoader, GDNativeLibraryResourceSaver, GDScriptNativeClass, IP_Unix, InputDefault, Physics2DDirectBodyStateSW, Physics2DServerSW, ResourceFormatDDS, ResourceFormatImporter, ResourceFormatLoaderBMFont, ResourceFormatLoaderBinary, ResourceFormatLoaderCSharpScript, ResourceFormatLoaderDynamicFont, ResourceFormatLoaderGDScript, ResourceFormatLoaderImage, ResourceFormatLoaderNativeScript, ResourceFormatLoaderShader, ResourceFormatLoaderStreamTexture, ResourceFormatLoaderText, ResourceFormatLoaderTextureLayered, ResourceFormatLoaderTheora, ResourceFormatLoaderVideoStreamGDNative, ResourceFormatLoaderWebm, ResourceFormatPKM, ResourceFormatPVR, ResourceFormatSaverBinary, ResourceFormatSaverCSharpScript, ResourceFormatSaverGDScript, ResourceFormatSaverNativeScript, ResourceFormatSaverShader, ResourceFormatSaverText, ResourceImporter, ResourceImporterOGGVorbis, ResourceSaverPNG, TranslationLoaderPO

An instance of any of those types that is returned to script code (be it C#, godot-cpp, etc) is vulnerable to the godotengine/godot-cpp#175 issue. Applying this fix to all of these types completely eliminates the point of the exposed tag, as everything is now exposed.

I think the better fix is to register the classes but still mark them as not exposed. Then each bindings generator can decide how to deal with them.

There's still the problem of types like IP_Unix that are not present on all platforms/targets.

[aqnuep]

IMO the API json itself should contain all interfaces though, even if some of the implementation classes only ever going to exist on a subset of the platforms/configurations.

This will not happen. The ClassDB API has to be the same on all platforms and for all targets (debug/release).

I didn't say otherwise, in fact we're in agreement. I just suggested that if we do have these "platform-optional" classes, those should be registered even if they aren't actually available on any particular platform, unless you use the pimpl idiom as you suggested. FWIW, I guess the ARVR classes are another example where the interface class itself is exposed, but there may not be actual implementations for them on all platforms.

[reduz]

In a sense i don't think it's wrong to expose something like bulletphysicsserver if it's exposing extra features over the regular one (they need to be registered as virtual, though).

The problem is that servers are wrapped into the multithread wrapper, so you don't really have access to them anyway. Maybe a different approach should be taken if we understand the actual use case.

[akien-mga]

I'm still not sure what to do with this PR. I share @neikeq's concern over registering implementation classes which are not meant to user-facing, such as OS or library-specific implementations.

I'm not sure how to fix the GDNative issue otherwise, but I feel this is not the right approach.

BTW, I tried it locally to see if it would add the newly registered classes to the class reference, and doctool segfaults with these changes:

$ godot-git --doctool .
Godot Engine v3.2.alpha.custom_build.c52287208 - https://godotengine.org
git statusFound discrete GPU, setting DRI_PRIME=1 to use it.
Note: Set DRI_PRIME=0 in the environment to disable Godot from using the discrete GPU.
OpenGL ES 3.0 Renderer: AMD VEGAM (DRM 3.32.0, 5.2.16-desktop-2.mga7, LLVM 8.0.0)
 
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib64/libc.so.6(+0x3caf0) [0x7f7373212af0] (??:0)
[2] BulletPhysicsDirectBodyState::get_inverse_mass() const (/home/akien/Projects/godot/godot.git/modules/bullet/rigid_body_bullet.cpp:78)
[3] MethodBind0RC<float>::call(Object*, Variant const**, int, Variant::CallError&) (/home/akien/Projects/godot/godot.git/./core/method_bind.gen.inc:593 (discriminator 4))
[4] ClassDB::get_property(Object*, StringName const&, Variant&) (/home/akien/Projects/godot/godot.git/core/class_db.cpp:1069)
[5] Object::get(StringName const&, bool*) const (/home/akien/Projects/godot/godot.git/core/object.cpp:494)
[6] ClassDB::class_get_default_property_value(StringName const&, StringName const&, bool*) (/home/akien/Projects/godot/godot.git/core/class_db.cpp:1404 (discriminator 1))
[7] godot-git() [0x23cc529] (/home/akien/Projects/godot/godot.git/editor/doc/doc_data.cpp:214 (discriminator 1))
[8] DocData::generate(bool) (/home/akien/Projects/godot/godot.git/editor/doc/doc_data.cpp:283 (discriminator 2))
[9] Main::start() (/home/akien/Projects/godot/godot.git/main/main.cpp:1392)
[10] godot-git(main+0xf1) [0x1432043] (/home/akien/Projects/godot/godot.git/platform/x11/godot_x11.cpp:55)
[11] /lib64/libc.so.6(__libc_start_main+0xeb) [0x7f73731fcb0b] (??:0)
[12] godot-git(_start+0x2a) [0x1431eaa] (/home/iurt/rpmbuild/BUILD/glibc-2.29/csu/../sysdeps/x86_64/start.S:122)
-- END OF BACKTRACE --
Aborted (core dumped)

[neikeq]

It's not only that concern, but doing this would break API portability, which is critical and the whole reason exposed was added.

If a solution similar to what I proposed (which I use in C#) is not possible or not desired, then here is another option:

Add a way to register classes as "internal" or "private". Those would be generated by bindings generators as any other class, but they would be generated as a private API not visible/exposed to user.

• It's important that the API hash calculator excludes such "internal" classes when calculating the API hash.
• Such classes cannot bind methods/signals/properties as they are supposed to implement the virtual methods that are already bound by the exposed class they inherit.

Neither this solution nor this PR's solution solve the following issue: it's hard to know what needs to be bound until you get a crash/error. Unless you were to just bind every single non-exposed class as internal.

Ultimately, I prefer the solution I use in C#. Or a similar but better one if any one comes up with that.
If someone could point me to the relevant part of GDNative or godot-cpp where the object of such classes is not recognized, I could be of some help.

[sheepandshepherd]

The crash described in the 3 related issues no longer happens. Has it been fixed or are we still looking for other solutions? (I notice my PhysicsDirectSpaceState has a null type tag, so casting still doesn't work for non-exposed types.)

Type tags for Godot classes wouldn't be needed if we could access is_class_ptr and get_class_ptr_static (the engine's own cast_to implementation) from GDNative. They'd allow casting to engine types without any info on unregistered classes like BulletPhysicsDirectSpaceState. Casts to NativeScripts would still be handled with type tags.

The implementation is straightforward: changes to Godot, changes to C++ binding.

However, those 2 functions are unused in favor of dynamic_cast on most platforms, so it's worth considering whether we want to use them for GDNative. The functions themselves are not removed when the NO_SAFE_CAST macro is undefined, only their usage in cast_to.

[kimjiy9607]

So what is the status of this bug report? Is it waiting to be merged?

[akien-mga]

Superseded by #34688.