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

EXT_mesh_gpu_instancing: Allow quantized quaternions #1820

Merged
merged 3 commits into from
Aug 6, 2020
Merged

EXT_mesh_gpu_instancing: Allow quantized quaternions #1820

merged 3 commits into from
Aug 6, 2020

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Jun 2, 2020

This change cleans up the spec and explicitly allows quantized
quaternions with types BYTE and SHORT.

Also add an implementation note suggesting that the base mesh should not
be rendered.

Fixes #1818.

This change cleans up the spec and explicitly allows quantized
quaternions with types BYTE and SHORT.

Also add an implementation note suggesting that the base mesh should not
be rendered.
@zeux
Copy link
Contributor Author

zeux commented Jun 2, 2020

I've limited the expansion to BYTE and SHORT types, as based on prior experience of working with animation data, unsigned quaternion formats have extremely limited use even for animations; I'd expect their utility to be even more limited for instancing. Byte quaternions generally are pretty low precision, with an angular error of a few degrees, but it actually might be reasonable for some instancing applications where the precision of rotation isn't critical (piles of debris) so keeping it here.

Also added an implementation note for base mesh, which is indicative of a possible problem where there's no way for this extension to be meaningfully made optional. Perhaps this necessitates a revision where we include a required mesh into the extension blob, disassociating it from existing mesh reference?

@lexaknyazev
Copy link
Member

Perhaps this necessitates a revision where we include a required mesh into the extension blob, disassociating it from existing mesh reference?

How would the EXT_mesh_gpu_instancing.mesh interact with morph targets and skinning?

On a side note, does "the node's world transform" include node's own transform (TRS or matrix) or refers only to its parents?

@zeux
Copy link
Contributor Author

zeux commented Jun 2, 2020

How would the EXT_mesh_gpu_instancing.mesh interact with morph targets and skinning?

Good question. I'm wondering what the interaction is right now - the node's transform can't affect skinned mesh transform, so I'm not sure what the expectations are for skinned meshes (one can imagine that for skinned meshes the math is InstanceXform * JointXform * position and for non-skinned meshes the math is NodeXform * InstanceXform * position, which is the only way to allow instancing of skinned meshes, but it's not clear what the transform math even is in the current extension.

For morph targets, there don't seem to be obstacles to making the extension work either way, I'm assuming it uses the usual rules of inheriting morph weights from mesh (if present) and/or node (if present). With no ability to specify morph transforms per-instance. Having said that I strongly doubt any renderers will implement instance x morph combination so maybe we should leave a note saying that it's not guaranteed to be supported?

On a side note, does "the node's world transform" include node's own transform (TRS or matrix) or refers only to its parents?

It should. I'll clarify the wording.

Clarify transformation order
@zeux
Copy link
Contributor Author

zeux commented Jun 2, 2020

Added a section that's explicitly defining the transformation order and involved components. It currently doesn't contain information about skinning - should I add a note saying that skinning is not supported by the draft?

@donmccurdy
Copy link
Contributor

How would the EXT_mesh_gpu_instancing.mesh interact with morph targets and skinning?

Let's open a new issue for this? It's a good question but I think this PR could be merged as-is.

@donmccurdy
Copy link
Contributor

@ultrafishotoy I understand you're already using this extension — any objection to these changes?

@ultrafishotoy
Copy link
Contributor

@ultrafishotoy I understand you're already using this extension — any objection to these changes?

not at all!

@emackey emackey merged commit ba36f57 into KhronosGroup:master Aug 6, 2020
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.

EXT_mesh_gpu_instancing should support quantized quaternions
5 participants