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.

Sign up for GitHub

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

NodePath properly updated in the editor in more cases when nodes are moved or renamed #49812

Merged
merged 1 commit into from
Jun 29, 2021
Merged

NodePath properly updated in the editor in more cases when nodes are moved or renamed #49812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/classes/NodePath.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
@"/root/Main" # If your main scene's root node were named "Main".
@"/root/MyAutoload" # If you have an autoloaded node or scene.
[/codeblock]
[b]Note:[/b] In the editor, [NodePath] properties are automatically updated when moving, renaming or deleting a node in the scene tree, but they are never updated at runtime.
</description>
<tutorials>
<link title="2D Role Playing Game Demo">https://godotengine.org/asset-library/asset/520</link>
Expand Down
163 changes: 109 additions & 54 deletions editor/scene_tree_dock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1409,9 +1409,102 @@ void SceneTreeDock::fill_path_renames(Node *p_node, Node *p_new_parent, List<Pai
_fill_path_renames(base_path, new_base_path, p_node, p_renames);
}

bool SceneTreeDock::_update_node_path(const NodePath &p_root_path, NodePath &p_node_path, List<Pair<NodePath, NodePath>> *p_renames) {
NodePath root_path_new = p_root_path;
for (List<Pair<NodePath, NodePath>>::Element *F = p_renames->front(); F; F = F->next()) {
if (p_root_path == F->get().first) {
root_path_new = F->get().second;
break;
}
}

// Goes through all paths to check if it's matching.
for (List<Pair<NodePath, NodePath>>::Element *F = p_renames->front(); F; F = F->next()) {
NodePath rel_path_old = p_root_path.rel_path_to(F->get().first);

// If old path detected, then it needs to be replaced with the new one.
if (p_node_path == rel_path_old) {
NodePath rel_path_new = F->get().second;

// If not empty, get new relative path.
if (!rel_path_new.is_empty()) {
rel_path_new = root_path_new.rel_path_to(rel_path_new);
}

p_node_path = rel_path_new;
return true;
}

// Update the node itself if it has a valid node path and has not been deleted.
if (p_root_path == F->get().first && p_node_path != NodePath() && F->get().second != NodePath()) {
NodePath abs_path = NodePath(String(root_path_new).plus_file(p_node_path)).simplified();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed an extra fix here that fixes errors when moving a Skeleton node with BoneAttachment nodes as its children.

Building the new path now uses root_path_new instead of p_root_path, otherwise in cases where the root path and the child path change, this code would generate a buggy node path relative to the old root.

NodePath rel_path_new = F->get().second.rel_path_to(abs_path);

p_node_path = rel_path_new;
return true;
}
}

return false;
}

bool SceneTreeDock::_check_node_path_recursive(const NodePath &p_root_path, Variant &p_variant, List<Pair<NodePath, NodePath>> *p_renames) {
switch (p_variant.get_type()) {
case Variant::NODE_PATH: {
NodePath node_path = p_variant;
if (_update_node_path(p_root_path, node_path, p_renames)) {
p_variant = node_path;
return true;
}
} break;

case Variant::ARRAY: {
Array a = p_variant;
bool updated = false;
for (int i = 0; i < a.size(); i++) {
Variant value = a[i];
if (_check_node_path_recursive(p_root_path, value, p_renames)) {
if (!updated) {
a = a.duplicate(); // Need to duplicate for undo-redo to work.
updated = true;
}
a[i] = value;
}
}
if (updated) {
p_variant = a;
return true;
}
} break;

case Variant::DICTIONARY: {
Dictionary d = p_variant;
bool updated = false;
for (int i = 0; i < d.size(); i++) {
Variant value = d.get_value_at_index(i);
if (_check_node_path_recursive(p_root_path, value, p_renames)) {
if (!updated) {
d = d.duplicate(); // Need to duplicate for undo-redo to work.
updated = true;
}
d[d.get_key_at_index(i)] = value;
}
}
if (updated) {
p_variant = d;
return true;
}
} break;

default: {
}
}

return false;
}

void SceneTreeDock::perform_node_renames(Node *p_base, List<Pair<NodePath, NodePath>> *p_renames, Map<Ref<Animation>, Set<int>> *r_rem_anims) {
Map<Ref<Animation>, Set<int>> rem_anims;

if (!r_rem_anims) {
r_rem_anims = &rem_anims;
}
Expand All @@ -1424,60 +1517,22 @@ void SceneTreeDock::perform_node_renames(Node *p_base, List<Pair<NodePath, NodeP
return;
}

// Renaming node paths used in script instances
if (p_base->get_script_instance()) {
ScriptInstance *si = p_base->get_script_instance();

if (si) {
List<PropertyInfo> properties;
si->get_property_list(&properties);
NodePath root_path = p_base->get_path();

for (List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
String propertyname = E->get().name;
Variant p = p_base->get(propertyname);
if (p.get_type() == Variant::NODE_PATH) {
NodePath root_path_new = root_path;
for (List<Pair<NodePath, NodePath>>::Element *F = p_renames->front(); F; F = F->next()) {
if (root_path == F->get().first) {
root_path_new = F->get().second;
break;
}
}

// Goes through all paths to check if its matching
for (List<Pair<NodePath, NodePath>>::Element *F = p_renames->front(); F; F = F->next()) {
NodePath rel_path_old = root_path.rel_path_to(F->get().first);
// Renaming node paths used in node properties.
List<PropertyInfo> properties;
p_base->get_property_list(&properties);
NodePath base_root_path = p_base->get_path();

// if old path detected, then it needs to be replaced with the new one
if (p == rel_path_old) {
NodePath rel_path_new = F->get().second;

// if not empty, get new relative path
if (!rel_path_new.is_empty()) {
rel_path_new = root_path_new.rel_path_to(F->get().second);
}

editor_data->get_undo_redo().add_do_property(p_base, propertyname, rel_path_new);
editor_data->get_undo_redo().add_undo_property(p_base, propertyname, rel_path_old);

p_base->set(propertyname, rel_path_new);
break;
}

// update the node itself if it has a valid node path and has not been deleted
if (root_path == F->get().first && p != NodePath() && F->get().second != NodePath()) {
NodePath abs_path = NodePath(String(root_path).plus_file(p)).simplified();
NodePath rel_path_new = F->get().second.rel_path_to(abs_path);

editor_data->get_undo_redo().add_do_property(p_base, propertyname, rel_path_new);
editor_data->get_undo_redo().add_undo_property(p_base, propertyname, p);

p_base->set(propertyname, rel_path_new);
}
}
}
}
for (List<PropertyInfo>::Element *E = properties.front(); E; E = E->next()) {
if (!(E->get().usage & (PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_EDITOR))) {
continue;
}
String propertyname = E->get().name;
Variant old_variant = p_base->get(propertyname);
Variant updated_variant = old_variant;
if (_check_node_path_recursive(base_root_path, updated_variant, p_renames)) {
editor_data->get_undo_redo().add_do_property(p_base, propertyname, updated_variant);
editor_data->get_undo_redo().add_undo_property(p_base, propertyname, old_variant);
p_base->set(propertyname, updated_variant);
}
}

Expand Down
3 changes: 3 additions & 0 deletions editor/scene_tree_dock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,9 @@ class SceneTreeDock : public VBoxContainer {
static SceneTreeDock *singleton;
static void _update_configuration_warning();

static bool _update_node_path(const NodePath &p_root_path, NodePath &p_node_path, List<Pair<NodePath, NodePath>> *p_renames);
static bool _check_node_path_recursive(const NodePath &p_root_path, Variant &p_variant, List<Pair<NodePath, NodePath>> *p_renames);
Comment on lines +250 to +251
Copy link
Member

Choose a reason for hiding this comment

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

I was just wondering, do those actually need to be static?

Also, since p_node_path and p_variant are used to return new values, they should probably be named with a r_ prefix instead (our convention for a mutable reference that will get changed by the method).

Copy link
Contributor Author

@pouleyKetchoupp pouleyKetchoupp Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

They don't have to be static but they can. I generally set methods as static when possible to make it clear they don't use anything from the class. Do we have a different rule about when to use static?

Makes sense for the missing r_, prefix, I can make a fixup PR to change it.

Copy link
Member

Choose a reason for hiding this comment

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

They don't have to be static but they can. I generally set methods as static when possible to make it clear they don't use anything from the class. Do we have a different rule about when to use static?

I don't think we have a specific rule, I'm just asking out of curiosity (and lack of expertise on the actual impact of making things static in C++). If it doesn't hurt and makes the code clearer, that's fine with me :)


protected:
void _notification(int p_what);
static void _bind_methods();
Expand Down