Cleanup EditorNode, EditorHistory, EditorSelection and EditorData #58395

Geometror · 2022-02-21T17:45:22Z

This PR aims to tidy-up the EditorNode, EditorData, EditorSelection and EditorHistory classes (partially) with the following changes:

Remove unused enums (unused HISTORY_MAX constant)
Remove unused class attributes
Remove some old commented out code (reenabling it caused causes undesired behavior, so it seems fine to remove it)
Some renames of variables to be more descriptive/explicit (e.g., the term "history" was used ambiguously)
- Rename EditorHistory to EditorSelectionHistory to better reflect what it does (this might require some discussion, as I am not quite sure with it)
Adjusted the comment style for consistency (// Begin with whitespace + uppercase letter and place a dot at the end.)
Removed some uninformative comments (e.g., //bye, //save after save();)
Restructure the EditorNode header to improve readability
- Clearly separate class attributes and methods
- Move enum and struct declarations
- Group static methods
- Group attributes/methods in a way that makes sense (I hope)
Reimplement rendering_driver check in EditorNode::_rendering_driver_selected (old commented out version)
Add TODO/FIXME in front of some comments which were (implicitly or explicitly) suggesting changes/marking hacks.
Remove an unnecessary visibility modifier since the GDCLASS macro includes "private:" at the end
Includes almost all changes in Refactored EditorHistory, removed unused code, added documentation. #44222 and Removed unused code from EditorSelection and added documentation. #44249 except the simplifications using the ERR_FAIL_INDEX macro, therefore co-authored by @EricEzaM.

EricEzaM · 2022-02-22T21:35:16Z

You may be interested in having at a look at these PRs and seeing if there are any changes worth bringing into this one (I just never got around to updating them) #44222 #44249

Geometror · 2022-03-30T00:08:13Z

Rebased and incorporated some of the changes made in #44222 and #44249 (basically all of it except the ERR_FAIL_INDEX macro additions/simplifications since they were used where negative values indicate a valid state, resulting in error spamming).

akien-mga · 2022-03-30T09:50:06Z

editor/editor_node.cpp

 			//		case SET_VIDEO_DRIVER_SAVE_AND_RESTART: {
 			//			ProjectSettings::get_singleton()->set("rendering/driver/driver_name", video_driver_request);
 			//			save_all_scenes();


This is dead code that can be removed.

akien-mga

Looks pretty good to me. It makes variables and comments clearer, and the API renames and refactoring seems useful/clearer.

KoBeWi · 2022-03-30T11:25:54Z

editor/editor_node.cpp

+	scene->set_meta("__editor_run_settings__", Variant());
 	scene->set_meta("__editor_plugin_states__", Variant());


This could be changed to remove_meta()

Just did a quick search and it looks like this is actually the only place where these meta values are used, so I guess we could remove them completely.

Indeed:

$ rg __editor_run_settings__ editor/editor_node.cpp 1396: scene->set_meta("__editor_run_settings__", Variant()); // clear it (no point in keeping it) $ rg __editor_plugin_states__ editor/editor_node.cpp 1397: scene->set_meta("__editor_plugin_states__", Variant());

Removed it.

KoBeWi · 2022-03-30T11:29:31Z

editor/editor_node.cpp

@@ -2289,7 +2289,7 @@ void EditorNode::_edit_current(bool p_skip_foreign) {
 		InspectorDock::get_inspector_singleton()->set_use_folding(!disable_folding);
 	}

-	/* Take care of PLUGIN EDITOR */
+	// Take care of PLUGIN EDITOR.


This comment is odd. What is PLUGIN EDITOR?

You're right, this is odd. We could change it to // Take care of the main editor plugin.

KoBeWi · 2022-03-30T11:32:20Z

editor/editor_node.cpp

@@ -3366,14 +3365,14 @@ void EditorNode::set_addon_plugin_enabled(const String &p_addon, bool p_enabled,

 	EditorPlugin *ep = memnew(EditorPlugin);
 	ep->set_script(script);
-	plugin_addons[p_addon] = ep;
+	name_to_editor_plugin[p_addon] = ep;


name_to_editor_plugin doesn't sound right. The original name is ok, not sure how it can be improved. Maybe addon_plugins? ~~Or plugins_from_addons?~~

The "addon_plugin" name in the method itself is a bit weird. Doesn't addon and plugin mean the same in this context? Or is there an API for non-plugin addons?

Since this stores editor plugins, it could just be editor_plugins.

The point of this rename was that "addon" is inconsistent which the usual "plugin" naming, but just changing "addons" to "plugins" wont work since there is already Vector<EditorPlugin *> editor_plugins;
In addition, "plugin_addons" sound like it is just a list of all editor plugins/addons, which is not the case: It is actually of the type Map<String, EditorPlugin *>, so it maps the name of the editor plugin to the editor plugin object. In my opinion naming a map x_to_y makes it clearer since it directly tells you what is mapped to what.

If I understand this variable correctly, it tracks which plugin was added from which user addon.

Ok, this naming is indeed a bit weird. "Addon plugins" are basically editor plugins provided by the user, right? Should we then rename the corresponding methods/variables to use a "user_plugin" name scheme instead of a mix of plugin_addons and addon_plugins?

I don't know tbh. It's rather established that installable plugins are called addons (their default folder is even called that). "addon plugins" could indeed be renamed to "user plugins", but the other usage of "addon" is probably fine.

Ok, I agree, we shouldn't do too much in this PR.
However, I still would rename plugin_addons to addon_name_to_plugin. What do you think?

addon_name_to_plugin is ok.

KoBeWi · 2022-03-30T11:33:53Z

editor/editor_node.cpp

@@ -3933,7 +3932,7 @@ void EditorNode::register_editor_types() {

 	// FIXME: Is this stuff obsolete, or should it be ported to new APIs?
 	GDREGISTER_CLASS(EditorScenePostImport);
-	//ClassDB::register_type<EditorImportExport>();
+	// ClassDB::register_type<EditorImportExport>();


Seems like this type no longer exists. The code could be removed. (there is another commented-out instance in the code base if you search it)

If we remove the dead code I guess we can also remove the FIXME comment above. I added this more than a year ago, if this didn't change since it's likely not relevant anymore.

Isn't it for EditorScenePostImport? Or was EditorScenePostImport just added between the comment and the commented out code?

Ok, removed both comments.

In PR #10656

//ClassDB::register_type<EditorImporter>(); //ClassDB::register_type<EditorPostImport>();

was replaced with:

// FIXME: Is this stuff obsolete, or should it be ported to new APIs? //ClassDB::register_class<EditorExportPlugin>(); //ClassDB::register_class<EditorScenePostImport>(); //ClassDB::register_type<EditorImportExport>();

IIRC EditorScenePostImport was ported recently. The only thing left would be EditorImportExport, but it's not clear what it's supposed to do. Also it's 5 years old...

I did a quick research with git log, but couldn't find the exact commit where EditorImportExport was removed or renamed. But in the end it should be fine to remove this commented-out line.

Good to remove.

editor/editor_node.cpp

KoBeWi · 2022-03-30T12:17:57Z

editor/editor_node.h

+	EditorCommandPalette *command_palette;
+	EditorData editor_data;
+	EditorExport *editor_export;
+	EditorFolding editor_folding;
+	EditorInterface *editor_interface;
+	EditorLog *log;
+	EditorNativeShaderSourceVisualizer *native_shader_source_visualizer;
+	EditorPlugin *editor_plugin_screen;
+	EditorPluginList *editor_plugins_force_input_forwarding;
+	EditorPluginList *editor_plugins_force_over;
+	EditorPluginList *editor_plugins_over;
+	EditorQuickOpen *quick_open;
+	EditorQuickOpen *quick_run;
+	EditorResourcePreview *resource_preview;
+	EditorRun editor_run;
+	EditorRunNative *run_native;
+	EditorSelection *editor_selection;
+	EditorSelectionHistory editor_history;
+	EditorSettingsDialog *editor_settings_dialog;
+
+	ProjectExportDialog *project_export;
+	ProjectSettingsEditor *project_settings_editor;


These could be initialized to nullptr here.

You're right, I changed it accordingly. But aren't there much more pointer member variables which are not initialized to nullptr (across the whole codebase)? So is there a general rule where to initialize to nullptr or should it ideally always be done?

Ideally, it should always be done. If it's not done somewhere, it's usually old code or someone missed it during review. We have occasional crashes caused by uninitialized pointers or variables.

We did a pass at initialization most member variables a while ago, but at the time we did not decide to initialize pointers to nullptr. So most of the codebase doesn't initialize them, but we can confirm with the other contributors that this should be done and then implement it with a regex.

If you do if (pointer), the behavior is undefined when the pointer is not initialized and results in crashes that occur randomly (I've seen two of them recently). I don't know if there is anything against initializing them, but I'd say it's better to be safe.

Co-authored-by: Eric M <[email protected]>

akien-mga · 2022-03-30T21:41:40Z

Thanks!

Geometror requested review from a team as code owners February 21, 2022 17:45

akien-mga self-requested a review February 21, 2022 17:54

akien-mga added enhancement topic:codestyle topic:editor labels Feb 21, 2022

akien-mga added this to the 4.0 milestone Feb 21, 2022

Geometror force-pushed the editor-node-data-cleanup branch 3 times, most recently from 7e47acd to 14d8e80 Compare March 29, 2022 23:59

Geometror changed the title ~~Cleanup EditorNode and EditorData~~ Cleanup EditorNode, EditorHistory, EditorSelection and EditorData Mar 30, 2022

akien-mga reviewed Mar 30, 2022

View reviewed changes

akien-mga approved these changes Mar 30, 2022

View reviewed changes

akien-mga requested a review from a team March 30, 2022 09:52

This was referenced Mar 30, 2022

Refactored EditorHistory, removed unused code, added documentation. #44222

Closed

Removed unused code from EditorSelection and added documentation. #44249

Closed

KoBeWi reviewed Mar 30, 2022

View reviewed changes

editor/editor_node.cpp Outdated Show resolved Hide resolved

Geometror force-pushed the editor-node-data-cleanup branch 2 times, most recently from dfbcada to 988e61f Compare March 30, 2022 12:13

KoBeWi reviewed Mar 30, 2022

View reviewed changes

Geometror force-pushed the editor-node-data-cleanup branch from 988e61f to 843a898 Compare March 30, 2022 17:58

Cleanup EditorNode and EditorData

314430b

Co-authored-by: Eric M <[email protected]>

Geometror force-pushed the editor-node-data-cleanup branch from 843a898 to 314430b Compare March 30, 2022 18:12

akien-mga merged commit b7850bb into godotengine:master Mar 30, 2022

Geometror mentioned this pull request Apr 27, 2022

Upcoming C++ code style changes for 4.0+ #33027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup EditorNode, EditorHistory, EditorSelection and EditorData #58395

Cleanup EditorNode, EditorHistory, EditorSelection and EditorData #58395

Geometror commented Feb 21, 2022 •

edited

Loading

EricEzaM commented Feb 22, 2022

Geometror commented Mar 30, 2022

akien-mga Mar 30, 2022

Geometror Mar 30, 2022

akien-mga left a comment

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

akien-mga Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022 •

edited

Loading

akien-mga Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

KoBeWi Mar 30, 2022 •

edited

Loading

akien-mga Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

fire Mar 30, 2022

KoBeWi Mar 30, 2022

Geometror Mar 30, 2022

KoBeWi Mar 30, 2022

akien-mga Mar 30, 2022

KoBeWi Mar 30, 2022

akien-mga commented Mar 30, 2022

		scene->set_meta("__editor_run_settings__", Variant());
		scene->set_meta("__editor_plugin_states__", Variant());

Cleanup EditorNode, EditorHistory, EditorSelection and EditorData #58395

Cleanup EditorNode, EditorHistory, EditorSelection and EditorData #58395

Conversation

Geometror commented Feb 21, 2022 • edited Loading

EricEzaM commented Feb 22, 2022

Geometror commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoBeWi Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Mar 30, 2022

Geometror commented Feb 21, 2022 •

edited

Loading

KoBeWi Mar 30, 2022 •

edited

Loading

KoBeWi Mar 30, 2022 •

edited

Loading