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

Generate C# singletons as non-static classes #59208

Closed

Conversation

raulsntos
Copy link
Member

Singleton classes are no longer generated as static classes so they can be inherited.

This fixes mono after #59140 (introduces a class PhysicsServer3DExtension that inherits from PhysicsServer3D which is a singleton class) and re-enables mono CI.

The disadvantage of this is we now have to access the singleton class using the Singleton property everytime, which is cumbersome, but it's consistent with how it works in the engine and godot-cpp (using the get_singleton method).

@raulsntos raulsntos added this to the 4.0 milestone Mar 16, 2022
@raulsntos raulsntos force-pushed the fix-csharp-singletons branch from 1bd42bc to 921496e Compare March 16, 2022 18:53
@@ -1889,7 +1887,7 @@ Error BindingsGenerator::_generate_cs_method(const BindingsGenerator::TypeInterf
p_output.append(MEMBER_BEGIN);
p_output.append(p_imethod.is_internal ? "internal " : "public ");

if (p_itype.is_singleton) {
if (p_itype.name == "Engine" && p_imethod.name == "get_singleton") {
Copy link
Member Author

Choose a reason for hiding this comment

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

Engine.GetSingleton is still generated as static to avoid an infinite loop.

@raulsntos raulsntos marked this pull request as ready for review March 16, 2022 18:58
@raulsntos raulsntos requested review from a team as code owners March 16, 2022 18:58
Singleton classes are no longer generated as static classes so they can
be inherited.
@raulsntos raulsntos force-pushed the fix-csharp-singletons branch from 921496e to 99546cf Compare March 16, 2022 19:18
@neikeq
Copy link
Contributor

neikeq commented Mar 16, 2022

The disadvantage of this is we now have to access the singleton class using the Singleton property everytime, which is cumbersome, but it's consistent with how it works in the engine and godot-cpp (using the get_singleton method).

This is precisely why I made them static classes. It's a pretty big downside. I'll see what can be done about PhysicsServer3DExtension.

@neikeq
Copy link
Contributor

neikeq commented Mar 16, 2022

Another possible solution, if we can't find a different one, is to have a @GlobalScope-like class with static fields for each singleton. C# 10 introduces "implicit usings", which means we can add an implicit using static Godot.GlobalScope; and make it be exactly like GDScript. Although, we would need to move these classes to a different namespace, which won't be easy (Not just in the bindings generators, but other places that assume the namespace as well. It should be much easier once we merge the dotnet6 branch with master).

@neikeq
Copy link
Contributor

neikeq commented Mar 16, 2022

For now, let's just ignore classes that inherit from singletons.

EDIT: We don't want to ignore all of those, as to not silence possible future regressions. Instead, let's hard-code a list of classes that should be ignored, with PhysicsServer3DExtension on it.

@raulsntos
Copy link
Member Author

Superseeded by #59286

@raulsntos raulsntos closed this Mar 18, 2022
@YuriSizov YuriSizov removed this from the 4.0 milestone Jul 14, 2023
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