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

Fix the behavior of the resource property of the sub-scene root node on instantiation #65011

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Aug 28, 2022

See #64999 (comment), this PR complements it.

The sub-scene root node will be set successively in the sub-scene and the main scene.

In the following tables, the R prefix indicates a resource that does not have resource_local_to_scene enabled. The RL prefix indicates a resource with resource_local_to_scene enabled.

1

A gray background means that the record will be empty; others will still create a new record in the main scene.

2

In the case marked in yellow above, it is actually impossible to determine whether a new RL1 or an old RL0 is used.

The PR is simply to determine intent from the record, the above yellow cases are considered RL0. Mainly the cases when resource_local_to_scene is enabled in main scene.

After setting the properties of the sub-scene root node, call setup_local_to_scene() on the nested resource with resource_local_to_scene enabled so that the nested resource is setup correctly.

When updating resources according to the records of the main scene, use the scene_unique_id in the main scene to prevent the ID of the resource from changing continuously when saving the scene.

Fix #77997, fix #76693.

@Chaosus Chaosus added this to the 4.0 milestone Aug 29, 2022
@Rindbee Rindbee requested a review from a team as a code owner August 29, 2022 04:40
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from 736c5ee to 08f09a2 Compare August 29, 2022 05:13
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.

I can't really test all the new changes. They look overall good.

core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
scene/resources/packed_scene.cpp Outdated Show resolved Hide resolved
@RandomShaper RandomShaper added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Aug 29, 2022
@RandomShaper
Copy link
Member

I was wondering if we could remove part of the recently added logic to scene instantiation (including the present PT) and do the fix instead at scene packing time; i.e., store nodes that wouldn't normally be stored for being pure instances in case they have properties pointing to local-to-scene resources, and store only those properties. @Rindbee, thoughts?

Aside again, I've tagged this PR for cherry-picking into 3.5 and 3.x, like the previous ones regarding this. However, given the changes are starting to get a bit complex and error prone, I'm wondering if they shouldn't be cherry-picked into 3.5 for the time being, to avoid regressions. @akien-mga, thoughts?

@akien-mga
Copy link
Member

akien-mga commented Aug 29, 2022

Yes any change to PackedScene will cause regressions in my experience :) This series of changes should not be cherry-picked for 3.5.

Stable branches should only get safe fixes (or if unsafe, they need to be critical fixes worth the risk of regression).

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 29, 2022

I think it's worth a shot, some things are easier to handle when packing.

@RandomShaper
Copy link
Member

I'll untag from 3.5 all the PRs in the changeset.

@RandomShaper RandomShaper removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 29, 2022
@RandomShaper
Copy link
Member

All the related (merged and not yet merged) PRs are now only tagged for being cherry-picked into 3.x.
That said, @Rindbee, bear in mind that some changes may not be trivially cherry-pickable. Such cases would require dedicated PRs for 3.x.

@RandomShaper
Copy link
Member

I think it's worth a shot, some things are easier to handle when packing.

So, do you still want to have this one merged or you'd rather try the other way first?

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 29, 2022

I'm now more familiar with scene instancing than scene packaging, so learning scene packaging may take more time.

There is no need to merge it now, and it may be revised later.

@RandomShaper
Copy link
Member

Will you split this PR into two? The part affecting instantiation and the part affecting node copy? The latter is ready to be merged right away. The former could be kept in standby until you decide.

@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 29, 2022

The previous copy_from is directly replaced with the attribute value in the source resource, which is similar to the previous situation of the sub-scene root.

When local-to-scene is enabled, it is desirable to keep the previous resource instance, because direct replacement will break the sharing relationship.

These situations may currently exist:

  1. node->res1(local)
  2. node->res2(local)->res1(local)
  3. node->res3->res1(local)

Although the cases of 2 and 3 may be rare.

Now hopefully 1 and 2 are properly resolved. As for 3, it's a bit difficult. Edit: I think its more like a bug, res3 is shared globally.

@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from 08f09a2 to b3ce5d9 Compare August 29, 2022 12:33
@Rindbee
Copy link
Contributor Author

Rindbee commented Aug 29, 2022

OK, what can be done has been done.

  1. node->res3->res1(local)

I think it's better to treat it as an error when instantiating, res3 that can be shared globally should not have local-to-scene sub-resources.

A possible solution is to propagate the local-to-scene of res1 to res3, i.e. res3 is also local-to-scene. It may be more appropriate to deal with it when packing.

@RandomShaper
Copy link
Member

Well, if this already fixes some cases I think the best thing at this point is to have it merged. That will make it easier to track this waterfall of changes about instantiation and local-to-scene resources. If you at some point find a good way to do some of the fixes at packing time, I gues there's no harm in a new PR that partially reverts the current approach and adds the new way.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 10, 2023
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from b3ce5d9 to 9e1c8da Compare June 14, 2023 10:07
@Rindbee Rindbee changed the title Supplements the case where the local-to-scene resource is only set in the main scene Fix the behavior of the resource property of the sub-scene root node on instantiation Jun 14, 2023
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from 9e1c8da to a5de1dd Compare June 15, 2023 07:33
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch 3 times, most recently from 7cece19 to e50ebe3 Compare August 4, 2023 14:20
@YuriSizov YuriSizov requested a review from KoBeWi August 4, 2023 19:18
@akien-mga akien-mga requested a review from reduz August 17, 2023 09:48
core/io/resource.h Outdated Show resolved Hide resolved
core/io/resource.h Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from e50ebe3 to e5d6402 Compare August 17, 2023 13:42
@Rindbee Rindbee requested a review from a team as a code owner August 17, 2023 13:42
core/io/resource.h Outdated Show resolved Hide resolved
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch 3 times, most recently from 37cb86f to fb0bcca Compare August 18, 2023 11:01
@Rindbee Rindbee requested review from reduz and KoBeWi August 18, 2023 11:04
…on instantiation

The sub-scene root node will be set successively in the sub-scene and the main scene.

The PR is simply to determine intent from the record. Mainly the cases when
`resource_local_to_scene` is enabled in main scene.

When updating resources according to the records of the main scene, use the
`scene_unique_id` in the main scene to prevent the ID of the resource from
changing continuously when saving the scene.
@Rindbee Rindbee force-pushed the fix-first-set-in-main-scene branch from fb0bcca to bd42d33 Compare August 18, 2023 11:09
@akien-mga akien-mga merged commit 4330a94 into godotengine:master Sep 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Rindbee Rindbee deleted the fix-first-set-in-main-scene branch September 8, 2023 10:47
Rindbee added a commit to Rindbee/godot that referenced this pull request Oct 23, 2023
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 added a commit to Rindbee/godot that referenced this pull request Oct 23, 2023
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.
@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
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:core
Projects
None yet
8 participants