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

Respect cache mode when loading external dependencies. #87759

Closed

Conversation

HolonProduction
Copy link
Member

@HolonProduction HolonProduction commented Jan 30, 2024

This line is part of #86960, but this seems to have quite a way ahead of it and I need it to fix the CI for #86973.

Also this might fix
#82830

@AThousandShips
Copy link
Member

AThousandShips commented Jan 31, 2024

This should also be done in the binary format if done here, the same code is used there with the reuse mode

external_resources.write[i].load_token = ResourceLoader::_load_start(path, external_resources[i].type, use_sub_threads ? ResourceLoader::LOAD_THREAD_DISTRIBUTE : ResourceLoader::LOAD_THREAD_FROM_CURRENT, ResourceFormatLoader::CACHE_MODE_REUSE);

You also have:

Ref<Resource> res = ResourceLoader::load(path, exttype);

Which might be good to also make use the same cache mode

@HolonProduction
Copy link
Member Author

Done

@RandomShaper
Copy link
Member

I think this change makes sense. I'd like more people looking into it, though.

One thing I'd like to point out is that, maybe, the editor is relying on the current behavior not to have to reload external resources when loading a scene:

Ref<PackedScene> sdata = ResourceLoader::load(lpath, "", ResourceFormatLoader::CACHE_MODE_REPLACE, &err);

If that were the case, we may need to add a mode, such as CACHE_MODE_REPLACE_ROOT_REUSE_OTHERS, but with a more compact name.

@RandomShaper
Copy link
Member

There's another issue. During export, CACHE_MODE_IGNORE is used to load PackedScenes. As a result of that, with this PR applied, the path to base scenes is lost and, therefore, when the exported version of a scene is written to the export directory, an error is printed ("Resource file not found: res:// (expected type: )") and the export data is incomplete.

@RandomShaper
Copy link
Member

RandomShaper commented Feb 22, 2024

One thing I'd like to point out is that, maybe, the editor is relying on the current behavior not to have to reload external resources when loading a scene:

Ref<PackedScene> sdata = ResourceLoader::load(lpath, "", ResourceFormatLoader::CACHE_MODE_REPLACE, &err);

If that were the case, we may need to add a mode, such as CACHE_MODE_REPLACE_ROOT_REUSE_OTHERS, but with a more compact name.

On an n-th thought, if replace is meant to change the way subresources are loaded, I does make sense that the mode is just propagated.

UPDATE: But propagating would mean that subresources of external resources also get the same treatment, when the external resources themselves don't. That doesn't seem correct. So, I'd be back to my original statement.

@HolonProduction
Copy link
Member Author

The change seems to be far more critical than I thought it would be. Closing in favor of #88664.

@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 22, 2024
@HolonProduction HolonProduction deleted the fix-cache-mode branch October 5, 2024 18:57
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.

3 participants