Fix export variable of type Node pointing to a wrong child node when duplicating

[warriormaster12]

Took a while to figure that this was a two part issue.

1. properties were considered as overridden since get_property_value method would return a variant that was read from a packed scene file (NodePath). However, when calling instantiate the variants would be converted to a proper type (Object) if they were in the deferred_properties list.
2. When duplicating a node, variants of type Object would not update their path in that process, which is fine for nodes that aren't children of the duplicated node.

before pr:

before_duplication_fixed.mp4

pr:

duplication_fixed.mp4

Fixes: #82670
Supersedes: #83006

[warriormaster12]

Updated a comment in the node.cpp as it was a bit unclear

[warriormaster12]

Is it also possible to cherrypick for 4.1.x? If there's going to be another maintenance release.

[Mickeon]

Am I reading this correctly? Does this also supersede #81866 by solving a similar or pretty much identical issue?

[warriormaster12]

@Mickeon seems like it solves a similar problem. Though I'm not sure how yours handles arrays if it even does.

[warriormaster12]

So what is the next step? Would be nice to merge this so that I can continue working on my game project without worrying about duplicating nodes 😊

[AThousandShips]

No team member has approved this yet, it's still waiting for a review

[warriormaster12]

Hmm seems like the first element only triggers property being different. Should I try to fix it for this pr or maybe a follow up since it might not be critical issue?

[AThousandShips]

Is it that way before? If not this PR should make sure it doesn't introduce that behaviour

In other words: Does this happen without this PR, then you can but not necessary in this PR, otherwise you have to ensure it doesn't introduce new bugs

[warriormaster12]

Aight, I'll take a closer look at home. Hopefully it will be simple enough to patch this.

[AThousandShips]

I'd suggest to first update your branch to get a good view of how the code looks right now, as you're about 9 days behind

[warriormaster12]

Do you mean that I should update the branch with the master?

[AThousandShips]

Yes, rebase it

[warriormaster12]

Aight, I'll do that 👍

[MetRiko]

Does someone know what is the status for this PR?
It looks like it was approved 2 weeks ago but I didn't find it in the changelog of the latest Godot 4.2 RC1 or Beta 6 release. Does it mean it will be in the RC2 or is it not ready yet?

[KoBeWi]

It has 4.3 milestone with 4.2 cherry-pick label, so it will be available in 4.2.1.
It was approved around release freeze, all PRs that weren't merged before then and weren't fixing issues important for the release were pushed to the next version.

[YuriSizov]

Would be nice to evaluate if this would lead to performance regressions like other similar recent changes did. Looks like we're iterating over a bunch of nodes and all of their properties to solve the issue here.

[KoBeWi]

It's not recursive, so it's probably fine.

[YuriSizov]

It's not recursive

It kind of is, I think? We just collect all the nodes ahead of time. Though that's not new to this PR. So I guess it won't be any worse.

[warriormaster12]

The pr itself imo doesn't have recursion directly since it just takes the nodepath and converts into an object. If you have child with a property that needs to be converted then you get recursion. Maybe there might be a performance penalty for nodepath conersion 🤔

[YuriSizov]

Thanks!

[MikeFP]

It has 4.3 milestone with 4.2 cherry-pick label, so it will be available in 4.2.1. It was approved around release freeze, all PRs that weren't merged before then and weren't fixing issues important for the release were pushed to the next version.

I couldn't find this commit in either 4.2.1 nor 4.3-dev. Was it included?

[Mickeon]

Once the cherry-pick is done, the label is removed, so this has been forgotten.

[warriormaster12]

It was included in 4.3 dev1

You just have to go through interactive commit history.

[MikeFP]

The issue solved by this PR still happens in 4.3-dev1, so that's why I guessed the commit wasn't included. I don't know, though.

The test I did:

Root > Node1 > NodeWithProp (exported Node prop set to parent `Node1`)

I selected Node1, hit duplicate, and the result was:

Root > Node2 > NodeWithProp (exported Node prop set to `Node1` instead of `Node2`)

[warriormaster12]

Interesting, maybe I misunderstood how interactive commit page works. I’ll have to check once I get a chance.

[warriormaster12]

@MikeFP could you send a minimum project?

[kleonc]

The test I did:

Root > Node1 > NodeWithProp (exported Node prop set to parent `Node1`)

I selected Node1, hit duplicate, and the result was:

Root > Node2 > NodeWithProp (exported Node prop set to `Node1` instead of `Node2`)

@MikeFP Can confirm, indeed this case seems to be broken. 🤔

The issue solved by this PR still happens in 4.3-dev1

#82670 was specifically reporting a case when it's the Node with the exported Node property being duplicated though. And such case seems to be fixed by this PR.
What you've mentioned (exported Node property in a child of a Node being duplicated) is a different case, you could open a new issue for it.

[MikeFP]

@kleonc I see. In fact, there's already an issue here: #78060, with an open PR #81866. I thought it would get superseded by this, like a previous comment in this discussion suggested:

I wouldn't mind more eyes here, but I can give my take with some confidence:

This supersedes #81866. In that one we concluded that updating the node references should be the default behavior, which is intrinsic to the present PR. Also, this one handles every possibility of typed variables, so this one is more comprehensive.

But maybe this PR didn't actually end up covering that case. Now it's a matter of working on that other PR, then.

[warriormaster12]

I’ll take a look after new year celebration. I already have one suspect.