Store panels and docks instances in their classes #55066

trollodel · 2021-11-17T20:16:29Z

Based on the work in #47520

This is a cleanup PR that reduces a bit the editor code dependencies to EditorNode.
It moves all the panel and dock instances, currently stored in EditorNode in their classes as a singleton.

This is one approach (IMO the simplest). The other approach is to store the node instances in a dedicated class called EditorSingletons or something like that (closer to what I did in #47520).

KoBeWi · 2022-01-11T15:23:18Z

editor/node_dock.cpp

+NodeDock::~NodeDock() {
+	singleton = nullptr;
+}


This could be in the header, like in other classes.

Now that I checked more carefully, this case is the only one when I need to add a destructor and I add it into the cpp like the other destructors I modified to be consistent. However, I don't mind moving all the destructor I changed to the header.

KoBeWi · 2022-01-11T15:31:36Z

Another thing that would be nice to do is unifying the usage of EditorNode singleton. Many plugins will have an editor property. EditorNode is passed in the constructor and saved to that property, but then it's only used half of the time, because the class sometimes still accesses the editor via singleton. I'd remove all these "cached" editors and just always access it using singleton. This way these classes won't need the EditorNode to be passed with constructor for no real reason.

trollodel · 2022-01-11T16:37:51Z

@KoBeWi definitely agree. In fact, this is only the first of some editor related cleanup PRs I want to do, and the EditorNode parameter thing is something I already noted. I waited to do more work because I was unsure if this is desired.

KoBeWi · 2022-01-11T16:45:26Z

I waited to do more work because I was unsure if this is desired.

tbh, it's better if you waited a bit longer, so the PR doesn't end like the previous one .-. I'll add a PR meeting label.

akien-mga · 2022-01-20T16:40:18Z

We discussed this today in a PR review meeting and agreed with the change.

In fact, this is only the first of some editor related cleanup PRs I want to do, and the EditorNode parameter thing is something I already noted. I waited to do more work because I was unsure if this is desired.

We also intend to cleanup EditorNode at some point, but we decided to postpone that work to after 4.0 is released because we don't have much bandwidth for this in the core team and a lot of pending 4.0 items to finish. So you're welcome to work on this, but be aware that most contributors likely won't have much bandwidth to review it for several months - so you might also prefer to wait for a more opportune time to participate in and/or lead discussions with core contributors and then get the agreed-upon changes done.

akien-mga

Should probably be rebased, then it should be good to merge if it's ready.

KoBeWi · 2022-01-20T16:48:14Z

The editor changes I mentioned could be done now, no? It doesn't require EditorNode modifications.

trollodel · 2022-01-20T19:39:26Z

We also intend to cleanup EditorNode at some point, but we decided to postpone that work to after 4.0 is released because we don't have much bandwidth for this in the core team and a lot of pending 4.0 items to finish. So you're welcome to work on this, but be aware that most contributors likely won't have much bandwidth to review it for several months - so you might also prefer to wait for a more opportune time to participate in and/or lead discussions with core contributors and then get the agreed-upon changes done.

I didn't want to do massive, risky cleanup PRs for several reasons, including this one. So, I think it's better to start with low risks PRs like this one and the future editor parameter PR (so @KoBeWi is happy 😂).
In any case, feel free to ping me on RC when the topic is raised.

akien-mga · 2022-01-20T19:41:08Z

The editor changes I mentioned could be done now, no? It doesn't require EditorNode modifications.

Yeah for sure. As long as there are people available to review low-impact and consensual changes, that's definitely fine.

akien-mga · 2022-01-20T21:29:20Z

Thanks!

TokageItLab · 2022-01-21T06:12:18Z

editor/plugins/animation_player_editor_plugin.cpp

@@ -1794,9 +1794,9 @@ void AnimationPlayerEditorPlugin::_notification(int p_what) {
 	switch (p_what) {
 		case NOTIFICATION_ENTER_TREE: {
 			Node3DEditor::get_singleton()->connect("transform_key_request", callable_mp(this, &AnimationPlayerEditorPlugin::_transform_key_request));
-			editor->get_inspector()->connect("property_keyed", callable_mp(this, &AnimationPlayerEditorPlugin::_property_keyed));
+			InspectorDock::get_singleton()->connect("property_keyed", callable_mp(this, &AnimationPlayerEditorPlugin::_property_keyed));


It caused bug, since editor->get_inspector() and editor->get_inspector_dock() is different.

I probably missed it in the rebase. Thanks for the fix.

…55066

trollodel requested review from a team as code owners November 17, 2021 20:16

YeldhamDev added enhancement topic:editor labels Nov 17, 2021

YeldhamDev added this to the 4.0 milestone Nov 17, 2021

trollodel force-pushed the less_singletons_in_editornode branch from f6e3b8d to 9b92278 Compare November 21, 2021 10:51

trollodel requested a review from a team as a code owner November 21, 2021 10:51

KoBeWi reviewed Jan 11, 2022

View reviewed changes

KoBeWi added the for pr meeting label Jan 11, 2022

akien-mga approved these changes Jan 20, 2022

View reviewed changes

Store panels and docks singletons in their own classes

aa1102f

trollodel force-pushed the less_singletons_in_editornode branch from 9b92278 to aa1102f Compare January 20, 2022 19:15

akien-mga merged commit e6170aa into godotengine:master Jan 20, 2022

TokageItLab reviewed Jan 21, 2022

View reviewed changes

TokageItLab added a commit to TokageItLab/godot that referenced this pull request Jan 21, 2022

Fixed target which connected animation player, follow up godotengine#…

b3a800b

…55066

TokageItLab mentioned this pull request Jan 21, 2022

Fixed wrong target object which connected animation player #57031

Merged

Calinou removed the for pr meeting label Jan 22, 2022

trollodel mentioned this pull request Jan 27, 2022

Remove most EditorNode constructor parameters and fields #57306

Merged

volokh0x mentioned this pull request Jan 28, 2022

Update InspectorDock when moving files via FileSystemDock #57244

Open

trollodel deleted the less_singletons_in_editornode branch February 11, 2022 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store panels and docks instances in their classes #55066

Store panels and docks instances in their classes #55066

trollodel commented Nov 17, 2021

KoBeWi Jan 11, 2022

trollodel Jan 11, 2022

KoBeWi commented Jan 11, 2022 •

edited

Loading

trollodel commented Jan 11, 2022

KoBeWi commented Jan 11, 2022

akien-mga commented Jan 20, 2022

akien-mga left a comment

KoBeWi commented Jan 20, 2022

trollodel commented Jan 20, 2022

akien-mga commented Jan 20, 2022

akien-mga commented Jan 20, 2022

TokageItLab Jan 21, 2022 •

edited

Loading

trollodel Jan 21, 2022

Store panels and docks instances in their classes #55066

Store panels and docks instances in their classes #55066

Conversation

trollodel commented Nov 17, 2021

KoBeWi Jan 11, 2022

Choose a reason for hiding this comment

trollodel Jan 11, 2022

Choose a reason for hiding this comment

KoBeWi commented Jan 11, 2022 • edited Loading

trollodel commented Jan 11, 2022

KoBeWi commented Jan 11, 2022

akien-mga commented Jan 20, 2022

akien-mga left a comment

Choose a reason for hiding this comment

KoBeWi commented Jan 20, 2022

trollodel commented Jan 20, 2022

akien-mga commented Jan 20, 2022

akien-mga commented Jan 20, 2022

TokageItLab Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

trollodel Jan 21, 2022

Choose a reason for hiding this comment

KoBeWi commented Jan 11, 2022 •

edited

Loading

TokageItLab Jan 21, 2022 •

edited

Loading