Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure GDExtension class is the correct type for the Godot engine class #1050

Merged
merged 1 commit into from
May 25, 2023

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Feb 17, 2023

As discovered after merging PR #1037 (which was later reverted), when a Godot engine class is created on the Godot side and then passed to GDExtension, the GDExtension class that is created may not be of the correct type (most often it'll be created as godot::Object and then be unsafely cast to the correct class, which could lead to strange/incorrect behavior later).

This is explained in detail in these two comments:

This PR aims to ensure that the GDExtension class that's created to wrap a Godot engine class always matches the Godot class it wraps.

This is a draft currently, because there is still more that I'd like to do:

  • This isn't as efficient as it could be. We're always looking up the instance binding callbacks, even when we don't need them (which would be the case for any class created on the GDExtension side)
  • I'd like to make a utility method somewhere to correctly convert from a GodotObject* to a godot::Object, rather than have it only exist in Variant::operator Object*()
  • As pointed out by @zhehangd in Object::cast_to works incorrectly for GDExtension custom classes #995 (comment), there are more places in godot-cpp that need to be fixed (for which we'll use the utility function)

This depends on PR godotengine/godot#73511 to the Godot engine, because a new function needs to be added to the GDExtension interface.

UPDATE: Back in draft, since I'm rebasing this on godotengine/godot#76406

UPDATE: godotengine/godot#76406 was merged, so I'm taking this out of draft!

Fixes #995

@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 18, 2023

My latest update does the following:

  • Adds a "fast path" for getting instance bindings when an object already has them. This way we aren't trying to look up the instance binding callbacks when we don't actually need them.
  • Creates a godot::internal::get_object_instance_binding(GodotObject *p_engine_object) function that can be used to convert any Godot engine object into the correct godot-cpp object class, where the Godot engine class isn't known at compile time.
  • Switched to the new function everywhere in godot-cpp that it was needed. I left alone the use of internal::gde_interface->object_get_instance_binding() when making the singleton objects, since the class is 100% known at compile time.

So, once the CI has passed, I'm going to take this out of Draft!

(NOTE: just to make this absolutely clear, we don't need this for 4.0 stable - this can wait for 4.x :-))

@dsnopek dsnopek marked this pull request as ready for review February 18, 2023 21:22
@akien-mga akien-mga added the bug This has been identified as a bug label Feb 23, 2023
@dsnopek dsnopek force-pushed the object-correct-class branch 2 times, most recently from d0093c7 to 76ab02b Compare April 4, 2023 14:38
@dsnopek dsnopek marked this pull request as draft April 27, 2023 22:08
@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 27, 2023

Converting to a draft while PR godotengine/godot#76406 is in progress

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 10, 2023

I've updated this to be based on godotengine/godot#76406

So, this'll need to wait until at least that one is merged before this can come out of draft again.

However, I'd also like to add a test (based on PR #1101) to validate the fix, so we can't later regress!

So, this'll also need to wait for that PR to be merged.

UPDATE: I don't want to delay this any more :-)

@dsnopek dsnopek marked this pull request as ready for review May 16, 2023 20:41
@dsnopek
Copy link
Collaborator Author

dsnopek commented May 16, 2023

This is finally really ready for review! I've also put it on the agenda for the next GDExtension meeting :-)

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 19, 2023

Discussed at the GDExtension meeting, and overall the solution makes sense. @vnen is going to do code review

@BrianWiz
Copy link

Hey do you need any help here? I hate to be that guy but that bug was unacceptable. Are there unit tests for this? If not I could look into writing some.

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 23, 2023

Are there unit tests for this?

I'm planning to add tests for this once #1101 is merged! I just didn't want to (potentially) delay this one by basing it on that one. Hopefully, they'll both be merged soon :-)

@dsnopek dsnopek merged commit 8052f81 into godotengine:master May 25, 2023
@Darker
Copy link

Darker commented May 25, 2023

Thanks so much for this, I was waiting for it! Can't wait to update and try it :)

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
5 participants