-
Notifications
You must be signed in to change notification settings - Fork 43
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
Samples for KHR_animation_pointer #106
base: main
Are you sure you want to change the base?
Samples for KHR_animation_pointer #106
Conversation
Please add license/copyright info for the used images. |
Please see errors reported in automated validation checks. Converting to Draft until checks pass. |
@DRx3D Validation errors should be ignored for now as they are caused by a bug in the validator. |
This is a great asset for stress-testing the extension. However I see a few things that need to be fixed before it can be merged:
|
Yes, sorry! I want to update the PR, just didn’t get to it yet. I believe the cat asset has a permissive license but I’ll have to include that license of course. I disagree though on the validator warnings regarding unused tangents and UVs; excluding those breaks interoperability for files as one can’t assign a different material that needs these attributes in another software. There should really be separate validator settings for general use assets and final delivery assets. |
Hmm, I see the point of interop. This is not part of the core design philosophy for glTF which is to be a final-mile delivery format, not an interchange format. The glTF Validator is designed (partly) for assessing delivery efficiency. People certainly can and do use glTF for interop, and it does make sense since there are a lot of importers and tools these days. One of the goals of the asset repo is to be a collection of well-formed assets, and passing the validator is one way to help us get there. For interoperability assessment, you could perhaps leverage the glTF Asset Auditor. https://www.khronos.org/gltf/gltf-asset-auditor/ |
I have voiced my opinion on this many times and practical use of glTF confirms that interoperability is a main usecase, independent of whether Khronos holds the „pure distribution format“ stance. Imagine if Blender would exclude attributes when exporting, just so that files are „valid“ — the chaos! Much more often than not these files get imported into some 3D engine and changed further. Personally I have a number of test files that are „strictly valid“ to demonstrate to others how that breaks everyone’s workflows — happy to contribute these as sample assets if wanted. Independent of all the above 🙂I hope to get to update the PR in the next two weeks, I’ll see if I can maybe make variants that are production ready vs. make the validator happy. |
I'm with @hybridherbst on this and I'm citing Postel's Law in support.
My first reaction to reading this was "If I'd heard that a few years ago I would have run a mile" However. I do think it makes sense to separate assets designed to test how missing attributes etc from an asset designed to test a specific feature. |
I should clarify that I'm not speaking on behalf of Khronos. I agree interop is a main use case, in fact it's delineated in the specification: https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html
As I understand it, the glTF Sample Assets repo is meant to distribute a library of well-formed assets. The glTF Validator isn't perfect, but it is a key way for everyone to assess the fitness of an asset. I'm not personally against accepting assets with validator errors; I'm just passing along information I've learned from others here about how best to format new submissions. I'd recommend others with more experience chime in on this, like @lexaknyazev or @emackey or @rudybear. |
}, | ||
"name": "TextureTransform", | ||
"extensions": { | ||
"KHR_texture_transform": {} |
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.
KHR_texture_transform
is only valid on a textureInfo, not a material. https://github.com/KhronosGroup/glTF/tree/main/extensions/2.0/Khronos/KHR_texture_transform
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.
Thanks, that seems to be an oversight in the exporter (the extension on the material is never actually filled with data / animated as far as I can see). All actual data is on textureInfos. Will take a look at the export side!
I think removing the two extraneous |
|
||
Known exporters that support `KHR_animation_pointer` at the time of writing are: | ||
|
||
- [UnityGltf (prefrontal cortex' fork)](https://github.com/prefrontalcortex/unitygltf) |
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.
Blender 4.2.0 and higher can go on this list now.
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.
But actually I'm not sure that glTF-Sample-Assets is the correct place to maintain a list of supported importers and exporters, maybe remove this section from the readme.
As far as I understood the extension, the animation pointer for weights in AnimateAllTheThings is incorrect. It currently references the weights array in node 43 which does not exist (and weights do not have a default value). To fix this it should reference the existing weights property in the mesh. |
This is intentional. The size of the |
Related:
Fixes #82.
Sample assets for KHR_animation_pointer, ranging from "very simple" to "extensive" and "animated extensions".
The assets are the same as in the original PR to the models repository.