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

Stop caching packed scenes in GDScript cache (on preload) #85501

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

Jordyfel
Copy link
Contributor

@Jordyfel Jordyfel commented Nov 29, 2023

Adresses #85081

This code was added in #67714, with the goal of resolving problems with cyclic references is preload().

However, cyclic scene references still don't work, as illustrated by #70985 and other preload related issues.

Being resources, scenes don't support cyclic references, and caching seems to just make the problem appear a little bit later, and make it harder to debug (unlike with scripts, where this should be fine, because get_shallow_script() is designed to deal with the cyclic reference problem).

IMO this problem cannot be solved from outside core resource functionality and the GDScript module should not try. Pinging @adamscott as the original implementer, please do correct me if I have misunderstood the intent of this code.

Most posted issues regarding preload have no reproduction steps and just include already corrupted scenes, which makes it very hard to tell which this change fixes, however it should improve recovery and consistency of behavior as scenes will for sure be reloaded when the script is recompiled.

Bugsquad edit: Fixes #79545 Fixes #84981 Fixes #79840

@Jordyfel Jordyfel requested a review from a team as a code owner November 29, 2023 10:54
@akien-mga akien-mga added this to the 4.3 milestone Nov 29, 2023
@adamscott
Copy link
Member

Oh, sorry, I did not see your ping! I'll take the time to investigate.

@akien-mga
Copy link
Member

I just pinged you 3 min ago so you're not too late :P

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

To be honest, if I remember correctly, this was legacy code from when I tried to enable PackedScenes cyclic dependencies. Unfortunately, this idea was shut down, but the code stayed.

The all the tests pass and I even tested a small project. Everything seems to still work.

Great job! Thanks!

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 25, 2024
@akien-mga akien-mga merged commit ee5ace1 into godotengine:master Feb 25, 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
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:gdscript
Projects
None yet
3 participants