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

Remove early return for cached resources in GDScript::set_path_cache() #102063

Conversation

gofastlily
Copy link
Contributor

Fix #99006

From what I understand the logic that was removed meant that if a script were cached, it would never set the path cache, just the path.

I believe removing that check and early exit to guarantee setting the path cache is the appropriate fix.

@RandomShaper
Copy link
Member

The fix may have to do with that, but I'm not sure about potential regressions. The GDScript cache is built, as I understand it, as a parallel cache to the main ResourceCache. The fact that this function needed to be overridden makes me think there are some assumptions at play that may break. set_path_cache() was intended as a loophole so one could set the path of a resource without it being added to the resource cache. That was useful, for instance, if a resource was loaded with CACHE_MODE_IGNORE yet you were still interested in the path it was loaded from.

So, if set_path_cache() in GDScript actually puts the resource in the GDScript's own cache, again, I can't be sure about the potential unintended consequences of this change unless all the GDScript caching logic is revised. There are other issues that may need something as deep as that to be fixed, like #101018 (see my comment there).

@gofastlily
Copy link
Contributor Author

Okay, thanks for the extra context! I'll do some more digging to try and see what's happening here.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 30, 2025

If this doesn't bring back #95909 then it's probably fine.

@akien-mga akien-mga changed the title Fix set_path_cache() Remove early return for cached resources in GDScript::set_path_cache() Jan 30, 2025
@KoBeWi
Copy link
Member

KoBeWi commented Jan 30, 2025

Does not fix #101615, which is a regression from the same PR.

I tried testing #95909 and I can reproduce it with and without #96499 🤷‍♂️

akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 4, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
@akien-mga
Copy link
Member

Superseded by #102429. Thanks for the contribution!

@akien-mga akien-mga closed this Feb 4, 2025
@akien-mga akien-mga removed this from the 4.4 milestone Feb 4, 2025
@gofastlily gofastlily deleted the fix-99006-built-in-script-live-reloading branch February 4, 2025 18:55
sondredl pushed a commit to sondredl/godot that referenced this pull request Feb 6, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
SirQuartz pushed a commit to SirQuartz/godot that referenced this pull request Feb 7, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
dugramen pushed a commit to dugramen/godot that referenced this pull request Feb 8, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Feb 17, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Feb 17, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Feb 20, 2025
This reverts commit fd5fc9f.

This caused significant regressions which are worse than the bug that godotengine#96499
aimed to address.

- Reverts godotengine#96499.
- Reopens godotengine#95909.
- Supersedes godotengine#102063.
- Fixes godotengine#99006.
- Fixes godotengine#101615.
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.

Built-in script live reloading is broken again
5 participants