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

Supplement the case of scene instantiation for "Editable Children" #81530

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

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Sep 11, 2023

This is a follow-up to #65011.

For scenes with Editable Children enabled, the main scene will record more information and resource mapping will be valid for multiple nodes. See #64487 (comment).

@akien-mga akien-mga added this to the 4.2 milestone Sep 11, 2023
@akien-mga akien-mga requested a review from KoBeWi September 11, 2023 07:26
@Rindbee Rindbee force-pushed the supplement-scene-instantiation-for-editable-children branch from e5d0d2e to cea3ff6 Compare September 11, 2023 07:54
@KoBeWi
Copy link
Member

KoBeWi commented Oct 22, 2023

What does it fix exactly?

@Rindbee Rindbee force-pushed the supplement-scene-instantiation-for-editable-children branch from cea3ff6 to 216ec91 Compare October 23, 2023 00:59
@Rindbee
Copy link
Contributor Author

Rindbee commented Oct 23, 2023

When Editable Children is enabled, due to copying nodes or specifying the same resource instance for different nodes in batches, the same resource entries will be recorded when saving. When opened again, resources with local_to_scene enabled should be loaded as different instances for different sub-scenes. See #64487 (comment).

demo.zip

A test project is provided here. Before opening the scene, please check the text of test*.tscn. Nodes in different scenes may have the same resource records. These same resource records should be loaded as different instances for different sub-scenes.

0.mp4

@Rindbee Rindbee force-pushed the supplement-scene-instantiation-for-editable-children branch from 216ec91 to f7d0c23 Compare October 23, 2023 05:23
@Rindbee
Copy link
Contributor Author

Rindbee commented Oct 23, 2023

In the case in #81530 (comment), there may be multiple resource instances with the same scene_unique_id, I'm not sure if they need to be deduplicated like in resource_format_binary and resource_format_text.

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 force-pushed the supplement-scene-instantiation-for-editable-children branch from f7d0c23 to 439317f Compare October 23, 2023 06:30
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Can't say I fully understand the code changes, but it does fix a bug. I'm not familiar enough with using editable children/local to scene to tell if there are any obvious regressions.

@@ -161,6 +172,7 @@ Node *SceneState::instantiate(GenEditState p_edit_state) const {
bool gen_node_path_cache = p_edit_state != GEN_EDIT_STATE_DISABLED && node_path_cache.is_empty();

HashMap<Ref<Resource>, Ref<Resource>> resources_local_to_scene;
HashMap<Node *, HashMap<Ref<Resource>, Ref<Resource>>> resources_local_to_sub_scene; // Record the mappings in sub-scenes.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to create typedefs for these types, they're very cumbersome

Copy link
Member

@KoBeWi KoBeWi Nov 8, 2023

Choose a reason for hiding this comment

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

We rarely use typedefs, especially for cases like this.

@akien-mga
Copy link
Member

PackedScene changes at this stage seem risky (4.2 RC imminent), so moving to the next milestone.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 12, 2023
@KoBeWi KoBeWi modified the milestones: 4.3, 4.4 Aug 3, 2024
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.

4 participants