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

[pouleyKetchoupp]

Fix more cases of node path needing an update when nodes are renamed or moved in the editor.

Built-in node properties:
Before, node paths were checked only for script export variables. Now all properties are checked from the node, which includes built-in node properties.
Allows proper node path updates for nodes like remote transform, physics joints, etc.

Arrays and dictionaries:
Node paths nested in array and dictionary properties are now also updated in the editor.

Also update the documentation to be clear about node path update in the editor and at runtime.

Fixes #49810
Fixes #49811

Credits to @latorril for finding the issue and investigating the cause.

[pouleyKetchoupp]

Pushed a fix for errors in AnimationPlayer due to gathering all properties for checking node paths. Now only properties marked as editor or storage are checked.

[pouleyKetchoupp]

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.

[akien-mga]

Thanks!

[akien-mga]

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).

[pouleyKetchoupp]

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.

[akien-mga]

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 :)

[akien-mga]

Cherry-picked for 3.4.

[Flavelius]

Does this fix InstancePlaceHolders too by any chance (#44720 (comment))?

[pouleyKetchoupp]

@Flavelius I don't know much about instance placeholders, but it looks like it's a different issue. What this PR fixes is NodePath that are used to point to a node in the scene, when the node is moved or renamed, while the problem in #44720 seems to be related to pointing to another scene as a file path when this path changes. Fixing #44720 would require specific editor code to update instance placeholders when files are moved in the file system.

[koodikulma-fi]

This feature is great, but there is one problematic use case, which could however be solved with something like: PROPERTY_USAGE_NO_NODE_PATH_CORRECTION

Background

In my case the problem occurs due to a custom "compiling" system, where the source scene contains extra nodes and setup, which is then "compiled" into a game-ready scene including various optimizations. One of the optimizations is storing all node paths within the compiled scene as node paths from the root node of baking, so there's no need to get them many times and again when reused. (On the uncompiled source side, things function like Godot expects: each path starts from the node it's stored in.)

Problem

Due to the new auto correction feature (which I really like otherwise), I'm getting errors / warnings in the editor whenever the tree is changed (about paths not being found) - and even worse, Godot might in certain cases do the auto-correction which messes up the functioning of the compiled scene.

Solutions

• However, if there was a corresponding flag in the usage of property info dictionaries (using _get_property_list()), it would be trivial to fix the problem in the script side.
• There is a work-around by not using NodePaths, but instead using Strings. However, this could cause problems in cases where TYPE_STRING vs. TYPE_NODE_PATH is used to detect other stuff. (--> This was problem in my case, but ended up solving it by prefixing the other path like strings with ":" since a valid node path will never start so.)