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 recursive resource local to scene (array/dictionary properties on resources) #71578

Merged

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Jan 17, 2023

Related to #71243
Fixes #71406
Fixes #85614

Any resource that contains arrays or dictionaries with local to scene resources will now be duplicated and configured correctly. This also includes built-in nodes, such as when marking a mesh's surface materials as local to scene.

A side effect of this change is that any array and dictionary property is now duplicated for local-to-scene instances.

What isn't handled in this PR is the case where a scene's node has an exported array/dictionary property containing local to scene resources. That code has a lot more special cases that I don't fully understand yet, but the solution would look largely similar.


NOTE: Using resources as dictionary keys does not work with the inspector (they get saved as null into the scene file), so this code branch was not tested.

@RedMser RedMser requested a review from a team as a code owner January 17, 2023 20:19
@RedMser RedMser changed the title Fix recursive resource local to scene Fix recursive resource local to scene (array/dictionary properties on resources) Jan 17, 2023
@RedMser RedMser marked this pull request as draft January 17, 2023 20:47
@RedMser

This comment was marked as outdated.

@RedMser RedMser force-pushed the fix-recursive-resource-local-to-scene branch from a77ad0a to bf10522 Compare January 17, 2023 21:13
@RedMser RedMser marked this pull request as ready for review January 17, 2023 21:15
@Calinou Calinou added this to the 4.0 milestone Jan 17, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@Luzzotica
Copy link

I would like to push this if possible.

I'm building a game that relies heavily on resources to decouple systems and I have a lot of resources in arrays and dictionaries.

With this merged in I can at least create a workaround by only having a Local to Scene resource hold my arrays of resources. Not 100% optimal, but it would let me do what I want to do.

Also a little annoying to debug this issue when you first discover the Local to Scene checkbox.

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@akien-mga akien-mga requested review from KoBeWi and RandomShaper June 19, 2023 20:31
@Eliwi
Copy link

Eliwi commented Aug 1, 2023

Can I push this again? It's really frustrating to debug for days until you find out that the error is because the resources inside an array are shared even if specified differently. Makes the use of Resources for anything more than surface-level data-objects needlessly difficult.

core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
@RedMser
Copy link
Contributor Author

RedMser commented Oct 20, 2023

Rebased and applied review feedback, which cleaned up the code by a lot! :D
Also tested both issue's MRPs again and they behave as expected.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 20, 2023

Seems like it fixes #71243 too, not sure why you didn't link it.

@RedMser
Copy link
Contributor Author

RedMser commented Oct 20, 2023

Seems like it fixes #71243 too, not sure why you didn't link it.

It doesn't fix the case where an array or a dictionary with sub-resources is exported directly on a Node (as opposed to being a property on a Resource).

To fix it, we would need changes in the PackedScene loading code, which looks scary to me since it also needs to handle scene inheritance among other things.

Here's what that issue's MRP prints with this PR. Note how the local scene is still null for array/dictionary entries of the node properties. Also note that the values for these three properties is not the same when compared to the initial state (lines marked with an arrow).

Output Log
pre-modified first ---------
simple:
true	1	LocalToSceneTests1:<Node#26323453127>
array:
true	2	<Object#null>
true	3	<Object#null>
dict:
true	4	<Object#null>
sub:
true	5	LocalToSceneTests1:<Node#26323453127>
sub array:
true	6	LocalToSceneTests1:<Node#26323453127>
sub dict:
true	20	LocalToSceneTests1:<Node#26323453127>
check first --------- (should be same as above)
simple:
true	1	LocalToSceneTests1:<Node#26323453127>
array:
true	4	<Object#null>       <-------------- should be 2
true	6	<Object#null>       <-------------- should be 3
dict:
true	8	<Object#null>       <-------------- should be 4
sub:
true	5	LocalToSceneTests1:<Node#26323453127>
sub array:
true	6	LocalToSceneTests1:<Node#26323453127>
sub dict:
true	20	LocalToSceneTests1:<Node#26323453127>
check second --------- (should be *= 2 on each field)
simple:
true	2	LocalToSceneTests2:<Node#26440893662>
array:
true	4	<Object#null>
true	6	<Object#null>
dict:
true	8	<Object#null>
sub:
true	10	LocalToSceneTests2:<Node#26440893662>
sub array:
true	12	LocalToSceneTests2:<Node#26440893662>
sub dict:
true	40	LocalToSceneTests2:<Node#26440893662>

core/io/resource.cpp Outdated Show resolved Hide resolved
core/io/resource.h Outdated Show resolved Hide resolved
@RedMser RedMser force-pushed the fix-recursive-resource-local-to-scene branch from c628e01 to f83a532 Compare October 20, 2023 15:15
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.

It fixes the issue and seems to work correctly. Not sure how to test for potential regressions, probably still needs checking from core team to spot if something is obviously wrong.

You should squash the commits into 1.

@RedMser RedMser force-pushed the fix-recursive-resource-local-to-scene branch from f83a532 to 47130f8 Compare October 20, 2023 15:24
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 26, 2023
@akien-mga
Copy link
Member

Should be good to merge now, but could be worth a rebase to make sure everything still builds fine.

Any resource that contains other local to scene resources inside of
arrays or dictionaries will now be duplicated and configured.

The case where a scene's node has an exported array/dictionary
property containing local to scene resources is NOT handled here.
@RedMser RedMser force-pushed the fix-recursive-resource-local-to-scene branch from 47130f8 to 608b5d2 Compare January 11, 2024 19:02
@akien-mga akien-mga merged commit e6536f0 into godotengine:master Jan 11, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Local to Scene doesn't work when inside exported arrays Materials Not Honoring Local to Scene
7 participants