-
Notifications
You must be signed in to change notification settings - Fork 725
Implement simple cache for decoded ASTC textures #1339
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
base: main
Are you sure you want to change the base?
Implement simple cache for decoded ASTC textures #1339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, to make your PR work with vkb::scene_graph::components::HPPImage
, you would need to add the data_hash
to that class as well.
Besides that, as the caching is an ASTC-specific thing, I would have expected that the hash lives in vkb::sg::ASTC
.
With this PR, you calculate the hash for each and every image, and you calculate it based on the image data itself. Wouldn't it be more efficient, to just hash the URI of that file? That would probably require to just modify the vkb::GLTFLoader
.
Thanks! Prefer to leave it as is, to support cases where the image itself changes, but not its URI. |
This PR breaks batch mode for me. When reacing the hlsl_shaders sample (which worked fine last time I did batch with master) it now asserts in create_vk_image. I'm also getting a lot of (silent?) exceptions when running samples that decode astc now: Not sure if any of these is related, but can't approve this without being able to do a batch run. |
I can reproduce the issue, and found a workaround in my latest commit, but cannot really explain what is wrong, any ideas @asuessenbach ?
These are probably the ones I introduced when a problem occurs trying to load an image from the cache. The first time the cache does not exist, so they will be thrown, but in later runs they should not be there anymore |
Regarding all those exceptions: I don't think, it's the right approach to use exceptions for non-exceptional cases. And... having that |
Any chance of speeding this up? I'm currently doing large changes to the framework, and testing is extremely tedious as all those high-level framework samples use ASTC and every time it needs to decode them. That makes testing very slow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
A minor suggestion: Add the astc cache folder to the .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could delete the warning in gltf_loader.cpp
, line 1489.
Besides that (and the comment below), it looks great now.
{ | ||
LOGW("ASTC cache file {} does not exist. ASTC image will be decoded", path.string()) | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add some
LOGI("Loading ASTC cache data from file: {}", path.string());
Description
Introduce a simple caching mechanism for decoded ASTC textures on platforms that do not support the format. Before decoding, the system will check if there are existing binaries saved locally, and otherwise create them for future runs.
Fixes #1202
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properly