Fix behavior of ResourceFormatLoader CACHE_MODE_REPLACE (reverted)

[SaracenOne]

I've noticed that CACHE_MODE_REPLACE doesn't seem to be behaving the way I expect it to when using it load resources which have already been cached, specifically that it doesn't seem to be updating, just fetching from the cache. With this change, I've also modified the scene reimport system to use it since the CACHE_MODE_IGNORE enum it was using before appears to break external resource paths.

Would appreciate any feedback and testing for this PR since this part of the resource loader is an area of engine I'm less familiar with.

[KoBeWi]

In #82884 I tried to add descriptions for cache modes. I assumed the current behavior is intended and described it as:

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.

(in CACHE_MODE_REUSE the part about sub-resources change)

As someone commented, that's not exactly how it works in every case. tbh I'm not sure what it's supposed to do. Maybe as the name says it should replace resources in cache, but the implementation is nothing like that.
EDIT:
nvm, I just noticed enum comments .-.

[SaracenOne]

Yeah, if CACHE_MODE_REPLACE works like this now, indeed it should make the need to use the explicit set_path method redundant.

[KoBeWi]

Resource and subresource use path cache, but replace existing loaded resources when available with information from disk.

This sounds like the ResourceLoader is supposed to reuse cache if the resource file does not exist. Right now trying to REPLACE file that no longer exists results in an error, but what's worse, the resulting null is actually stored in cache.

[SaracenOne]

So just to clarify: do we want to make this so that REPLACE will priotize loading from the disk, but if the file doesn't exist, we fallback on the cache instead?

[KoBeWi]

Yes.
Or at least, if loading fails, it should not replace the cached value with null.

[SaracenOne]

How does this look? I made a change so that if the _thread_load_function doesn't return a valid resource (and we're not using the IGNORE enum), it will attempt to go to the cache instead. I assume this will prevent the cache being set to null if the file goes missing. Not sure if we want the _loaded_callback in this situation or not though.

[SaracenOne]

I'm not 100% sure how I feel about this either. While I agree that, yes, clearing the cache on a failed load is probably no-good, I'm not sure how I feel about it just failing to load from disk silently with no indication that anything failed. In the context I wanted to use this, I wanted it to unambigiously go to disk and if it failed, I'd rather it make it clear that it couldn't load from disk. Still, this is the behaviour described in the enum's comment, so I guess it makes sense to make it behave like this.

[KoBeWi]

I tested it briefly (some obvious cases) and it works correctly.

[RandomShaper]

LGTM!

Just to confirm: in the chat discussion I brought up some piece of logic that I thought was lost. It is calling reset_state() in case of CACHE_MODE_REPLACE if the resource exists and the newly loaded one is of the same type. On closer inspection, I've found that such code is still there in ResourceLoaderText. So, there's no need to re-add it. My question is: with the changes in this PR that logic will be enabled again, wouldn't it? Formerly it was effectively disabled because CACHE_MODE_REPLACE wouldn't be let flow down to the text resource loader. If you can confirm my understanding is correct, I'd say we're fine to go.

[SaracenOne]

@RandomShaper Hmm, I think I understand. Let me know though if you think of anything that I might have missed, but if both you and @KoBeWi think this is okay, I might as well take this out of draft.

[RandomShaper]

Looks great. Also, considering this is now scheduled for 4.3, it should be safe as hell to merge it.

[SaracenOne]

Since this is intended to fix a whole class of bugs with the importer's hot-reloading functionality, would there be any consideration to the possibility of cherry-picking it for a 4.2 in a point release once it's been sufficently battle-tested?

[RandomShaper]

I think it will make sense to cherry-pick indeed. I've labeled appropriately.

[dandeliondino]

Thank you for working on this! I just tested this build artifact with the MRP for #82830 and am getting the same results as in the original issue (originally tested in 4.1.1-stable and 4.2-dev6), with inconsistencies in whether cache is replaced or reused when using nested scenes and external resources. I don't know if this PR was actually intended to fix these inconsistencies or not, but wanted to report my results.

[SaracenOne]

Rebased to fix conflict.

[SaracenOne]

Thank you for working on this! I just tested this build artifact with the MRP for #82830 and am getting the same results as in the original issue (originally tested in 4.1.1-stable and 4.2-dev6), with inconsistencies in whether cache is replaced or reused when using nested scenes and external resources. I don't know if this PR was actually intended to fix these inconsistencies or not, but wanted to report my results.

I'll investigate. This fix was mainly targetting issues with reimporting scenes while they're already opening.

[LakshayaG73]

Will this fix #85892 ?

[AThousandShips]

You can test yourself with the artifacts from the build, once the rebuild is done 🙂 (or compile yourself)

[YuriSizov]

Thanks!

[akien-mga]

We're reverting this for now due to a critical regression #86187 for which we didn't find a fix yet. As mentioned in #86990, this is still a PR that seems to do important fixes so this should be revisited, ideally soon so it can still be merged for a future 4.3 dev snapshot.