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

Clarify some descriptions regarding resource cache #82884

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Oct 5, 2023

This PR provides more insight on how resource caching works and also adds descriptions to CacheMode enum values.

@KoBeWi KoBeWi added this to the 4.2 milestone Oct 5, 2023
@KoBeWi KoBeWi requested review from a team as code owners October 5, 2023 23:27
@dandeliondino
Copy link

dandeliondino commented Oct 8, 2023

This is very helpful and much needed! I opened #82830 the other day and it appears that some of the perceived inconsistencies may be expected behaviors.

Since I just spent a chunk of time trying (and failing) to effectively use CacheModes, I wanted to share some questions/requests for clarifications that I hope will be useful as feedback coming from a user who does not have experience with this source code.

I say this because perhaps some things here may be working as expected and in accordance with the edits in this PR, but just not in a way that would be obvious to a user like me without deeper understanding of the engine.

CACHE_MODE_IGNORE:

The resource is always loaded from disk, even if a cache entry exists for its path, and the newly loaded copy will not be cached. Instances loaded with this mode will exist independently.
  • PackedScenes that contain saved branches still load any resources located inside of subresources of the saved branches from cache (and save them to cache), while saved branches with subresources do load those subresources from disk. Is this intentional behavior that should also be documented here?

CACHE_MODE_REUSE:

If a resource is cached, returns the cached reference. Otherwise it's loaded from disk.

If the resource has sub-resources, they will not use cache and will always be loaded anew.
  • It appears that resources located inside internal sub-resources of the resource being loaded are still loaded from cache. If this is expected behavior, perhaps this could be clarified?

CACHE_MODE_REPLACE:

If a resource is cached, returns the cached reference. Otherwise it's loaded from disk.

If the resource has sub-resources, they will be fetched from cache if available and/or stored in resource cache using their local paths.
  • It appears that sub-resources are also loaded from disk in replace mode, even if a cached version exists. The exception is if they are a resource inside an internal subresource in a saved branch, in which case they use the cached version.

In addition, whether resources stored in subresources that are saved externally use the cache or are loaded from disk appears to vary based on the type of external resource. If this is expected behavior, should it be noted here that it can vary by resource type?

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 24, 2023

PackedScenes that contain saved branches still load any resources located inside of subresources of the saved branches from cache

I think PackedScenes might be an exception, especially when you need to instantiate them to reach the subresources.

CACHE_MODE_REUSE

The description I written was based only on the source code, but it's so convoluted that apparently I got it wrong. From my testing now, CACHE_MODE_REUSE and REPLACE do exactly the same, so no idea what's the difference. Maybe I'm testing wrong.

Not sure about the other quirks; they can be left for another PR.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 30, 2023

Seems like this PR depends on #84167 now, because the current behavior is wrong. I could document how it's supposed to work, but it might be confusing.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 12, 2023
@YuriSizov
Copy link
Contributor

Feel free to update the descriptions now, if it's required!

@KoBeWi KoBeWi force-pushed the commit_message_cached,_please_refresh branch from 640ceb5 to 5a3c738 Compare December 10, 2023 19:29
@KoBeWi KoBeWi force-pushed the commit_message_cached,_please_refresh branch from 5a3c738 to a1aa1a4 Compare December 10, 2023 19:30
@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 10, 2023

Updated the description of CACHE_MODE_REPLACE.

Also removed the mentions of sub-resources, as I'm not that sure about their inner workings. Possibly they are irrelevant for cache mode and should be mentioned somewhere else.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM

@YuriSizov YuriSizov merged commit 4b258cc into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

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.

5 participants