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

Add texture-compression-bc-sliced-3d #4763

Merged
merged 14 commits into from
Jul 25, 2024

Conversation

mehmetoguzderin
Copy link
Member

Fixes #4705

This PR makes base BCn extension 2D only, and requires the bc-sliced-3d extension to allow 3D dimension for BCn textures as sliced 3D.

This extension requires base bc extension, so I tried to phrase it in a succinct way. Please let me know if that would benefit from any improvement.

TYVM!

@mehmetoguzderin mehmetoguzderin added the api WebGPU API label Jul 20, 2024
Copy link
Contributor

github-actions bot commented Jul 20, 2024

Previews, as seen when this build job started (0047db9):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

With the suggested changes I think this will be ready to land.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
@kainino0x kainino0x added needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet api resolved Resolved - waiting for a change to the API specification labels Jul 24, 2024
mehmetoguzderin and others added 5 commits July 25, 2024 12:13
@mehmetoguzderin
Copy link
Member Author

@kainino0x thank you very much for the suggestions, I believe that with reverts and clarifications, the extension as in PR is got better not only for itself but as a template for ASTC Sliced 3D and beyond. However, I'd like to consult for just a few matters:

  1. The outdated suggestion. Guidance would be appreciated.
  2. CTS label. The original 3D issue is still not resolved, so, I renamed it to "Test texture-compression-bc-sliced-3d" in cts repo, and I can continue the progress there. Is that sufficient or is another ticket needed? Test texture-compression-bc-sliced-3d cts#3761
  3. Validation. Do we need to add a validation rule to createTexture or is the extension text sufficient enough? I think it is sufficient and nicely helps to keep validation as simple as "is format present and is 3D checked" but I would like to double check if such an interpretation is in range of desirability.

TYVM!

@kainino0x
Copy link
Contributor

2: Looks good to me
3: Also looks good to me

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x
Copy link
Contributor

LGTM, thank you!

@kainino0x kainino0x added this to the Milestone 0 milestone Jul 25, 2024
@kainino0x kainino0x merged commit 177c640 into gpuweb:main Jul 25, 2024
4 checks passed
@Kangz
Copy link
Contributor

Kangz commented Aug 20, 2024

GPU Web WG 2024-07-24 Atlantic-time
  • KG: Mozilla suggests changing to bc2d, have another bc3d entry. Shaders transpilation to Metal. #5 in the list.
  • CW: solutions we don't want? auto-enable we don't.
  • KN: between Is a clean break from WebGL really necessary? #2 and s/GPU for the Web/GPU on the Web/ #3 I'm fine with both. Is a clean break from WebGL really necessary? #2 is a little simpler to spec. (No new logic for producing an error.)
  • KG: more I think about it, the more I like renaming it to 2D.
  • MOD: option Keep viability for compilation to WebAssembly or transpilation via Emscripten in mind #1 is not optimal. I'm OK with the other options.
  • KN: don't really want to change the name of the extension. We have bc already, we'd be removing it and adding bc2d. On our impl we'd have to deprecate, do warnings, then remove bc. Bunch of work.
  • MM: what's the reason why the other formats could work with 3d but not the other?
  • KN: list of format's the same for the 2 features. Just whether they can be used for 3D textures or not.
  • MM: why not one extension for 3d, the other one for 2d?
  • KN: like ASTC doing it that way? Yes. Wanted symmetry between the compressed texture extensions.
  • MM: symmetry's less important than having 2 extensions.
  • KN: there'll be 2 for BC and 2 for ASTC. And 1 for ETC since it doesn't support 3D slices.
  • GT: ASTC supports 3D 3D, not slices of 3D.
  • KN: that exists in some GL impls. Never existed in Vk, D3D12 or Metal. Not even extensions for them.
  • KG: not just desire for symmetry; it indicates that they behave differently.
  • CW: not sure a rename is super necessary. Option Is a clean break from WebGL really necessary? #2 makes sense. One's the format, and the other one's sliced. But it's always predicated on the 2D support. Small opinions one way or the other, so I propose going with Is a clean break from WebGL really necessary? #2.
  • (consensus)
  • MOD: do we carry over the same idea for the ASTC - that it sets texture creation validation error?
  • KN: it should work the same way as much as possible.
  • KG: agree
  • KN: what you have for the table in the current PR is what you want. Just need clarifying text.
  • MOD: thanks.

juj added a commit to juj/wasm_webgpu that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api resolved Resolved - waiting for a change to the API specification api WebGPU API needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider moving sliced 3D compressed BC texture to an extension.
4 participants