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

[.NET] Replace Reflection-Based implementation with Generated one in CreateManagedForGodotObjectBinding #96955

Conversation

Delsin-Yu
Copy link
Contributor

Before this PR, ScriptManagerBridge used reflection-based implementation to instantiate engine wrapper types; this PR introduces a generated Constructors class to store all constructor calls inside a dictionary and invoke them with nativeTypeNameStr as key.

A new internal constructor was added to the GodotObject class that accepts IntPtr to avoid the FormatterServices.GetUninitializedObject call.

@Delsin-Yu Delsin-Yu requested a review from a team as a code owner September 13, 2024 10:49
@Delsin-Yu Delsin-Yu force-pushed the generator-based-CreateManagedForGodotObjectBinding branch from 105a498 to efa64d0 Compare September 13, 2024 10:51
@akien-mga akien-mga added this to the 4.x milestone Sep 13, 2024
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.

Thank you so much, this looks great! I haven't been able to test it very thoroughly yet, but so far it doesn't seem to break anything.

@raulsntos raulsntos modified the milestones: 4.x, 4.4 Sep 15, 2024
@Delsin-Yu Delsin-Yu force-pushed the generator-based-CreateManagedForGodotObjectBinding branch 2 times, most recently from e4efd85 to 866b533 Compare September 15, 2024 23:33
@Delsin-Yu Delsin-Yu force-pushed the generator-based-CreateManagedForGodotObjectBinding branch 2 times, most recently from b2166ab to 4504408 Compare September 19, 2024 10:32
@Delsin-Yu
Copy link
Contributor Author

What?

image

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.

Looks good to me and seems to work fine in my testing, although I'm always wary of changes to the script bridge so more testing is always welcome.

@raulsntos raulsntos changed the title [.Net] Replace Reflection-Based implementation with Generated one in CreateManagedForGodotObjectBinding [.NET] Replace Reflection-Based implementation with Generated one in CreateManagedForGodotObjectBinding Sep 21, 2024
@Delsin-Yu
Copy link
Contributor Author

I'm currently testing it with our in-development game; once I identify any issues, I will push follow-up fixes (if not merged) or open a new PR (if merged).

@Delsin-Yu Delsin-Yu force-pushed the generator-based-CreateManagedForGodotObjectBinding branch 3 times, most recently from 782136b to 37b7d3e Compare September 22, 2024 09:55
@AThousandShips
Copy link
Member

Please edit your commit message to remove unnecessary information, just keeping the title itself, the individual squashed steps are not necessary

Co-authored-by: Raul Santos <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
@Delsin-Yu Delsin-Yu force-pushed the generator-based-CreateManagedForGodotObjectBinding branch from 37b7d3e to 3072249 Compare September 22, 2024 10:01
@akien-mga akien-mga merged commit ea8d20d into godotengine:master Sep 23, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Delsin-Yu Delsin-Yu deleted the generator-based-CreateManagedForGodotObjectBinding branch September 23, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants