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

Remove most EditorNode constructor parameters and fields #57306

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

trollodel
Copy link
Contributor

Follow up of #55066

Remove most EditorNode parameters and class attributes.
The only missing EditorNode references are:

  • EditorScript.
  • Plugins registered with EditorPlugins::add_by_type (only in some modules).
    Any reference to the given parameter is gone, and they were renamed to _p_editor.
  • EditorPlugins::creator, because, is a more important change that deserves its PR.

In some cases, I create local variables to shorten lines (since EditorNode::get_singleton() it's bigger than editor).


This PR has tons of changed files, Here's a summary of the changes to them:

  • InspectorDock, FileSystemDock, SceneTreeDock, CanvasItemEditor, Node3DEditor and related classes has notable changes due to several usages of EditorNode.
  • Some classes had from 1 to 4 usages of EditorNode.
  • Some classes use EditorNode only in the constructor and in the make_visible method (if it's a bottom panel plugin).
  • Some classes use EditorNode only in the constructor by calling EditorNode::add_bottom_panel_item.
  • Few classes don't use EditorNode at all.
  • AbstractPolygon2DEditor and subclasses no longer has the EditorNode parameter,
  • Module's editor plugins no longer use the EditorNode parameter, and that parameter was renamed from p_editor to _p_editor.
  • Removed the EditorNode reference in all headers (excluding EditorScript).

This PR increases the EditorNode::get_singleton usages, as expected.
In many cases, it's possible to use the EditorPlugin methods instead (which use EditorNode, but that's an implementation detail).
It will be done in a follow-up PR.

EditorNode::get_singleton usages in master (1894f3f):

rg "EditorNode::get_singleton()" | wc -l
745

EditorNode::get_singleton usages in this PR:

rg "EditorNode::get_singleton()" | wc -l
996

CC @KoBeWi

@Calinou Calinou added this to the 4.0 milestone Jan 27, 2022
@trollodel trollodel marked this pull request as ready for review January 27, 2022 11:57
@trollodel trollodel requested review from a team as code owners January 27, 2022 11:57
@trollodel trollodel force-pushed the remove_editornode_param branch from 926581f to c1da4c6 Compare February 12, 2022 11:05
@trollodel trollodel force-pushed the remove_editornode_param branch from c1da4c6 to 05b56f3 Compare February 14, 2022 13:16
@akien-mga
Copy link
Member

I like the idea. I've actually be poking at improving some include dependencies in editor following #57641, and there indeed plenty of unnecessary editor_node.h includes in headers due to this (and sometimes unnecessary dependencies in the .cpp too for plugins which don't actually need EditorNode at all).

akien-mga@7b724cd

There's a handful of plugins changed in the above commit to forward declare EditorNode but this becomes unnecessary after this PR.

@trollodel
Copy link
Contributor Author

trollodel commented Feb 14, 2022

Once I'll migrate editor classes to use EditorPlugin and I'll remove the EditorNode usages in "editor_plugin.h", most editor classes won't need to interact (directly) with EditorNode at all.

@akien-mga akien-mga merged commit f810f76 into godotengine:master Feb 14, 2022
@akien-mga
Copy link
Member

Thanks!

@@ -87,7 +87,6 @@ class AbstractPolygon2DEditor : public HBoxContainer {
bool _polygon_editing_enabled;

CanvasItemEditor *canvas_item_editor;
EditorNode *editor;
Copy link
Member

Choose a reason for hiding this comment

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

There is an unnecessary EditorNode forward declaration further up. If you are removing editor variable, you should also remove all EditorNode references that aren't necessary anymore. There might be some unused includes now too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but this is not the last cleanup PR I will do :)

There might be some unused includes now too.

Unused probably not. But some that can be moved to the .cpp? Surely yes!

@trollodel trollodel deleted the remove_editornode_param branch February 14, 2022 15:43
@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2022

I was still reviewing this .-.
But well, it's fine overall, there are just some leftover like I commented above.

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