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

Fix exception when using base types of extension-based types from C# #75955

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Apr 12, 2023

Fixes #74801.

This PR allows for GDExtension-based classes to be interacted with from C# so long as it's done through one of the engine-based base classes.

For example, without this fix the following code will cause a NullReferenceException when using a GDExtension-based physics server, because in such cases the state parameter must be backed by an extension class inheriting from PhysicsDirectBodyState3DExtension, which causes the C# integration to look for that extension class within the GodotSharp assembly, which it won't find.

public partial class MyRigidBody : RigidBody3D
{
    public override void _IntegrateForces(PhysicsDirectBodyState3D state)
    {
    }
}

This fix solves this (somewhat crudely) by traversing the class hierarchy when looking for the appropriate ClassInfo, and skipping them so long as it's a GDExtension class or its name happens to be either PhysicsServer2DExtension or PhysicsServer3DExtension. The exceptions for the physics server classes have to be made due to a workaround introduced in #59286 which explicitly filters them out when generating the C# bindings.

Note that this fix doesn't do anything to solve the issue of actually exposing the GDExtension class itself to C#. If anything, this fix would probably get in the way of solving that problem, but I figured this could be a stopgap solution.

EDIT: MRP can be found here.

@mihe mihe force-pushed the csharp-gdextension branch 2 times, most recently from 76b9790 to db99cdc Compare April 12, 2023 08:32
@neikeq neikeq self-assigned this Apr 12, 2023
@neikeq neikeq self-requested a review April 12, 2023 16:30
@mihe mihe changed the title Fix crash when using base types of extension-based types from C# Fix exception when using base types of extension-based types from C# Jun 6, 2023
@mihe mihe closed this Jun 6, 2023
@mihe mihe deleted the csharp-gdextension branch June 6, 2023 09:03
@mihe mihe restored the csharp-gdextension branch June 6, 2023 14:03
@mihe mihe reopened this Jun 6, 2023
@mihe
Copy link
Contributor Author

mihe commented Jun 6, 2023

Apologies, I did not mean to close this. I was culling some old branches and happened to delete this one, which apparently closes the PR.

This is still very much relevant.

EDIT: Also, if there's anything I can do to help speed things up here, just let me know.

@raulsntos raulsntos removed the archived label Jun 6, 2023
@raulsntos
Copy link
Member

I think this is a good temporary solution until the .NET module implements proper support for GDExtension classes.

I haven't tested this very thoroughly, but it allows getting a GDExtension node with GetNode and it's possible to access its members with the Call/Get/Set methods in GodotObject.

@YuriSizov
Copy link
Contributor

@raulsntos Would you mind giving this another look and an approval if you find it a good temp solution?

@mihe
Copy link
Contributor Author

mihe commented Jun 12, 2023

I went ahead and rebased against latest master (593d5ca), just in case. I also updated the comments slightly.

I also took the time to create a one-off build of my GDExtension that works with latest Godot master, which as of writing this is 593d5ca, and have included that in an MRP that you'll find below. I assume it'll be forward-compatible for the foreseeable future, but that's the specific commit I targeted if you need it.

With this MRP you'll immediately run into the errors that prompted this PR to begin with, like not being able to create the instance binding for my JoltPhysicsServer3D, and when applying the changes in this PR you'll see that the errors go away.

I'll be honest and say that I have not tested this extensively, nor with any larger project, but the changes seem straight-forward enough that I don't see why they wouldn't scale.

Anyway, here's the MRP: PR75955_MRP.zip

(I was not able to sign this specific build for macOS, so you'll get the typical "untrusted developer" Gatekeeper warning if you try to run this on macOS, unfortunately.)

Let me know if there's anything else I can do to help move things along.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the MRP, it was very helpful. I also tested it a bit with Godot Rust, this PR will also fix an issue they were having (godot-rust/gdext#166).

The PR looks good to me so I'm approving.

@akien-mga akien-mga merged commit 4632137 into godotengine:master Jun 12, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants