-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Make EditorInterface
accessible as a singleton
#75694
Make EditorInterface
accessible as a singleton
#75694
Conversation
21e85ee
to
24da596
Compare
I initially wanted to preserve the This actually means that this PR breaks C# compatibility no matter what. Not sure how much of a problem we would consider that. Updating user projects to use the new API is trivial. But the ABI compatibility is just going to be broken regardless. In the meantime, to make the tests pass and keep this compatible for GDScript at least, I'm changing the return type of the method to PS. In |
d01f12c
to
76dd2c1
Compare
For C# we could add a hard-coded list of singleton classes that must not be generated as static classes. That way we avoid breaking compat until the next major release. The singleton could still be accessed globally via |
We discussed the situation with @akien-mga and the rest of the production team a bit, and I think that it's better to break compatibility by removing the existing method than to create workarounds and hacks to make it work. For our end-users the changes would be minimal, but yes, they'd need to use some updated assemblies. On the plus side, the singleton introduced in this PR will work just like any other singleton, as a static class, convenient and consistent. |
76dd2c1
to
5ca6ca9
Compare
EditorInterface
globally accessibleEditorInterface
accessible as a singleton
I'd really prefer not to break compat. For Godot 5, I'll probably change the way we generate C# singletons to avoid this problem, but for Godot 4 it's better to go with a workaround. Here's the patch I was referring to: neikeq@29a733c. I can push it to your branch or make a follow-up PR (or you are welcome to just add them to your commit if you want). The class name has to be added to the list, so it's still possible to miss this in the future and accidentally break compat, but in the coming days I'll add a CI step to check for C# API compatibility. |
Do you think it's worth preserving compatibility for a very small number of users who create editor plugins with C# by making the API inconsistent and overall coding experience for them worse? The reason why we concluded that it's better to break backwards compatibility here is because the impact of it is small, but we avoid creating workarounds, hacks, and inconsistencies. In this case the benefit from preserving compatibility seems smaller than from keeping the API consistent. |
Unlike GDScript, breaking backwards compatibility in C# can have very negative results, like crashes. Just as it would be to break compatibility in GDExtension. I don't think this inconsistency will be that annoying to the user considering new singletons classes should not be that common, at least until Godot 5. I really wanted to take compatibility seriously for C# in Godot 4. We're going to have a CI step to ensure this, and I don't know if we can tell the .NET compat tool to ignore stuff. |
GDExtensions are still in a beta phase, so compatibility there is not as important. We also can't really ensure ABI compatibility with such a vast API surface. That said, GDExt has a mechanism to keep compatibility pointers, so to some extend it can be preserved. But there are going to be other changes that we have announced ahead of time. Some parts of the public API, marked as experimental, will be replaced and complete BC will not always be possible. Which is why we target a minor release for this, not a patch release. Bug fixes will also break compatibility, and already have, where incorrect types were specified for arguments and return values. So if any change, even unrelated, can result in crashes in our C# API, then we need to find a way to better protect users from using outdated assemblies. |
5ca6ca9
to
0500733
Compare
I don't see why we can't, as long as we have a CI check to ensure this. The question is whether or not we care.
I truly hope that doesn't end up being the case. I talked about this with @reduz a few times before, and there really seemed to be a commitment to ensure backwards compatibility. We even intended to add a CI check for 4.0. This didn't end up happening, but I'm going to add one for C#. When you say some APIs are marked as experimental, do you mean that this is how we communicated it or is there an actual flag in ClassDB I can query to know whether an API is experimental?
There are some types of changes I'm aware I can't really ask Godot not to make, and I have a plan to ensure they don't break C# ABI compatibility. Since GDScript doesn't have method overloads, adding new optional parameters is the norm to avoid breaking GDScript compatibility. It does break C# compatibility though (and other languages), but I already have a solution for that (adding a method overload for each optional parameter). Regarding the change of types in signatures. I think this already shows little interest in backwards compatibility, but if it's going to happen I can work on a solution within C# as well. |
We communicated it, and there is a flag in documentation. But not in ClassDB. Same for deprecated.
Well, in the cases that I refer to there is no point in keeping compatibility because that was invalid code. This in unavoidable, and I don't think it makes much sense to try and keep invalid method signatures.
There absolutely is desire to take ABI BC seriously, but there are things that are going to break it anyway. Going forward, we can mitigate them, but we can't really protect every possible language out there. C#, of course, should be treated on the same level as GDScript, but I don't see a way in this specific situation that is not a compromise. And if we have to compromise I'd sacrifice BC rather than code consistency and coding experience. Edit: I can suggest we keep |
I don't see what's the problem with the patch to the C# bindings. It's simple and solves the issue (and won't be needed in Godot 5). Adding a new class for the singleton sounds worse than the slight inconsistency with Not that I'm against doing that. I just think the C# patch seems less intrusive to the rest of the engine. |
TBH, I don't see how such a patch can be C# only, as yuri noted, gdscript doesn't like returning singleton instances either and third-party language bindings probably made similar design decisions and would need similar workarounds. The easiest backwards compatible change would probably be to just add a static Also I think C# is probably one of the better languages to break like this, since you generally get all the errors at compile time, rather than if that code actually gets executed. Also even if you load a precompiled binary, you get a pretty clear |
I don't understand the part about GDScript. There may be some confusion there. My patch can be C#-only because the compatibility issue is caused by how we implemented singletons in C#. We went with static classes because Regarding other languages, Godot singletons are normal classes, and the constructor is not private (unlike the singleton pattern) so adding a new singleton doesn't necessarily break compatibility. But ultimately, it depends on how these third-party bindings implement singletons. |
There was a problem hiding this 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, if it gets the OK from the .NET team.
Ah, right, we still have this check on the CI:
It exists specifically for C# I think, what do we want to do about it? |
Would be good to evaluate how singleton access works in GDExtension too to make sure this is compatible. |
1eb5721
to
a59f1e1
Compare
I (temporarily) removed the check for singletons. If we want to keep it, it needs to sync somehow with the list of compat singletons from C# bindings. But that may be better done as a separate C# API compatibility check rather than a general test for ClassDB. |
Returning a singleton type should no longer be a concern in C# after #79470, so I don't think we need the check (at least for C#'s sake). What we need to check going forward is that turning an existing type into a singleton would break compat unless it's added to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a few changes in the C# documentation, but otherwise looks good to me.
- EditorPlugin.get_editor_interface() is removed as redundant.
a59f1e1
to
951ea24
Compare
It occurs to me that we could remove them as long as the they're provided as compat methods. This diff removes them and adds compat methods for GDExtension and C#: diff --git a/doc/classes/EditorPlugin.xml b/doc/classes/EditorPlugin.xml
index 942a5ca0dd..b73612d776 100644
--- a/doc/classes/EditorPlugin.xml
+++ b/doc/classes/EditorPlugin.xml
@@ -558,13 +558,6 @@
The callback should have 4 arguments: [Object] [code]undo_redo[/code], [Object] [code]modified_object[/code], [String] [code]property[/code] and [Variant] [code]new_value[/code]. They are, respectively, the [UndoRedo] object used by the inspector, the currently modified object, the name of the modified property and the new value the property is about to take.
</description>
</method>
- <method name="get_editor_interface" is_deprecated="true">
- <return type="EditorInterface" />
- <description>
- Returns the [EditorInterface] singleton instance.
- [i]Deprecated.[/i] [EditorInterface] is a global singleton and can be accessed directly by its name.
- </description>
- </method>
<method name="get_export_as_menu">
<return type="PopupMenu" />
<description>
diff --git a/doc/classes/EditorScript.xml b/doc/classes/EditorScript.xml
index 8033c18918..e61d3ac25a 100644
--- a/doc/classes/EditorScript.xml
+++ b/doc/classes/EditorScript.xml
@@ -48,13 +48,6 @@
[b]Warning:[/b] The implementation of this method is currently disabled.
</description>
</method>
- <method name="get_editor_interface" qualifiers="const" is_deprecated="true">
- <return type="EditorInterface" />
- <description>
- Returns the [EditorInterface] singleton instance.
- [i]Deprecated.[/i] [EditorInterface] is a global singleton and can be accessed directly by its name.
- </description>
- </method>
<method name="get_scene" qualifiers="const">
<return type="Node" />
<description>
diff --git a/editor/editor_plugin.compat.inc b/editor/editor_plugin.compat.inc
new file mode 100644
index 0000000000..03dd420fa9
--- /dev/null
+++ b/editor/editor_plugin.compat.inc
@@ -0,0 +1,41 @@
+/**************************************************************************/
+/* editor_plugin.compat.inc */
+/**************************************************************************/
+/* This file is part of: */
+/* GODOT ENGINE */
+/* https://godotengine.org */
+/**************************************************************************/
+/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
+/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
+/* */
+/* Permission is hereby granted, free of charge, to any person obtaining */
+/* a copy of this software and associated documentation files (the */
+/* "Software"), to deal in the Software without restriction, including */
+/* without limitation the rights to use, copy, modify, merge, publish, */
+/* distribute, sublicense, and/or sell copies of the Software, and to */
+/* permit persons to whom the Software is furnished to do so, subject to */
+/* the following conditions: */
+/* */
+/* The above copyright notice and this permission notice shall be */
+/* included in all copies or substantial portions of the Software. */
+/* */
+/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
+/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
+/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
+/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
+/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
+/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
+/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+/**************************************************************************/
+
+#ifndef DISABLE_DEPRECATED
+
+EditorInterface *EditorPlugin::_get_editor_interface_bind_compat_75694() {
+ return EditorInterface::get_singleton();
+}
+
+void EditorPlugin::_bind_compatibility_methods() {
+ ClassDB::bind_compatibility_method(D_METHOD("get_editor_interface"), &EditorPlugin::_get_editor_interface_bind_compat_75694);
+}
+
+#endif // DISABLE_DEPRECATED
diff --git a/editor/editor_plugin.cpp b/editor/editor_plugin.cpp
index 2d4c07b263..d00c7bdbd0 100644
--- a/editor/editor_plugin.cpp
+++ b/editor/editor_plugin.cpp
@@ -29,6 +29,7 @@
/**************************************************************************/
#include "editor_plugin.h"
+#include "editor_plugin.compat.inc"
#include "editor/debugger/editor_debugger_node.h"
#include "editor/editor_file_system.h"
@@ -503,10 +504,6 @@ void EditorPlugin::hide_bottom_panel() {
EditorNode::get_singleton()->hide_bottom_panel();
}
-EditorInterface *EditorPlugin::get_editor_interface() {
- return EditorInterface::get_singleton();
-}
-
ScriptCreateDialog *EditorPlugin::get_script_create_dialog() {
return SceneTreeDock::get_singleton()->get_script_create_dialog();
}
@@ -587,7 +584,6 @@ void EditorPlugin::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_input_event_forwarding_always_enabled"), &EditorPlugin::set_input_event_forwarding_always_enabled);
ClassDB::bind_method(D_METHOD("set_force_draw_over_forwarding_enabled"), &EditorPlugin::set_force_draw_over_forwarding_enabled);
- ClassDB::bind_method(D_METHOD("get_editor_interface"), &EditorPlugin::get_editor_interface);
ClassDB::bind_method(D_METHOD("get_script_create_dialog"), &EditorPlugin::get_script_create_dialog);
ClassDB::bind_method(D_METHOD("add_debugger_plugin", "script"), &EditorPlugin::add_debugger_plugin);
ClassDB::bind_method(D_METHOD("remove_debugger_plugin", "script"), &EditorPlugin::remove_debugger_plugin);
diff --git a/editor/editor_plugin.h b/editor/editor_plugin.h
index 7dcf62144d..09cc04732f 100644
--- a/editor/editor_plugin.h
+++ b/editor/editor_plugin.h
@@ -69,6 +69,12 @@ protected:
void _notification(int p_what);
static void _bind_methods();
+
+#ifndef DISABLE_DEPRECATED
+ EditorInterface *_get_editor_interface_bind_compat_75694();
+ static void _bind_compatibility_methods();
+#endif
+
EditorUndoRedoManager *get_undo_redo();
void add_custom_type(const String &p_type, const String &p_base, const Ref<Script> &p_script, const Ref<Texture2D> &p_icon);
@@ -189,7 +195,6 @@ public:
virtual void edited_scene_changed() {} // if changes are pending in editor, apply them
virtual bool build(); // builds with external tools. Returns true if safe to continue running scene.
- EditorInterface *get_editor_interface();
ScriptCreateDialog *get_script_create_dialog();
void add_undo_redo_inspector_hook_callback(Callable p_callable);
diff --git a/editor/editor_script.compat.inc b/editor/editor_script.compat.inc
new file mode 100644
index 0000000000..e8a67ec103
--- /dev/null
+++ b/editor/editor_script.compat.inc
@@ -0,0 +1,41 @@
+/**************************************************************************/
+/* editor_script.compat.inc */
+/**************************************************************************/
+/* This file is part of: */
+/* GODOT ENGINE */
+/* https://godotengine.org */
+/**************************************************************************/
+/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
+/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
+/* */
+/* Permission is hereby granted, free of charge, to any person obtaining */
+/* a copy of this software and associated documentation files (the */
+/* "Software"), to deal in the Software without restriction, including */
+/* without limitation the rights to use, copy, modify, merge, publish, */
+/* distribute, sublicense, and/or sell copies of the Software, and to */
+/* permit persons to whom the Software is furnished to do so, subject to */
+/* the following conditions: */
+/* */
+/* The above copyright notice and this permission notice shall be */
+/* included in all copies or substantial portions of the Software. */
+/* */
+/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
+/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
+/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
+/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
+/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
+/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
+/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+/**************************************************************************/
+
+#ifndef DISABLE_DEPRECATED
+
+EditorInterface *EditorScript::_get_editor_interface_bind_compat_75694() {
+ return EditorInterface::get_singleton();
+}
+
+void EditorScript::_bind_compatibility_methods() {
+ ClassDB::bind_compatibility_method(D_METHOD("get_editor_interface"), &EditorScript::_get_editor_interface_bind_compat_75694);
+}
+
+#endif // DISABLE_DEPRECATED
diff --git a/editor/editor_script.cpp b/editor/editor_script.cpp
index 4e8c5ad8b5..d7a6715be8 100644
--- a/editor/editor_script.cpp
+++ b/editor/editor_script.cpp
@@ -29,6 +29,7 @@
/**************************************************************************/
#include "editor_script.h"
+#include "editor_script.compat.inc"
#include "editor/editor_interface.h"
#include "editor/editor_node.h"
@@ -56,10 +57,6 @@ Node *EditorScript::get_scene() const {
return EditorNode::get_singleton()->get_edited_scene();
}
-EditorInterface *EditorScript::get_editor_interface() const {
- return EditorInterface::get_singleton();
-}
-
void EditorScript::run() {
if (!GDVIRTUAL_CALL(_run)) {
EditorNode::add_io_error(TTR("Couldn't run editor script, did you forget to override the '_run' method?"));
@@ -69,7 +66,6 @@ void EditorScript::run() {
void EditorScript::_bind_methods() {
ClassDB::bind_method(D_METHOD("add_root_node", "node"), &EditorScript::add_root_node);
ClassDB::bind_method(D_METHOD("get_scene"), &EditorScript::get_scene);
- ClassDB::bind_method(D_METHOD("get_editor_interface"), &EditorScript::get_editor_interface);
GDVIRTUAL_BIND(_run);
}
diff --git a/editor/editor_script.h b/editor/editor_script.h
index d7c813261d..8e7a9d9f1d 100644
--- a/editor/editor_script.h
+++ b/editor/editor_script.h
@@ -44,12 +44,16 @@ class EditorScript : public RefCounted {
protected:
static void _bind_methods();
+#ifndef DISABLE_DEPRECATED
+ EditorInterface *_get_editor_interface_bind_compat_75694();
+ static void _bind_compatibility_methods();
+#endif
+
GDVIRTUAL0(_run)
public:
void add_root_node(Node *p_node);
Node *get_scene() const;
- EditorInterface *get_editor_interface() const;
virtual void run();
diff --git a/modules/mono/glue/GodotSharp/GodotSharpEditor/Compat.cs b/modules/mono/glue/GodotSharp/GodotSharpEditor/Compat.cs
index 7a3bb0df7e..f51fa2b279 100644
--- a/modules/mono/glue/GodotSharp/GodotSharpEditor/Compat.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharpEditor/Compat.cs
@@ -7,6 +7,34 @@ using System.ComponentModel;
namespace Godot;
+partial class EditorPlugin
+{
+ /// <summary>
+ /// <para>Returns the <see cref="Godot.EditorInterface"/> singleton instance.</para>
+ /// <para><i>Deprecated.</i> <see cref="Godot.EditorInterface"/> is a global singleton and can be accessed directly by its name.</para>
+ /// </summary>
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ [Obsolete("Use EditorInterface.Singleton.")]
+ public EditorInterface GetEditorInterface()
+ {
+ return EditorInterface.Singleton;
+ }
+}
+
+partial class EditorScript
+{
+ /// <summary>
+ /// <para>Returns the <see cref="Godot.EditorInterface"/> singleton instance.</para>
+ /// <para><i>Deprecated.</i> <see cref="Godot.EditorInterface"/> is a global singleton and can be accessed directly by its name.</para>
+ /// </summary>
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ [Obsolete("Use EditorInterface.Singleton.")]
+ public EditorInterface GetEditorInterface()
+ {
+ return EditorInterface.Singleton;
+ }
+}
+
partial class EditorUndoRedoManager
{
/// <inheritdoc cref="CreateAction(string, UndoRedo.MergeMode, GodotObject, bool)"/>
However, I still get these validation errors and I don't know how to fix them:
It seems like this would be the first API that we remove and provide compat methods for, all other APIs that were removed before were intentionally breaking compat so a compat method was never provided. I wonder if the compat system doesn't support this scenario. |
That's curious, but I'd rather look into it in a follow-up, if there are no other problems with this right now. |
From the perspective of GDExtension in isolation, I think this is fine. I don't think there's anything in the GDExtension interface that would have a problem with a singleton instance being returned by an ordinary method. However, similar to the problems that came up for C#, there may be specific GDExtension bindings (like godot-cpp, godot-rust, etc) that made the assumption that singleton instances can't be passed around and exposed them like static classes, and this could break their code generators. I think this should be fine for godot-cpp - I just tried building it with the Personally, I'd be fine with making this change even if some GDExtension bindings would need to update their code generators. This won't break binary compatibility and we never really promised that singletons couldn't be passed as instances. But this is probably something that more GDExtension team members should weight in on! |
Thanks for the mention! In godot-rust, we currently treat singletons like regular instances, so it should be OK right now. We have been considering to move towards static-method syntax, and for that it's good to consider cases like this one 👍 So to clarify:
|
Since in the engine singletons are just your normally instanced classes with a static instance reference, it can happen quite easily. In fact, there are some classes that can be freely instantiated while also having a singleton reference. That's unlikely to be needed in the public API, but not impossible. |
Thanks! |
Supersedes #66109 and part of #68696. Closes #68685.
It was previously impossible to access this editor-specific singleton without first creating an instance of
EditorScript
orEditorPlugin
. Which led to hacks, where users would create such instances ad-hoc in their tool scripts. And within the editor plugins you would need to carry a reference everywhere just to have this access. All while theEditorInterface
object was internally a singleton, and accessible throughout the editor code directly.So I made it a singleton officially, in the API. Naturally, like any other editor class it is only available in editor builds. So if you end up relying on it, guard your code accordingly.
In the future we could probably refine the exposed API more, make use of the editor-specific singletons more too.