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.

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

Add DUPLICATE_NODE_REFERENCES flag to duplicate Nodes and change references to the newly duplicate Nodes #81866

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 18, 2023

Closes #78060.

This PR adds a new flag to be used in Node.duplicate() called DUPLICATE_NODE_REFERENCES .
When added to the method, the newly duplicated Node and its children will "correct themselves". All exported Node properties will point to their respective nodes in the hierarchy, instead of the nodes of the original instance.

Alternatively. quoting the newly added description for it:

Duplicate the node's references, changing all exported Node properties to be part of the newly duplicated hierarchy, if possible. Does not work without DUPLICATE_USE_INSTANTIATION.

I apologise for the sheer amount of amends this PR has suffered from.


Stuff is still up the air. Perhaps this shouldn't even be a new flag, and just the default behavior of DUPLICATE_USE_INSTANTIATION? It can remain to be discussed.

Sidenote, the DUPLICATE_USE_INSTANTIATION description is completely screwed up and entirely incorrect. See #68560.

@Mickeon Mickeon requested review from a team as code owners September 18, 2023 13:48
@Mickeon Mickeon force-pushed the duplicate-keep-node-references-flag branch from 6907fd0 to 247acfd Compare September 18, 2023 13:51
@Mickeon Mickeon changed the title Add DUPLICATE_NODE_REFERENCES flag to duplicate Nodes and transfer references to the new Node Add DUPLICATE_NODE_REFERENCES flag to duplicate Nodes and changes references to the newly duplicate Nodes Sep 18, 2023
@Mickeon Mickeon changed the title Add DUPLICATE_NODE_REFERENCES flag to duplicate Nodes and changes references to the newly duplicate Nodes Add DUPLICATE_NODE_REFERENCES flag to duplicate Nodes and change references to the newly duplicate Nodes Sep 18, 2023
@Mickeon Mickeon force-pushed the duplicate-keep-node-references-flag branch 2 times, most recently from 321918f to 3886359 Compare September 18, 2023 14:15
@RandomShaper
Copy link
Member

The idea of it being the default behavior of DUPLICATE_USE_INSTANTIATION is intriguing. I worked on this baaack in time, but I think the rationale still stands: duplicating a node, without that flag, means raw duplication, which is what you may want in some cases; however, if your node is a scene/prefab, something you encapsulated to intantiate as a self-contained thing, and you make that explicit by using DUPLICATE_USE_INSTANTIATION, to me, the internal node paths should be fixed up to point within the instantiated scene. So, that not being the case looks like a bug to me indeed.

@RandomShaper
Copy link
Member

Aside, I wonder why DUPLICATE_USE_INSTANTIATION is a must. In other words, iIf we kept DUPLICATE_NODE_REFERENCES for raw duplicate, why wouldn't it work?

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 18, 2023

to me, the internal node paths should be fixed up to point within the instantiated scene. So, that not being the case looks like a bug to me indeed.

I agree wholeheartedly. I would fix it, myself.
Another problem arises, however. Doing so would technically break compatibility.
I personally think it would be worth it. The users from the issue above literally cannot use duplicate normally because it doesn't work the way one may expect, but I'm just one man.

Aside, I wonder why DUPLICATE_USE_INSTANTIATION is a must. In other words, iIf we kept DUPLICATE_NODE_REFERENCES for raw duplicate, why wouldn't it work?

I was waiting to personally test it. I... actually do not know. This actually works without it, I just realised. It's just that it's almost functionally useless without the DUPLICATE_USE_INSTANTIATION or DUPLICATE_SCRIPTS flags, because any scripts the developer may have set are not carried over in any way.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 18, 2023

I want to also note. This was not a possible situation in 3.x because exporting Node properties was not possible (only NodePaths).
This is a new feature of 4.x. @onready Nodes are not affected by this, exported NodePaths are not affected by this. So this may actually be considered unexpected behavior to be fixed, indeed.

@Chaosus Chaosus added this to the 4.2 milestone Sep 18, 2023
@RandomShaper
Copy link
Member

I personally think it would be worth it. The users from the issue above literally cannot use duplicate normally because it doesn't work the way one may expect, but I'm just one man.

I think the revised behavior would be clearly the right one. An explanation in the release notes should be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node.duplicate() does not resolve exported node properties
4 participants