Duplicating a node with an export variable doesn't update variable's nodepath

[warriormaster12]

Godot version

v4.2.dev.custom_build [0ca8542]

System information

Pop Os 22.04 lts 12th Gen Intel® Core™ i7-12700H × 20 NVIDIA RTX A2000 8GB 32.0GB  ram

Issue description

A node that has a script attached to it and it has a couple of child nodes (type of a child node doesn't matter). In the script you have an export variable of type Node. You save the scene into a file and you add it in level scene. When you start to duplicate that scene in the level and you'll hover over exported variable to show a node path, it will show an incorrect path.

2023-10-02.12-27-37.mp4

Steps to reproduce

1. create a scene with a node and a child node.
2. attach a script to the parent node and create an export variable of type Node
3. assign child node to that exported variable through the editor
4. save the scene and create a level scene where you are going to add just saved scene.
5. Possibly requires a reboot of the engine before seeing the bug
6. start duplicating nodes in the level scene

Minimal reproduction project

NodeTest.zip

[warriormaster12]

Scene.tscn might give a clue since it has saved NodePaths of objects that were duplicated in the video.

[Calinou]

• Related to Duplicating a node should result in either a true Instance or true unique copy, not a mix of both godot-proposals#317.

What you're referring to here is definitely a bug though (and something I've reproduced in 4.2.dev5).

[lostminds]

As I mentioned in #82711 I think part of the issue is that when instancing a scene ("SceneInstance") with NodePath exports, these exports are marked as overridden from the start, even if they are not changed. At least when hovering over it looks like the path is set correctly. And if you reset the overrides the path doesn't change. However, if the NodePaths have these override flags they are treated as referencing a specific node when duplicating SceneInstance and the path is changed to keep referencing this specific node. If you remove the override by clicking the reset button the NodePath export is not changed this way when duplicating SceneInstance and it references the corresponding node in the new copy of SceneInstance.

I think this duplication behavior may be correct and can be seen as intentional. The bug is instead I think that the NodePath exports are flagged as overridden when the first SceneInstance is instanced from file. Not that overridden NodePaths are changed when SceneInstance is duplicated.

[warriormaster12]

@Calinou @lostminds Found the line that caused the issue. Changed the else statement to duplicate the value as well and the issue went away. Is it okay to duplicate everything or should I only duplicate the value when the type is node path? I also need to test if an exported variable is already overriden then will this still cause the bug.

https://github.com/godotengine/godot/blob/master/scene/main/node.cpp#L2541

[warriormaster12]

Quick status update. With the one line change, you'll still have to reset the override but after that duplicating works fine.

[warriormaster12]

Looking at the source code a bit further I noticed something interesting in editor_inspector.cpp, for some reason when we are comparing if the property is revert-able I found this. When user assigns a Node to a variable in the inspector, it is considered as an object instead of a nodepath

[warriormaster12]

Another update, the underlying issue seems to be that when we are reading from a packedscene we don't differentiate between an export variable that is a nodepath and an export variable that is of type Node, thus getting this issue where types don't match and being able to revert variables in the editor inspector.

[warriormaster12]

Decided to find the first version when this bug happened... 4.1-dev4. How nobody has noticed this for so long😂

[warriormaster12]

#76389
This seems to be the pr that introduced this. Let's see if I can find the culprit line.

[RoboClonk]

Noticed this issue in 4.1.3 and it results in weird and hard to find bugs as is expected ^^'

[warriormaster12]

@RoboClonk don't worry, the fix is coming #83343 :)

[RoboClonk]

@RoboClonk don't worry, the fix is coming #83343 :)

Cool! Thanks for that :)

[fmoo]

Was this only fixed for scene-owned nodes or for packed scene instances as well? I am still able to reproduce the issue with packed scene instances in 4.3dev5 (it actually seems to be broken worse - with some references being nulled out instead of copied; in C# at least, I haven't tried GDScript yet)

[warriormaster12]

Should be for both and this should be language independent fix. Are the nulled references by any chance inherited from Resource?

[fmoo]

No resources. The nulled reference are to Nodes in a nested packed scene. Hopefully this screenshot helps clarify.

(Happy to zip up the whole project and put it somewhere or report as a new issue if that's useful.)

[fmoo]

And here's an example of a duplicated packed scene's inner node's export pointing back to the original node instead of referring to the duplicated node parent (its internal path is "..")

[warriormaster12]

Weird, I can’t replicate this on my end (tested against master branch and gdscript).

Open up a new issue and just in case try this with both C# and gdscript. I’m also curious if this also happens with master branch of godot on your end.

[fmoo]

I was able to verify the same behavior with gdscript in a much simpler project as well.

I've grabbed a video of the issue here (on 4.3dev5):
https://github.com/godotengine/godot/assets/143504/eb7d2712-ddb5-440c-a1fa-78edfe4e8a8d
Using this sample project:
test-duplicate.zip

I'd like to test master, but am having trouble generating the glue at the moment so I'll have to poke at that some more tomorrow. That said, GDScript seems to be working fine there, so maybe I'll just have to wait until the next dev6/alpha build for the fix.

[warriormaster12]

@fmoo Aight, I decided to test 4.3 dev 5, it's indeed broken. I incorrectly said that I tested against master branch, in reality I tested against my pr (that is up to date with master) where I might have fixed it. #91329. Can you try and confirm it for yourself?

Edit: Wait so it works fine with GDScript in master build?

[fmoo]

@warriormaster12 yes, gdscript and c# exhibit the bad behavior on 4.3dev5 (with and without mono), but when I checked the github artifacts from yesterday morning, as well as a local build of master, duplicating the GDScript nodes were behaving as expected.

[warriormaster12]

Perfect, thanks 👍.

Sorry for my rambling btw, bad habbit.

[fmoo]

Just for fun I did a bisect and confirmed that 9851c1b did fix this. The missing piece for me was that this wasn't merged in until Mar 24, and dev5 was built on Mar 15. Sorry for the scare!