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

Enforce that custom nodes/resources keep their original type (assigned script) #7905

Closed
bjornmp opened this issue Sep 28, 2023 · 5 comments · Fixed by godotengine/godot#91341
Milestone

Comments

@bjornmp
Copy link

bjornmp commented Sep 28, 2023

Describe the project you are working on

Improvement to Godot's custom node and resource type handling.
#7808

Describe the problem or limitation you are having in your project

Currently Godot's custom node/resource types are no different than native nodes/resources with a script attached to them. The problem with that is that plugins exposing custom node types and resources with systems functionality (AI for example) are no different than scripted nodes by the user with game logic. Extending a plugins node to add ones own game logic to it and then clearing the extension to that node in a later stage, also clears its original script, turning it into a naked native node and losing the fundamental logic it was meant to perform.

A similar problem happens with resources. The script of a custom resource type can accidentally be cleared, effectively changing it's underlying type.

A custom node type or resource type, once created, should retain it's baseline type, just like native nodes or resources do. This is an intuitive expectation I believe most users have.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Custom node types will have a faded script icon. This script cannot be cleared.
image

An additional property "Custom Type Script" will hold it's underlying type (only visible in the inspector with DEBUG_ENABLED). This will be the fallback script in case "Script" is cleared.
image

When a custom node holds an extension to it's type, it's script icon is shown in full color.
image

"Script" will hold the extension script as normal.
image

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

An additional property "custom_type_script" has been added next to "script" inside Object. When a custom type object is instantiated, both "custom_type_script" and "script" are set. "custom_type_script" defines the baseline type of this object and will be the fallback in case "script" is cleared by the user. A check is also performed so that "script" must be a valid extension of "custom_type_script". These checks are currently in form of warnings, but can be changed in a later stage to actively prevent setting an incompatible script.

Note for C#: When running a project, there will be several warnings that "Script" was set to null, not respecting the existence of a "custom_type_script" as baseline. This happens during C# domain reload where scripts are detached from all objects and re-attached after reload.

If this enhancement will not be used often, can it be worked around with a few lines of script?

I would say, no.

Is there a reason why this should be core and not an add-on in the asset library?

I believe this needs to be part of Godot's core functionality, to make a clear distinction for the user between different node types:

  • Native nodes, that carry core engine system functionality
  • Custom nodes, that carry systems functionality (i.e. AI system provided by a plugin)
  • Scripted nodes, that carry game logic
@dalexeev
Copy link
Member

I think this can be solved at the editor UI level, without adding a new property:

Also the problem is that in godotengine/godot#91341 this is done only for nodes, but resources are also modifiable through the editor, and in general any object can have a script.

@bjornmp
Copy link
Author

bjornmp commented Apr 30, 2024

I think this can be solved at the editor UI level, without adding a new property:

During saving and loading of a scene you lose the information if a node is a custom node or a native node with a script attached. With current loading behavior you will always get a native node with a loose script attached that you can clear. The new property is there to tell that this node was instantiated with this underlying type and that it should be instantiated again with this type during loading, even if script contains another script that's an extension of the original type.

Also the problem is that in godotengine/godot#91341 this is done only for nodes

godotengine/godot#82684 did handle nodes and resources alike, because it managed custom types at the Core/Object level. But changes to Core weren't desired so I created this new PR where it's managed on a Scene/Node level, for now. I intend to add this feature to Resource again if this PR gets accepted.

@dalexeev
Copy link
Member

During saving and loading of a scene you lose the information if a node is a custom node or a native node with a script attached. With current loading behavior you will always get a native node with a loose script attached that you can clear.

In my opinion, this should be an internal property or editor metadata, not a documented property. For metadata we use the prefixes _edit_, __editor_ or __.

List
212 matches
editor/action_map_editor.cpp: 31
123: 29: 		String old_name = ti->get_meta("__name");
144: 25: 		String name = ti->get_meta("__name");
145: 35: 		Dictionary old_action = ti->get_meta("__action");
168: 31: 			current_action = item->get_meta("__action");
169: 36: 			current_action_name = item->get_meta("__name");
176: 45: 			current_action = item->get_parent()->get_meta("__action");
177: 50: 			current_action_name = item->get_parent()->get_meta("__name");
179: 43: 			current_action_event_index = item->get_meta("__index");
181: 35: 			Ref<InputEvent> ie = item->get_meta("__event");
188: 28: 			String name = item->get_meta("__name");
193: 48: 			Dictionary action = item->get_parent()->get_meta("__action").duplicate();
194: 49: 			String action_name = item->get_parent()->get_meta("__name");
196: 32: 			int event_index = item->get_meta("__index");
205: 33: 			ERR_FAIL_COND_MSG(!item->has_meta("__action_initial"), "Tree Item for action which can be reverted is expected to have meta value with initial value of action.");
207: 34: 			Dictionary action = item->get_meta("__action_initial").duplicate();
208: 35: 			String action_name = item->get_meta("__name");
220: 26: 	if (!item || !item->has_meta("__event")) {
262: 20: 	if (selected->has_meta("__action")) {
266: 20: 	if (selected->has_meta("__event")) {
288: 47: 	if (d["input_type"] == "action" && item->has_meta("__event")) {
316: 36: 		String relative_to = target->get_meta("__name");
317: 38: 		String action_name = selected->get_meta("__name");
322: 37: 		int current_index = selected->get_meta("__index");
323: 34: 		int target_index = target->get_meta("__index");
326: 55: 		Dictionary new_action = selected->get_parent()->get_meta("__action");
352: 67: 		emit_signal(SNAME("action_edited"), selected->get_parent()->get_meta("__name"), new_action);
432: 20: 		action_item->set_meta("__action", action_info.action);
433: 20: 		action_item->set_meta("__name", action_info.name);
451: 21: 			action_item->set_meta("__action_initial", action_info.action_initial);
470: 20: 			event_item->set_meta("__event", event);
471: 20: 			event_item->set_meta("__index", evnt_idx);
editor/connections_dialog.cpp: 3
1215: 32: 		if (!item->get_child(i)->has_meta("_inherited_connection")) {
1251: 43: 	bool connection_is_inherited = item->has_meta("_inherited_connection");
1547: 27: 					connection_item->set_meta("_inherited_connection", true);
editor/create_dialog.cpp: 2
411: 21: 	if (!selected->get_meta("__instantiable", true)) {
509: 21: 	if (to_select->get_meta("__instantiable", true)) {
editor/debugger/script_editor_debugger.cpp: 6
558: 15: 			error->set_meta("_is_warning", true);
560: 15: 			error->set_meta("_is_error", true);
877: 21: 					if (error->has_meta("_is_warning")) {
881: 28: 					} else if (error->has_meta("_is_error")) {
1650: 16: 			if (ti->has_meta("_is_warning")) {
1652: 23: 			} else if (ti->has_meta("_is_error")) {
editor/filesystem_dock.cpp: 8
3777:  7: 		set_meta("_dock_display_mode", get_display_mode());
3778:  7: 		set_meta("_dock_file_display_mode", get_file_list_display_mode());
3780: 86: 		FileSystemDock::DisplayMode new_display_mode = FileSystemDock::DisplayMode(int(get_meta("_bottom_display_mode", int(FileSystemDock::DISPLAY_MODE_HSPLIT))));
3781:107: SystemDock::FileListDisplayMode new_file_display_mode = FileSystemDock::FileListDisplayMode(int(get_meta("_bottom_file_display_mode", int(FileSystemDock::FILE_LIST_DISPLAY_THUMBNAILS))));
3787:  7: 		set_meta("_bottom_display_mode", get_display_mode());
3788:  7: 		set_meta("_bottom_file_display_mode", get_file_list_display_mode());
3793: 58: 		new_display_mode = FileSystemDock::DisplayMode(int(get_meta("_dock_display_mode", new_display_mode)));
3794: 71: 		new_file_display_mode = FileSystemDock::FileListDisplayMode(int(get_meta("_dock_file_display_mode", new_file_display_mode)));
editor/group_settings_editor.cpp: 8
 70: 36: 		String old_description = ti->get_meta("__description");
121: 45: 	String old_name = rename_group_dialog->get_meta("__name");
305: 13: 		item->set_meta("__name", E);
306: 13: 		item->set_meta("__description", groups_cache[E]);
331: 28: 	String old_name = ti->get_meta("__name");
344: 31: 	String description = ti->get_meta("__description");
472: 24: 	String name = ti->get_meta("__name");
475: 27: 	rename_group_dialog->set_meta("__name", name);
editor/groups_editor.cpp: 21
224: 13: 		item->set_meta("__local", true);
225: 13: 		item->set_meta("__name", E);
226: 13: 		item->set_meta("__description", "");
255: 13: 		item->set_meta("__local", false);
256: 13: 		item->set_meta("__name", E);
257: 13: 		item->set_meta("__description", global_groups[E]);
387: 26: 	bool is_local = ti->get_meta("__local");
388: 30: 	String group_name = ti->get_meta("__name");
402: 33: 			String description = ti->get_meta("__description");
456: 15: 		if (ti->get_meta("__local")) {
462: 31: 		String group_name = ti->get_meta("__name");
520: 28: 	String old_name = ti->get_meta("__name");
537: 32: 		String description = ti->get_meta("__description");
571: 24: 	String name = ti->get_meta("__name");
572: 26: 	bool is_local = ti->get_meta("__local");
582: 32: 		String description = ti->get_meta("__description");
711: 28: 	bool is_global = !ti->get_meta("__local");
715: 24: 	String name = ti->get_meta("__name");
718: 27: 	rename_group_dialog->set_meta("__name", name);
751: 28: 	bool is_global = !ti->get_meta("__local");
767: 45: 	String old_name = rename_group_dialog->get_meta("__name");
editor/gui/scene_tree_editor.cpp: 2
411: 19: 		if (p_node->has_meta("_edit_lock_")) {
414: 19: 		if (p_node->has_meta("_edit_group_")) {
editor/input_event_configuration_dialog.cpp: 23
159: 53: 					int input_type = input_item->get_parent()->get_meta("__type", 0);
169: 87: 							bool key_match = k.is_valid() && (Variant(k->get_keycode()) == input_item->get_meta("__keycode") || Variant(k->get_physical_keycode()) == input_item->get_meta("__keycode"));
169:162: 							bool key_match = k.is_valid() && (Variant(k->get_keycode()) == input_item->get_meta("__keycode") || Variant(k->get_physical_keycode()) == input_item->get_meta("__keycode"));
170: 98: 							bool joyb_match = joyb.is_valid() && Variant(joyb->get_button_index()) == input_item->get_meta("__index");
171: 90: 							bool joym_match = joym.is_valid() && Variant(joym->get_axis()) == input_item->get_meta("__axis") && joym->get_axis_value() == (float)input_item->get_meta("__value");
171:157: 							bool joym_match = joym.is_valid() && Variant(joym->get_axis()) == input_item->get_meta("__axis") && joym->get_axis_value() == (float)input_item->get_meta("__value");
172: 92: 							bool mb_match = mb.is_valid() && Variant(mb->get_button_index()) == input_item->get_meta("__index");
283: 16: 		kb_root->set_meta("__type", INPUT_KEY);
294: 14: 			item->set_meta("__keycode", keycode_get_value_by_index(i));
303: 19: 		mouse_root->set_meta("__type", INPUT_MOUSE_BUTTON);
318: 14: 			item->set_meta("__index", mouse_buttons[i]);
327: 18: 		joyb_root->set_meta("__type", INPUT_JOY_BUTTON);
341: 14: 			item->set_meta("__index", i);
350: 18: 		joya_root->set_meta("__type", INPUT_JOY_MOTION);
367: 14: 			item->set_meta("__axis", i >> 1);
368: 14: 			item->set_meta("__value", (i & 1) ? 1 : -1);
458: 20: 	if (selected->has_meta("__type")) {
462: 69: 	InputType input_type = (InputType)(int)selected->get_parent()->get_meta("__type");
466: 42: 			Key keycode = (Key)(int)selected->get_meta("__keycode");
503: 54: 			MouseButton idx = (MouseButton)(int)selected->get_meta("__index");
523: 50: 			JoyButton idx = (JoyButton)(int)selected->get_meta("__index");
532: 47: 			JoyAxis axis = (JoyAxis)(int)selected->get_meta("__axis");
533: 30: 			int value = selected->get_meta("__value");
editor/plugins/canvas_item_editor_plugin.cpp: 25
253: 21: 	return p_node->get_meta("_edit_lock_", false);
439: 31: 			Array vguides = scene->get_meta("_edit_vertical_guides_", Array());
444: 31: 			Array hguides = scene->get_meta("_edit_horizontal_guides_", Array());
667: 29: 				if (ci_tmp && node->has_meta("_edit_group_")) {
709: 35: 	bool lock_children = p_node->get_meta("_edit_group_", false);
1066: 73: 			Array vguides = EditorNode::get_singleton()->get_edited_scene()->get_meta("_edit_vertical_guides_", Array());
1067: 73: 			Array hguides = EditorNode::get_singleton()->get_edited_scene()->get_meta("_edit_horizontal_guides_", Array());
1160: 74: 				Array vguides = EditorNode::get_singleton()->get_edited_scene()->get_meta("_edit_vertical_guides_", Array());
1161: 74: 				Array hguides = EditorNode::get_singleton()->get_edited_scene()->get_meta("_edit_horizontal_guides_", Array());
2296: 32: 							if (ci_tmp && node->has_meta("_edit_group_")) {
2655: 34: 				if (all_locked && !item->has_meta("_edit_lock_")) {
2658: 33: 				if (all_group && !item->has_meta("_edit_group_")) {
2800: 30: 		Array vguides = scene->get_meta("_edit_vertical_guides_", Array());
2809: 30: 		Array hguides = scene->get_meta("_edit_horizontal_guides_", Array());
3394: 30: 		bool item_locked = ci->has_meta("_edit_lock_");
3817: 39: 		if (show_lock_gizmos && p_node->has_meta("_edit_lock_")) {
3823: 36: 		if (show_group_gizmos && ci->has_meta("_edit_group_")) {
4239: 17: 			if (n2d->has_meta("_edit_bone_") && n2d->get_parent_item()) {
4248: 17: 					if (n->has_meta("_edit_ik_")) {
4621: 27: 			if (root && (root->has_meta("_edit_horizontal_guides_") || root->has_meta("_edit_vertical_guides_"))) {
4621: 73: 			if (root && (root->has_meta("_edit_horizontal_guides_") || root->has_meta("_edit_vertical_guides_"))) {
4623: 19: 				if (root->has_meta("_edit_horizontal_guides_")) {
4624: 32: 					Array hguides = root->get_meta("_edit_horizontal_guides_");
4629: 19: 				if (root->has_meta("_edit_vertical_guides_")) {
4630: 32: 					Array vguides = root->get_meta("_edit_vertical_guides_");
editor/plugins/control_editor_plugin.cpp: 8
276: 41: 			int flag_value = flag_checks[i]->get_meta("_value");
295: 40: 		int flag_value = flag_checks[i]->get_meta("_value");
353: 11: 		cb->set_meta("_value", current_val);
760: 42: 			const bool use_anchors = control->get_meta("_edit_use_anchors_", false);
784: 11: 			E->set_meta("_edit_use_anchors_", true);
786: 14: 			E->remove_meta("_edit_use_anchors_");
835: 21: 	return p_node->get_meta("_edit_lock_", false);
930: 21: 			if (control->get_meta("_edit_use_anchors_", false)) {
editor/plugins/curve_editor_plugin.cpp: 2
994: 41: 				snap_button->set_pressed(curve->get_meta("_snap_enabled", false));
995: 43: 				snap_count_edit->set_value(curve->get_meta("_snap_count", DEFAULT_SNAP));
editor/plugins/gradient_editor_plugin.cpp: 2
614: 44: 				snap_button->set_pressed(gradient->get_meta("_snap_enabled", false));
615: 46: 				snap_count_edit->set_value(gradient->get_meta("_snap_count", DEFAULT_SNAP));
editor/plugins/gradient_texture_2d_editor_plugin.cpp: 2
271: 43: 				snap_button->set_pressed(texture->get_meta("_snap_enabled", false));
272: 45: 				snap_count_edit->set_value(texture->get_meta("_snap_count", DEFAULT_SNAP));
editor/plugins/mesh_library_editor_plugin.cpp: 4
 52:125: 		menu->get_popup()->set_item_disabled(menu->get_popup()->get_item_index(MENU_OPTION_UPDATE_FROM_SCENE), !mesh_library->has_meta("_editor_source_scene"));
 69: 38: 	String existing = mesh_library->get_meta("_editor_source_scene");
119: 20: 	mesh_library->set_meta("_editor_source_scene", p_str);
239: 98: 			cd_update->set_text(vformat(TTR("Update from existing scene?:\n%s"), String(mesh_library->get_meta("_editor_source_scene"))));
editor/plugins/node_3d_editor_plugin.cpp: 8
744: 34: 			if (selected_tmp && node->has_meta("_edit_group_")) {
1055: 35: 				if (selected_tmp && item->has_meta("_edit_group_")) {
1528: 21: 	return p_node->get_meta("_edit_lock_", false);
1574: 36: 					if (selected_tmp && node->has_meta("_edit_group_")) {
4677: 15: 		if (sp->has_meta("_edit_lock_")) {
5758: 16: 			if (sp->has_meta("_edit_lock_")) {
7396: 34: 				if (all_locked && !node->has_meta("_edit_lock_")) {
7399: 35: 				if (all_grouped && !node->has_meta("_edit_group_")) {
editor/plugins/script_editor_plugin.cpp: 7
700:  9: 	c->set_meta("__editor_pass", ++edit_pass);
2022: 22: 			int pass = n->get_meta("__editor_pass", -1);
2066: 13: 				se->set_meta("_edit_res_path", path);
2462: 10: 	se->set_meta("_edit_res_path", p_resource->get_path());
2875: 15: 		if (se->get_meta("_edit_res_path") == p_removed_file) {
3314: 42: 		tab_container->get_tab_control(i)->set_meta("__editor_pass", Variant());
3566:  9: 	n->set_meta("__editor_pass", ++edit_pass);
editor/plugins/theme_editor_plugin.cpp: 5
 98: 18: 		type_node->set_meta("_can_be_imported", false);
141: 24: 			data_type_node->set_meta("_can_be_imported", false);
209: 20: 				item_node->set_meta("_can_be_imported", true);
344: 24: 	if (!p_tree_item->get_meta("_can_be_imported")) {
378: 24: 	if (!p_tree_item->get_meta("_can_be_imported")) {
modules/gridmap/editor/grid_map_editor_plugin.cpp: 2
1013: 35: 	Vector3 edited_floor = node->get_meta("_editor_floor_", Vector3());
1173: 12: 	node->set_meta("_editor_floor_", Vector3(edit_floor[0], edit_floor[1], edit_floor[2]));
modules/noise/editor/noise_editor_plugin.cpp: 3
 80: 20: 			if (_noise->has_meta("_preview_in_3d_space_")) {
 91: 16: 			_noise->set_meta("_preview_in_3d_space_", true);
 93: 19: 			_noise->remove_meta("_preview_in_3d_space_");
scene/2d/skeleton_2d.cpp: 4
237: 13: 				if (has_meta("_local_pose_override_enabled_")) {
277: 13: 				if (has_meta("_local_pose_override_enabled_")) {
743: 24: 				bones[i].bone->set_meta("_local_pose_override_enabled_", true);
755: 27: 				bones[i].bone->remove_meta("_local_pose_override_enabled_");
scene/gui/dialogs.cpp: 4
290: 63: 	Control *right_spacer = Object::cast_to<Control>(button->get_meta("__right_spacer"));
309: 14: 	button->set_meta("__right_spacer", right_spacer);
343: 65: 	Control *right_spacer = Object::cast_to<Control>(p_button->get_meta("__right_spacer"));
358: 20: 		p_button->remove_meta("__right_spacer");
scene/gui/menu_bar.cpp: 7
506: 23: 		if (!popups[i]->has_meta("_menu_name") && String(popups[i]->get_name()) != get_menu_title(i)) {
582: 36: 	String menu_name = String(pm->get_meta("_menu_name", pm->get_name()));
640: 18: 	p_child->remove_meta("_menu_name");
641: 18: 	p_child->remove_meta("_menu_tooltip");
826: 14: 		pm->remove_meta("_menu_name");
828: 11: 		pm->set_meta("_menu_name", p_title);
851: 10: 	pm->set_meta("_menu_tooltip", p_tooltip);
scene/gui/tab_container.cpp: 8
505: 20: 		controls[i]->set_meta("_tab_index", i);
512: 25: 		if (!controls[i]->has_meta("_tab_name") && String(controls[i]->get_name()) != get_tab_title(i)) {
532:  9: 	c->set_meta("_tab_index", tab_bar->get_tab_count() - 1);
557: 28: 		tab_bar->move_tab(c->get_meta("_tab_index"), get_tab_idx_from_control(c));
590: 18: 	p_child->remove_meta("_tab_index");
591: 18: 	p_child->remove_meta("_tab_name");
760: 17: 		child->remove_meta("_tab_name");
762: 14: 		child->set_meta("_tab_name", p_title);
scene/gui/tree.cpp: 8
2194:108: 			if ((select_mode == SELECT_ROW && selected_item == p_item) || p_item->cells[i].selected || !p_item->has_meta("__focus_rect")) {
2198: 18: 					p_item->set_meta("__focus_rect", Rect2(r.position, r.size));
2210: 18: 					p_item->set_meta("__focus_col_" + itos(i), Rect2(r.position, r.size));
3784: 35: 							rect = get_selected()->get_meta("__focus_col_" + itos(selected_col));
3786: 35: 							rect = get_selected()->get_meta("__focus_rect");
3999: 17: 		rect = s->get_meta("__focus_col_" + itos(selected_col));
4001: 17: 		rect = s->get_meta("__focus_rect");
4378: 41: 				Rect2 rect = popup_edited_item->get_meta("__focus_rect");
scene/main/node.cpp: 4
2543: 21: 	Array pinned = get_meta("_edit_pinned_properties_", Array());
2556: 10: 		remove_meta("_edit_pinned_properties_");
2558:  7: 		set_meta("_edit_pinned_properties_", pinned);
2563: 21: 	Array pinned = get_meta("_edit_pinned_properties_", Array());
scene/resources/packed_scene.cpp: 4
 59: 29: 	Array pinned = p_node->get_meta("_edit_pinned_properties_", Array());
 74: 18: 		p_node->remove_meta("_edit_pinned_properties_");
238: 49: 						missing_node->set_original_scene(res->get_meta("__load_path__", ""));
495: 18: 				node->remove_meta("_edit_pinned_properties_");
scene/resources/resource_format_text.cpp: 1
185: 15: 			r_res->set_meta("__load_path__", ext_resources[id].path);

@bjornmp
Copy link
Author

bjornmp commented Apr 30, 2024

For metadata we use the prefixes _edit_, __editor_ or __.

I did a quick test to see how this gets stored in a packed scene:

metadata/__custom_type_script = ExtResource("2_plyvd")

So, I think this might work.

The question now is just, what makes more sense for a script resource? An internal property or a metadata?

@bjornmp
Copy link
Author

bjornmp commented May 1, 2024

So, I changed everything to use metadata instead. Works great!

It also works with resources now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants