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

Jump to bottom

Fix resource shared when duplicating an instanced scene #64487

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix resource shared when duplicating an instanced scene #64487

wants to merge 2 commits into from
+100 −35

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Aug 16, 2022 •
edited

Fix #45350, fix #47918.
Depends on #65011.

For resources with resource_local_to_scene enabled in the subscene, the resource is already set when the subscene is instantiated, so does not need to be set again.

@Rindbee Rindbee requested a review from a team as a code owner August 16, 2022 08:44
@Rindbee Rindbee changed the title Fix resource duplication when duplicating an instanced scene Fix resource shared when duplicating an instanced scene Aug 16, 2022
@Calinou Calinou added this to the 4.0 milestone Aug 16, 2022
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from c6f8718 to 661579b Compare August 17, 2022 11:18
@Rindbee Rindbee mentioned this pull request Aug 17, 2022
Open
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from 661579b to f2afbb6 Compare August 17, 2022 23:27
RandomShaper
RandomShaper reviewed Aug 29, 2022
View reviewed changes
scene/main/node.cpp Outdated Show resolved Hide resolved
RandomShaper
RandomShaper requested changes Aug 29, 2022
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Looks good. There's just the comment about structure to improve readability.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from f2afbb6 to a3af0c7 Compare August 29, 2022 08:41
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from a3af0c7 to c4f6fe5 Compare August 29, 2022 09:39
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 29, 2022 •
edited

There is also a similar situation here #64999 (comment)

I suggest to review #65011 first, this pr also needs to use a similar structure.

@RandomShaper RandomShaper added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 29, 2022
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from c4f6fe5 to e8e727f Compare August 29, 2022 12:54
Rindbee
Rindbee commented Aug 29, 2022
View reviewed changes
scene/main/node.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

The build seems to fail.

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 30, 2022 •
edited

Yes, here uses the copy_from after the modified in #65011. Compared with previous, the parameter list is different.

@RandomShaper
Copy link
Member

@Rindbee, you may want to rebase this PR on top of the other one this depends on so CI build passes. When the other one is merged, this one will get automatically trimmed down to the only additional commit.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from e8e727f to f8398dd Compare August 30, 2022 10:27
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 30, 2022 •
edited

I think there are fewer records about the correlation between resources between tscn.

All the related (merged and not yet merged) PRs are based on a judgment: Resources with local-to-scene enabled on the same property of the same object that recorded in the main scene, is a shared copy of the source resource with local-to-scene enabled which recorded in the sub-scene.

In most cases, this may be correct. But in fact, this situation cannot be ruled out: Really hope to use new resources to cover the resources on the scene root.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Feb 10, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Jun 13, 2023

Needs rebase.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from f8398dd to 01becb4 Compare June 14, 2023 10:16
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from 8ebe123 to 0ee053b Compare August 18, 2023 11:10
reduz
reduz reviewed Sep 8, 2023
View reviewed changes
scene/main/node.cpp Outdated Show resolved Hide resolved
reduz
reduz reviewed Sep 8, 2023
View reviewed changes
Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looks OK overall, but make sure to check the comment I left.

@reduz
Copy link
Member

reduz commented Sep 8, 2023

Sorry, I misunderstood the PR, as this is only the change in node. In this case, Its as I mentioned, you have to duplicate the children nodes manually if they have editable children flag set.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch 2 times, most recently from 56d4e49 to e61ce31 Compare September 8, 2023 10:29
@Rindbee Rindbee requested a review from reduz September 8, 2023 10:36
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch 6 times, most recently from 8e68044 to ff6a538 Compare September 10, 2023 01:26
@Rindbee
Copy link
Contributor Author

Rindbee commented Sep 10, 2023

Okay, the cases of enabling/disabling Editable Children has been completed for scene initialization and node duplication. I think it's ready to be reviewed again.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch 3 times, most recently from e58ae4b to 498e3fc Compare September 10, 2023 09:09
@Rindbee
Copy link
Contributor Author

Rindbee commented Sep 10, 2023

Node::_duplicate() does not seem to be a suitable place to remap resources. It seems more appropriate to modify the mapping logic in SceneTreeDock::paste_nodes().

I'll try to modify the mapping logic there later.

@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from 498e3fc to 4b56fe2 Compare September 11, 2023 06:56
@Rindbee Rindbee requested a review from a team as a code owner September 11, 2023 06:56
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from 4b56fe2 to 4f81116 Compare September 11, 2023 08:01
@Rindbee
Supplement the case of scene instantiation for "Editable Children"
216ec91 
This is a follow-up to godotengine#65011.

For scenes with **Editable Children** enabled, the main scene will record
more information and resource mapping will be valid for multiple nodes.
@Rindbee Rindbee mentioned this pull request Oct 23, 2023
Open
@Rindbee
Fix resource shared when duplicating an instanced scene
2737783 
For resources with `resource_local_to_scene` enabled in the sub-scene,
the resource is already set when the sub-scene is instantiated, so does
not need to be set again. Just needs to update the property of the
resource according to the value in the main scene.
@Rindbee Rindbee force-pushed the fix-instantiated-scene-duplicate branch from 4f81116 to 2737783 Compare October 23, 2023 02:47
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 26, 2023
@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Oct 26, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips Awaiting requested review from AThousandShips

@KoBeWi KoBeWi Awaiting requested review from KoBeWi

@RandomShaper RandomShaper Awaiting requested review from RandomShaper

@reduz reduz Awaiting requested review from reduz

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:core
Projects
None yet
Milestone
4.3
8 participants
@Rindbee @akien-mga @RandomShaper @KoBeWi @YuriSizov @reduz @AThousandShips @Calinou