Fix cast_to for non-exposed engine types

[sheepandshepherd]

This PR changes how Object::cast_to<T>(o) checks o's type when T is an engine class.

TagDB would fail on instances of non-exposed types like BulletPhysicsDirectSpaceState. Instead, engine types are now type-checked by the engine's own cast_to implementation, which doesn't require GDNative to have type tag/parent info about internal types. NativeScripts continue to be checked with TagDB.

Related to #175, godotengine/godot#28195, and godotengine/godot#28574, but they were actually fixed already. This only fixes casts affected by the same issue.

(Depends on godotengine/godot#34688 and updated godot_headers.)

[BastiaanOlij]

@sheepandshepherd could you check why travis wasn't happy with your fix?
Happy to merge this once travis is happy :)

[sheepandshepherd]

Looks like it just needed the updated 3.2 headers. Thanks for merging those. Travis is happy after a rebase.

[lyuma]

Hey, I'd like to voice my support for the PR. It actually fixes an issue I have been running into on macOS, and quashes my fears regarding sharing RTTI data across binaries.

Let me summarize the issue I ran into that this PR fixes:
I'm testing using the 3.2 branch of godot-cpp, together with the tip of godot 3.2 branch (3.2.2rc). After passing a node path from GDScript into my GDNative module, I have this GDNative code:

Node *targetNode = get_node_or_null(nodePath);
Spatial *targetSpatial = Object::cast_to<Spatial>(targetNode);

The bug I was running into specifically on Mac and with this combination of engine is that Object::cast_to returns null, even though I made absolutely sure that targetNode was indeed a Spatial.

My conclusion therefore was that RTTI's typeid().hash_code() was crossing ABI boundaries, and due to a nuance with how I compiled it, that caused the cast_to comparison to fail.

I tested both with and without this patch in two separate builds just in case RTTI was random..I suppose that's still possible given that std::type_info::hash_code is random and offers no uniqueness guarantees(!) but HIGHLY unlikely.

Anyway, in both sets of builds, I can confirm that this PR fixes my issue on my particular macOS environment in my particular GDNative module in my particular application. I know that's not much to go on, but I think it's a great change regardless!

[Zylann]

I still need to understand the changes in the PR (notably the new class header items), but, but there are conflicts to be resolved before it can be merged.

Is that fixing the potential crash that would occur when doing stuff like Ref<SurfaceTool>(ArrayMesh::_new())?

[sheepandshepherd]

No, those are a different issue. Ref just needs to use cast_to instead of always returning the pointer. (It's there in commented-out TODO blocks, I guess Ref was just implemented before cast_to was ready.) This only replaces the system currently used for cast_to type checking with the one built into Godot.

Currently, it always fails to cast objects of non-exposed types like BulletPhysicsDirectSpaceState because it expects a tag for the dynamic type of the object to be present in the TagDB. There was previously a PR (godotengine/godot#27936) that "fixed" TagDB by putting all internal types into the API JSON, which reduz and karroffel wanted to avoid.

Godot already has a dynamic_cast replacement, a virtual Object function is_class_ptr, that handles the core engine's cast_to when dynamic_cast is disabled. It takes a pointer "tag" for the class you want to cast to, which is stored in ClassDB for exposed classes and accessible to GDNative through godot_get_class_tag. The new static member variables in the headers (___class_tag) store this class pointer for each built-in class.

The function itself is exposed to GDNative already as well, as godot_object_cast_to. Unlike the TagDB it requires no information about unexposed classes to work. It only calls the virtual is_class_ptr on the Godot side, passing the class tag. This PR changes godot-cpp's cast_to to use it instead of TagDB only when casting to built-in types.

I'll fix the merge conflicts in a bit, and also rename the ___class_tag variables (reserved identifiers due to the multiple underscores, according to the standard...).

[Zylann]

Yeah I think I'll work on removing all ___ later, and expand the use of detail:: when applicable. Which convention do you want to use for class items? It sill has to be quite distinct from user ones. Maybe detail__xxx?

[sheepandshepherd]

Apparently double underscores are reserved anywhere in names in the C++ standard, but Microsoft uses slightly different wording reserving them only at the beginning.

Maybe we can use _detail_... to avoid the few identifiers in the Godot API that begin with detail_...? Either way looks good to me though. Let me know if you prefer some other naming scheme here. (It looks like single underscores at the end are allowed as well, for example.)

[Zylann]

_detail_ sounds fine.

[Zylann]

I see you made it so Godot classes now have a specific ___get_id(), however there might be a problem with custom classes:

godot-cpp/include/core/Godot.hpp

Line 103 in 1600019

inline static size_t ___get_base_id() { return typeid(Base).hash_code(); } \

The wrong ID will be returned here

[sheepandshepherd]

Thanks, I missed that one. ___get_base_id is used to access _detail_class_tag because both branches (cast to built-in class using class tag and cast to custom class using TagDB) need to compile for all types unless we introduce C++17's if constexpr here.

[Zylann]

Perhaps we can encapsulate this typeid(T).hash_code() by using __get_id() instead?

[Zylann]

Thanks!
Note: this works starting from Godot 3.2.x