Skip to content

Conversation

@dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Feb 11, 2023

Fixes #995 and godotengine/godot#71332

I tried a bunch of different variations on this, including using p_object->is_class(class_name) to check if the object was the right type, AND making changes in the Godot engine itself in gdextension_object_cast_to() (which would break the GDExtension ABI, but Remi actually said that would be fine while GDExtension is experimental :-)).

However, assuming there isn't some case where this won't work that I didn't think of, I like the version in this PR the best so far!

It's switching to dynamic_cast<T> at the end, which means C++ will check if the final class matches the requested type using RTTI. This should be much faster than p_object->is_class(class_name) (since that relies on walking up the inheritance chain and comparing strings).

Anyway, it's working in my testing with SG Physics 2D!

@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 12, 2023

Hm, maybe this needs something a little more. I'm now seeing issues with non-custom classes! I don't yet understand "why" though, since they should also have RTTI information that dynamic_cast<T> can use?

UPDATE: Nevermind, I think this is separate issue because it's only happening when getting an Object * in and out of Variant - probably another bug! :-)

@mihe
Copy link
Contributor

mihe commented Feb 12, 2023

Seeing as how RTTI is right up there with exception handling in terms of hard lines that divide the C++ ecosystem, I'm curious what the stance of godot-cpp is when it comes to allowing the disabling of RTTI. The code in this PR would, as far as I can tell, be the only thing that would require it to be enabled.

There is one usage of typeid in rid_owner.hpp (which I would consider optional) that seems to allow RTTI to be opt-out if you define NO_SAFE_CAST, which I weirdly can't seem to find any mention of anywhere else.

#ifdef NO_SAFE_CAST
printf("ERROR: %d RID allocations of type 'unknown' were leaked at exit.", alloc_count);
#else
printf("ERROR: %d RID allocations of type '%s' were leaked at exit.", alloc_count, typeid(T).name());
#endif

Anyway, don't take this as a review or anything. I just happened to stumble upon your PR and it caught my attention. It might very well be that your proposed solution is the only reasonable way of solving this problem.

@akien-mga
Copy link
Member

NO_SAFE_CAST was used in Godot 3.x, and it used to be possible to disable RTTI.
For 4.0, RTTI is required on all platforms and NO_SAFE_CAST was removed. Its usage in rid_owner.hpp here seems to show that this file is out of sync with upstream.

Likewise, object.h in upstream Godot does use dynamic_cast nowadays (but this used to be different).

@akien-mga akien-mga added the bug This has been identified as a bug label Feb 12, 2023
@akien-mga akien-mga merged commit 8a3faaa into godotengine:master Feb 14, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga changed the title Fix Object::cast_to<T>() for custom classes Fix Object::cast_to<T>() for custom classes (reverted) Feb 16, 2023
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.

Object::cast_to works incorrectly for GDExtension custom classes

6 participants