godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.13k stars 21.17k forks source link

Deleting metadata property in inherited scene does not persist #97596

Open WagnerGFX opened 2 weeks ago

WagnerGFX commented 2 weeks ago

Tested versions

System information

Windows 10 - Godot v4.3.0-stable

Issue description

Inherited scenes don't seem to handle the idea of deleting metadata that exists in the base scene. Instead, any changes are removed from the inherited scene's tscn file and the editor inteprets that as inheriting the data from the base scene, showing it again when reopening the inherited scene.

Considering how the UI works, I would expect the metadata property to be removed from the inherited scene. But, considering how inheritance works, I think the property should not be deleted.

In my opinion, the best option is to not show the X button for inherited properties (similar to how inherited groups can't be toggle off), just allowing the value to be changed, like any other node/script property.

There is also the option of making the X button return the value to the Variant default state, instead of the base scene default value. But, I'm not sure if a button with 2 different behaviors would be a good idea. People would start opening issues about the button not working.

The button could be changed to another visual, but it will be hard not to clash with the "revert to base value" button.

Steps to reproduce

1) Create and save a base scene with a metadata of any type. 2) Create a scene inheriting from the base scene. 3) Delete metadata in inspector from the inherited scene and save. 4) Reopen the inherited scene.

Minimal reproduction project (MRP)

Metadata Test.zip

dustdfg commented 1 week ago

I can reproduce this too. But I would like to extend the description. The same is applied to other deletable properties. Try to create TileMap (yeah it is deprecated but it just have several layers as deletable properties) and then delete one of the layers in the inherited scene.

dustdfg commented 1 week ago

I tried to solve but failed:

diff --git a/editor/editor_inspector.cpp b/editor/editor_inspector.cpp
index a215662f16..9a3a243a4a 100644
--- a/editor/editor_inspector.cpp
+++ b/editor/editor_inspector.cpp
@@ -3444,14 +3444,24 @@ void EditorInspector::update_tree() {
                                                break;
                                        }
                                }
-
                                ep->set_draw_warning(draw_warning);
                                ep->set_use_folding(use_folding);
                                ep->set_checkable(checkable);
                                ep->set_checked(checked);
                                ep->set_keying(keying);
                                ep->set_read_only(property_read_only || all_read_only);
-                               ep->set_deletable(deletable_properties || p.name.begins_with("metadata/"));
+
+                               if (p.name.begins_with("metadata/")) {
+                                       bool deletable = true;
+                                       if (ClassDB::is_parent_class(object->get_class(), "Node")) {
+                                               Node* nod = Object::cast_to<Node>(object);
+                                               auto ttt = nod->get_scene_inherited_state();
+                                               deletable =  !_is_value_potential_override(nod, p.name);
+                                       }
+                                       ep->set_deletable(deletable);
+                               } else {
+                                       ep->set_deletable(deletable_properties);
+                               }
                        }

                        current_vbox->add_child(editors[i].property_editor);

Following code almost works. It forbids deletion of metadata in inherited scene. But _is_value_potential_override isn't perfect and contains "potential" word which means it isn't made for precise check if value overrides something or not. If you edit inherited node, it won't let you to delete other metadata properties which you defined by yourself in the inherited scene

WagnerGFX commented 1 week ago

_is_value_potential_override()

I'm not sure of how things work in the C++ side of the engine, but following the function name, it probably checks only for the value and not the property. In the current situation, the code needs to check only the existence of the property, not it's value. Maybe that is why it's not working as expected, specially when fed with a property that might not exist.