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

(Wrong) Fix webgpu attribute code - but might inspire correct fix #25663

Closed
wants to merge 1 commit into from

Conversation

greggman
Copy link
Contributor

@greggman greggman commented Mar 13, 2023

Related issue: #24005

Description

Wrong fix for WebGPU gLTF support?

This works but is probably the wrong fix? I don't know three.js
that well so I thought I'd post this as an example of a fix
and it might lead to the correct fix.

The issue is WebGPU does not support itemSize = 1 or itemSize = 3
for 8bit or 16bit values.

When using KHR_mesh_quantization some attributes are like Int16x3.
But, according to the KHR_mesh_quantization spec they are actually
padded to Int16x4

In WebGL it's fine to set the attribute to

size = 3, type = gl.SHORT, normalize = true, stride = 8.

But in WebGPU there is no such thing as

size = 3, type = gl.SHORT

If it existed it would be called sint16x3 or uint16x3 or
snorm16x3 or unorm16x3 but none of those exist. Instead
you have to set the attribute to ???x4 for these situations.

You then need to be careful the shader doesn't actually use Z.
The shader can still declare its attribute has vec3f/vec3
but if it declares it as vec4/vec4 the z value would be
undefined (whatever happens to be in the file in the padding)

My guess is the correct fix, rather than this hack which expands
the type, it should look at the stride to figure
out when to expand the type from x3 to x4 or from x1 to x2
or x1 to x4? Even that isn't entire clear though because the
stride might be for interleaved vertex data so it's not clear
how to communicate at a lower level, in WebGPURenderPipeline,
that one of these unsupported types should be promoted to
a supported type.

In any case, with the code in this PR both of compressed glb files that @donmccurdy posted in this PR thread work.

This works but is probably the wrong fix. I don't know three.js
that well so I thought I'd post this as an example of a fix
and it might lead to the correct fix.

The issue is WebGPU does not support itemSize = 1 or itemSize = 3
for 8bit or 16bit values.

When using KHR_mesh_quantization some attributes are like Int16x3.
But, according to the KHR_mesh_quantization spec they are actually
padded to Int16x4

In WebGL it's fine to set the attribute to

```
size = 3, type = gl.SHORT, normalize = true, stride = 8.
```

But in WebGPU there is no such thing as

`size = 3, type = gl.SHORT`

If it existed it would be called `sint16x3` or `uint16x3` or
`snorm16x3` or `unorm16x3` but none of those exist. Instead
you have to set the attribute to `???x4` for these situations.

You then need to be careful the shader doesn't actually use Z.
The shader can still declare its attribute has vec3f/vec3<f32>
but if it declares it as vec4/vec4<f32> the z value would be
undefined (whatever happens to be in the file in the padding)

My guess is the correct fix, rather than this hack which expands
the type, it should look at the stride. The stride to figure
out when to expand the type from x3 to x4 or from x1 to x2
or x1 to x4
@greggman greggman force-pushed the fix-webgpu-attribute-code branch from 6dd385a to 6751276 Compare March 13, 2023 06:09

} else {
format = typeArraysToVertexFormatPrefixForItemSize1.get(ArrayType)

Check notice

Code scanning / CodeQL

Semicolon insertion

Avoid automated semicolon insertion (91% of all statements in [the enclosing function](1) have an explicit semicolon).
@donmccurdy
Copy link
Collaborator

Would it be fair to say that THREE.WebGPURenderer does not support interleaved vertex attributes at all yet? That appears to be the case, and because WebGPU requires padding on uint16x3 and other vertex layouts, we'll be using InterleavedBufferAttribute for those even if they aren't "interleaved" with other attributes.

Related — we will need to consider InterleavedBuffer#stride in the eventual fix.

@sunag
Copy link
Collaborator

sunag commented Mar 14, 2023

I added support for this here ( #24117 ), but following this issue it appears to be incomplete. I will check and release a possible fix as soon as possible.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 19, 2023

@sunag A user in the forum recently wanted to load an optimized glTF asset with KTX2 textures and compressed attribute data in its WebGPU three.js app (https://discourse.threejs.org/t/load-glb-models-in-webgpu/50397). Both features did not work however the KTX2 side is fixed via recent PRs (#25864, #25865, #25867).

I have remembered this discussion/PR and wonder now if you already have made progress in fully supporting interleaved vertex data. TBH, this topic isn't trivial and more complicated to implement than the missing texture compression support. Simply because the WebGPU spec is so different in that regard compared to WebGL.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 19, 2023

BTW: An ideal asset for testing is https://github.com/mrdoob/three.js/blob/dev/examples/models/gltf/coffeemat.glb.

@donmccurdy
Copy link
Collaborator

We could consider improvements to the InterleavedBuffer API, which could benefit both WebGL and WebGPU. Related:

@sunag
Copy link
Collaborator

sunag commented Apr 19, 2023

I have remembered this discussion/PR and wonder now if you already have made progress in fully supporting interleaved vertex data.

I didn't really like the results I got, in the tests I had forced to do the attribute conversion via CPU, I thought I'd go back to that when I had a better idea, I'd try again using Compute but that would be a trial too. I am finalizing the implementation of an API for be used in transmission and I will start PMREM next week, I think I can finish like this after implementing the API that will be used in transmission.

@sunag
Copy link
Collaborator

sunag commented Apr 24, 2023

@Mugen87 Your asset made it easy, thanks. It because I didn't need to resize the array, we're making progress on that.
@greggman Your code helped a lot, thank you for the initiative.

Tomorrow I'll be posting even not fully finished, it remains to resize the arrays if necessary and improve the pipeline to optimize gpu calls

image

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 25, 2023

Closing in favor of #25924.

@Mugen87 Mugen87 closed this Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants