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

Use a skinned model that shows off skinning #25654

Merged
merged 1 commit into from
Mar 12, 2023

Conversation

greggman
Copy link
Contributor

@greggman greggman commented Mar 11, 2023

Fixed #24005.

Description

Update a few skinning examples with a model that shows off skinning more clearly

@greggman greggman force-pushed the use-skinned-model branch from b07f7e5 to 3e54e24 Compare March 11, 2023 16:26
@greggman greggman force-pushed the use-skinned-model branch from 3e54e24 to 2236516 Compare March 11, 2023 16:50
@donmccurdy
Copy link
Collaborator

Shaved a bit off the file size, 3.1 MB → 1.3 MB:

Michelle.glb.zip

@greggman
Copy link
Contributor Author

@donmccurdy, what did you do? what tools/options do you use?

@donmccurdy
Copy link
Collaborator

npm install --global @gltf-transform/cli

gltf-transform optimize input.glb output.glb \
  --simplify 0 \
  --compress quantize \
  --texture-compress webp

@greggman
Copy link
Contributor Author

sadly it apparently uncovered a bug in the Three WebGPU support. With the compressed file I get

Attribute offset (24) with format VertexFormat::Float32x4 (size: 16) doesn't fit in the vertex buffer stride (28).
 - While validating attributes[0].
 - While validating buffers[2].
 - While validating vertex state.
 - While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor]).

webgpu_skinning.html:1

@donmccurdy
Copy link
Collaborator

Ah, possibly we don't support interleaved vertex attributes in WebGPU yet? This version should work if that's the cause, I added --vertex-layout separate.

Michelle.glb.zip

If that's not fixing it no worries, feel free to keep the existing model!

@greggman
Copy link
Contributor Author

That one got a similar error.

Attribute offset (0) with format VertexFormat::Float32x3 (size: 12) doesn't fit in the vertex buffer stride (8).

  • While validating attributes[0].
  • While validating buffers[0].
  • While validating vertex state.
  • While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor]).

I'll use the bigger file for now. I don't know if I have time to fix the WebGPU code. I'm sure it's more complicated but the error is straight forward so maybe there's a possibility that it's a simple fix.

@LeviPesin
Copy link
Contributor

Seems like the issue is in this file.

@LeviPesin
Copy link
Contributor

Related: #25249
I'm not sure how much correct was that fix and could a similar one be applied here... You should probably create a new issue.

@greggman
Copy link
Contributor Author

AFAICT there's a normalized Int16x3 attribute in the file. WebGPU only supports 32bit formats x3. It doesn't support any 16 bit formats x3 nor x1, nor 8bit formats by x3 nor x1

Two workarounds would be to either, at load time, convert the data to something WebGPU supports, or stop using attributes and use storage buffers and pull the data out in the shaders.

@donmccurdy
Copy link
Collaborator

Ok, sorry for the trouble! Ignore the compressed model for now. I'll have to look into what we can do with WebGPU, maybe need the tool to have an option padding the buffer for the correct byte stride.

@Mugen87 Mugen87 added this to the r151 milestone Mar 12, 2023
@Mugen87 Mugen87 merged commit 50ab636 into mrdoob:dev Mar 12, 2023
@greggman
Copy link
Contributor Author

maybe need the tool to have an option padding the buffer for the correct byte stride.

I'd think you'd want GLTFLoader to load any GLTF and not just padded ones. I'm not sure where is best to fix it. In the GLTFLoader (in which case it would have to be aware that it's loading for WebGPU vs WebGL) or in the WebGPURender (trying to make it either support 8bit and 16bit x3 and x1) directly via shader contortions or trying to make it do the conversion.

My guess would be it'd be best to make the loader do this. That would keep the renderer simpler

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 12, 2023

Ah, no I wouldn't want to change the renderer here either. And ideally GLTFLoader can just upload buffers without repacking them vertex by vertex. I'm thinking perhaps glTF Transform needs to have an additional vertex layout option, when quantizing geometry for WebGPU.

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.

Should skinning examples show skinning?
4 participants