-
Notifications
You must be signed in to change notification settings - Fork 946
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
Support texture-compression-bc-sliced-3d
in wgpu
#5751
Support texture-compression-bc-sliced-3d
in wgpu
#5751
Conversation
I do have a larger concern about this though; with the availability of ASTC/ETC formats on Vulkan gpuweb/gpuweb#3183 (comment) and it seems ETC formats on Metal as well (contrary to their docs) gpuweb/gpuweb#3183 (comment). |
@teoxoy right, seems like further investigation is necessary as to only allow BCn or also ASTC (ETC2 seems harder). |
Co-authored-by: Teodor Tanasoaia <[email protected]>
The macOS runner seems to be failing a lot of tests (looks like the ones that actually execute something on the queue) right after we start executing
For reference, this is how long the clear texture ones take on trunk:
This is odd since the BC test eventually succeeds but there is some internal state getting messed up making the device unusable even across tests? |
@teoxoy On my local device (just baseline M1), I get the following runtime for BCn clear tests:
So, unfortunately, I am unable to reproduce the CI's situation locally. BTW, I just checked the validation layers on my macOS environment, and they are all set to valid paths, which seem to work across many apps, including Vulkan Configurator itself, yet I still get the same |
That sounds like another curious issue tracked by #5665. |
@teoxoy apologies for the sudden ping during these busy times. As I was looking into migrating to extension, I got curious, although the tests pass with my local, non-paravirtualized M1 drivers, in case restructuring into extension keeps throwing errors for paravirtual driver, is it reasonable to tentatively blocklist this extension for Apple platforms? Is there a policy to allow partial forward progress with corner cases like this? All the best. |
No worries. Yes there is a |
@teoxoy thank you for the insight! I will be conscious of it as I update the PR. |
texture-compression-bc-sliced-3d
in wgpu
(Merging trunk made this branch go green with CI, interesting! Hopefully that'll hold valid by the time I update into extension) |
@teoxoy sorry for the ping again. I finally got around to looking at migrating to the extension structure. First of all, apologies for the increase in surface area of this PR due to the delay in putting the support behind a feature. I tried my best to:
Furthermore, I noticed that the feature comment for BC was outdated; nowadays, all Apple9 iOS and iPadOS devices, as well as some Apple7 and Apple8 iPadOS devices, support BC as mobile alongside desktops. However, the changes might have been less than optimal for a few aspects:
I just enabled the SLICED_3D feature for testing, as BC simply guarantees it. I'd appreciate it if you could take a look at your convenience. I'll try to improve in case there's a less-than-desirable aspect in the current shape of PR depending on review. On the good news, CI got green once again (which might let us put things in TODO if need be and go incremental)! |
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.
No problem, thanks for updating the PR!
For point 1, I think we should keep all the feature flags ordered, there is also this comment that needs updating:
Lines 418 to 420 in f6a3eef
// Bits 11-19 available for webgpu features. Should you chose to use some of them for | |
// for native features, don't forget to update `all_webgpu_mask` and `all_native_mask` | |
// accordingly. |
For point 2, I think it's fine as it is.
@teoxoy thank you very much for your kind review. Good catch, sorry for the overlook! The latest commit addresses that and makes the flag counting in order as they appear in spec. I appreciate the feedback, please let me know if there's anything further to refine this PR to make sure it is a reasonable commit for the project, and thank you and the team for all the efforts to keep wgpu a great project. |
Thanks for your kind words and your contributions! :) |
@teoxoy Likewise, thank you for the merge! |
Fixes #5750
Since it has been a while since I created a PR for wgpu, I might have overlooked some aspects. Your guidance would be appreciated. Thank you!