X Tutup
Skip to content

GDScript: Ensure correct caching of cyclic references#113600

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
HolonProduction:cache-fixup
Dec 5, 2025
Merged

GDScript: Ensure correct caching of cyclic references#113600
akien-mga merged 1 commit intogodotengine:masterfrom
HolonProduction:cache-fixup

Conversation

@HolonProduction
Copy link
Member

Followup to #109345
Requires and includes #113580

Fixes #113588

Explanation:

  • the original idea of this fix is to only set the path for non fully loaded scripts without caching them in the ResourceCache
  • In the original PR I applied another change on top which tried to ensure, that fully loaded scripts would be cached by the ResourceLoader
    • In hindsight this does not seem to be necessary. And for cyclic references it did not work. Because the GDScript compiler cached the fully loaded script before returning control to the resource loader.
    • The resource loader only sets the path on resources if the resource is not already part of the cache. However the GDScript resource format loader did reset the path before returning control to ensure that Scripts get cached when the ResourceLoader sets it. In the cyclic reference case the ResourceFormatLoader did reset the path but the ResourceLoader did not set it again, because the script was already added to the cache by the compiler.
    • With this PR the ResourceFormatLoaderGDScript is once again responsible for adding fully loaded scripts to the cache (as it was the case before Prevent shallow scripts from leaking into the ResourceCache #109345). This seems to work fine, the ResourceCache can handle it and we can ensure that fully loaded scripts always have their path set correctly.

I retested a lot of MRP's with this PR + #113580

And so far those issues seem to still be fixed with this change:


Note

With the original PR I always had the intention of "fixing forward" as in getting it out and investigating and addressing regressions. I don't think we will be able to make this resource mess work out of the box in a single PR.
What I did not intend was to get this merged so close to release. In fact I suggested giving the original PR a try early in the dev cycle but that never happened.
So if the production team decides for a revert instead of merging my followups that's fine by me.
Just would be nice to give this another try in the next dev cycle if we go with a revert.

@shakesoda
Copy link
Contributor

confirmed on my end this works for both the MRP in the cyclic references issue and my game 🎉

@akien-mga akien-mga added this to the 4.6 milestone Dec 5, 2025
@akien-mga
Copy link
Member

With the original PR I always had the intention of "fixing forward" as in getting it out and investigating and addressing regressions. I don't think we will be able to make this resource mess work out of the box in a single PR.
What I did not intend was to get this merged so close to release. In fact I suggested giving the original PR a try early in the dev cycle but that never happened.
So if the production team decides for a revert instead of merging my followups that's fine by me.
Just would be nice to give this another try in the next dev cycle if we go with a revert.

Yeah it's unfortunate that it didn't get approved earlier in the release cycle. But let's give it a try in dev6/beta1 with the regression fixes and see where to take it from there.

@akien-mga akien-mga merged commit c1bbaca into godotengine:master Dec 5, 2025
20 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.

[GDScript] Script circular references break cache

3 participants

X Tutup