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

C#: Generate instance types for singletons #79470

Merged

Conversation

raulsntos
Copy link
Member

Singleton classes in C# are currently exposed as static classes. In some cases these classes are inherited (e.g.: PhysicsServer2DExtension derives from the singleton class PhysicsServer2D).

We currently ignore these classes and don't generate them, because C# doesn't allow inheriting static classes. In order to fix this, with this PR C# now generates additional Instance classes for every singleton. E.g.:

// Good old static singleton class. Still generated.
public static class PhysicsServer2D { ... }

// New singleton instance class.
public class PhysicsServer2DInstance { ... }

// Now we can generate PhysicsServer2DExtension, deriving from the instance class.
public class PhysicsServer2DExtension : PhysicsServer2DInstance { ... }

Footnotes

  1. This would break compatibility and won't happen in 4.x, but in 5.0 we could remove the static classes, rename the instance classes to remove the Instance suffix and create a Singletons static class that can be used with global using. Similar to how singletons are exposed in GDScript using @GlobalScope. See https://github.com/godotengine/godot/pull/59208#issuecomment-1069583895.

@raulsntos raulsntos added this to the 4.x milestone Jul 14, 2023
@raulsntos raulsntos requested a review from a team as a code owner July 14, 2023 16:27
@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from 3052fc1 to e4fabdc Compare July 14, 2023 16:30
@YuriSizov
Copy link
Contributor

Does this help with resolving compatibility issues from #75694 ?

@raulsntos
Copy link
Member Author

As I understand it, #75694 changes EditorInterface to be a singleton. This means in C# the EditorInterface becomes static which breaks compat. This PR doesn't help with that, the EditorInterface would still be generated as a static class after #75694.

However, with this PR we generate an additional EditorInterfaceInstance class which is not static and would be essentially the same class that existed in 4.1. So I could hardcode this class so it doesn't add the Instance suffix and to avoid generating the static class, then it should avoid breaking compat.

@YuriSizov
Copy link
Contributor

However, with this PR we generate an additional EditorInterfaceInstance class which is not static and would be essentially the same class that existed in 4.1. So I could hardcode this class so it doesn't add the Instance suffix and to avoid generating the static class, then it should avoid breaking compat.

That sounds like a great idea, if it's possible. I think if we are going to merge this PR, we should merge it first, then you can suggest me how to update mine to make address the issue.

@raulsntos
Copy link
Member Author

I've added a commit with the some of the changes suggested by @neikeq in #75694 (comment). This means that now EditorInterface will still be generated as a non-static class, but your PR should not remove the get_editor_interface() methods to avoid breaking compat entirely.

@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from 40dbc35 to 64d8d34 Compare July 14, 2023 18:26
@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from 64d8d34 to 619a029 Compare July 14, 2023 18:35
@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from 619a029 to 64dc4f1 Compare July 15, 2023 11:31
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 4, 2023
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

Some more minor suggestions, but I (still) cannot think of a fundamentally better design for this.

modules/mono/editor/bindings_generator.cpp Show resolved Hide resolved
modules/mono/editor/bindings_generator.h Show resolved Hide resolved
modules/mono/editor/bindings_generator.cpp Outdated Show resolved Hide resolved
@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from 64dc4f1 to d5a18c6 Compare August 5, 2023 22:49
Co-authored-by: Ignacio Etcheverry <ignalfonsore@gmail.com>
@raulsntos raulsntos force-pushed the dotnet/singleton-can-be-instances-too branch from d5a18c6 to 23f7f24 Compare August 6, 2023 17:03
@akien-mga akien-mga merged commit 8018b47 into godotengine:master Aug 7, 2023
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/singleton-can-be-instances-too branch August 7, 2023 19:32
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