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

Enhance cache modes in resource loading #88664

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

RandomShaper
Copy link
Member

This would supersede #87759, addressing the concerns discussed there.

For context, quoting f8d03b9, where the original intent of CACHE_MODE_REUSE was stated (in essence, only affects subresources); bolds mine:

Improve resource load cache
-Added a new method in Resource: reset_state , used for reloading the same resource from disk
-Added a new cache mode "replace" in ResourceLoader, which reuses existing loaded sub-resources but resets their data from disk (or replaces them if they chaged type)
-Because the correct sub-resource paths are always loaded now, this fixes bugs with subresource folding or subresource ordering when saving.

Finally, it's a bit unfortunate that the cache modes are exposed in two places, which forces duplicate docs. The next compat breakage would be a nice opportunity for changing the exposed ResourceFormatLoader.load() to refer to the enum in ResourceLoader.

Comment on lines +66 to +68
CACHE_MODE_IGNORE,
CACHE_MODE_REUSE,
CACHE_MODE_REPLACE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the prior comments were completely wrong I don't see why removing them outright.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed them to keep the docs as the only source of truth. Well, actually two sources of truth, because due to how this enum is doubly-exposed, they have to be repeated. I had the impression that, when there's a natural place in the docs for some item, the unwritten rule is to document there instead of the source code. I can understand some will check the C++ enum, but if we keep explanations there, we'll already have three copies. Sounds like it's too easy for them to get out-of-sync over maintenance.

Copy link
Contributor

@Mickeon Mickeon Feb 23, 2024

Choose a reason for hiding this comment

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

That's true, but for very "deep" and somewhat important constants such as these, it's very unlikely their commented behavior "desyncs" with the documentation, unnoticed.

But at the same time you did have to make this PR in part because of that, so I can understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see if we can get more opinions for the scales to tip one way or the other.

Copy link
Member

Choose a reason for hiding this comment

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

The current documentation was copied from these comments. I think it's fine to remove them now.

CACHE_MODE_IGNORE, // Resource and subresources do not use path cache, no path is set into resource.
CACHE_MODE_REUSE, // Resource and subresources use patch cache, reuse existing loaded resources instead of loading from disk when available.
CACHE_MODE_REPLACE, // Resource and subresource use path cache, but replace existing loaded resources when available with information from disk.
CACHE_MODE_IGNORE,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe something like this is doable and it would ensure we at least know there's some duplication when reading code.

Suggested change
CACHE_MODE_IGNORE,
CACHE_MODE_IGNORE = ResourceLoader.CACHE_MODE_IGNORE,

I may be completely mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. However, I think that ship already sailed. This enum is not the only one duplicated across the source code. If we want to embrace an idiom like that to send a signal that this is just a mirror image of another (which I believe is a good idea), I'd suggest making a codebase-wide pass editing all cases. Also, that would be a good thing to make it easier to search for them to hopefully remove all duplicates next time we can break compat.

@@ -138,13 +138,19 @@
The resource was loaded successfully and can be accessed via [method load_threaded_get].
</constant>
<constant name="CACHE_MODE_IGNORE" value="0" enum="CacheMode">
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.
Resource and subresources are neither retrieved from nor stored into the resource cache. External resources are loaded with [constant CACHE_MODE_REUSE].
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmh, more to sink my teeth into. Yummy.
It's weird. The current descriptions in this PR are fairly hard to follow. Also worth noting that the class reference often mentions "from disk" when talking about non-volatile memory.

I do have a suggestion but as I am looking at both old descriptions and this PR's behavior I'm not sure how to word it better at the moment.

Suggested change
Resource and subresources are neither retrieved from nor stored into the resource cache. External resources are loaded with [constant CACHE_MODE_REUSE].
The resource and its subresources are [b]always[/b] loaded and stored to disk, and are not cached. A resource from a file is loaded from cache when possible. Otherwise, it is loaded from disk and cached.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open-arms to better-worded descriptions. English is, as one can notice, not my mother tongue. Also, I was too focused on making them technically accurate that I may have not written the friendliest descriptions. That said, this suggestion of yours contains a mistake, when you say stored to disk. The resources are loaded from disk and stored (in)to cache.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 24, 2024

I gave it a test and I don't get the difference between CACHE_MODE_REPLACE and CACHE_MODE_REPLACE_DEEP, and I don't see CACHE_MODE_REPLACE_DEEP actually used anywhere. I assumed that CACHE_MODE_REPLACE would reload a resource without reloading its external dependencies, but the dependencies get reloaded too. Same with CACHE_MODE_IGNORE.

EDIT:
Ok now I noticed the cache_mode_for_external. I still get wrong results though.

EDIT2:
lol I know why. You are assigning cache_mode_for_external unconditionally. It should use CACHE_MODE_REUSE by default, like before.

doc/classes/ResourceFormatLoader.xml Outdated Show resolved Hide resolved
doc/classes/ResourceFormatLoader.xml Outdated Show resolved Hide resolved
doc/classes/ResourceLoader.xml Outdated Show resolved Hide resolved
doc/classes/ResourceLoader.xml Outdated Show resolved Hide resolved
- Unify documentation, hoping to clear misconcepctions about about propagation of the cache mode across dependant loads.
- Clarify in docs that `CACHE_MODE_REPLACE` now also works on the main resource (from godotengine#87008).
- Add two recursive modes, counterparts of `CACHE_MODE_REPLACE` and `CACHE_MODE_IGNORE`, since it seems some need them (see godotengine#59669, godotengine#82830).
- Let resources, even loaded with one of the ignore-cache modes, get a path, which is useful for tools.
@akien-mga akien-mga merged commit 172b254 into godotengine:master Feb 29, 2024
16 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.

5 participants