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

Samples for KHR_animation_pointer #106

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hybridherbst
Copy link
Contributor

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.

@lexaknyazev
Copy link
Member

Please add license/copyright info for the used images.

@DRx3D
Copy link
Contributor

DRx3D commented Feb 26, 2024

Please see errors reported in automated validation checks. Converting to Draft until checks pass.

@DRx3D DRx3D marked this pull request as draft February 26, 2024 20:59
@lexaknyazev
Copy link
Member

@DRx3D Validation errors should be ignored for now as they are caused by a bug in the validator.

@echadwick-dgg3d
Copy link

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:

  1. Readme needs at least one screenshot. See SubmittingModels for guidelines. There should be a a larger version for embedding in the readme, and a smaller version for thumbnail use. An animated GIF might make sense given the context.
  2. The cat texture should be replaced with something you have personally made. A reverse image search shows it may be created by "klimkin" who seems to have an attribution stipulation: https://pixabay.com/photos/cat-hunting-siamese-eyes-close-up-2529563/ and also the Pixabay site has some license limitations: https://pixabay.com/service/license-summary/. Because of these complications I think you should just use a different image.
  3. glTF Validator has a bunch of errors, many are severity 0 and severity 1. Some can be attributed to lack of validator support for KHR_animation_pointer, but others seem be unrelated asset problems. I'm not sure we want an asset in the repo with so many errors? Empty nodes, unused UVs, unused tangents, duplicate animation targets, skinned mesh errors.

@hybridherbst
Copy link
Contributor Author

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.

@echadwick-artist
Copy link
Contributor

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/

@hybridherbst
Copy link
Contributor Author

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.

@andybak
Copy link

andybak commented Jul 2, 2024

I'm with @hybridherbst on this and I'm citing Postel's Law in support.

This is not part of the core design philosophy for glTF which is to be a final-mile delivery format, not an interchange format.

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.

@echadwick-dgg3d
Copy link

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

glTF is an API-neutral runtime asset delivery format. glTF bridges the gap between 3D content creation tools and modern graphics applications by providing an efficient, extensible, interoperable format for the transmission and loading of 3D content.
...
The glTF Specification is intended for use by both implementers of the asset exporters or converters (e.g., digital content creation tools) and application developers seeking to import or load glTF assets, forming a basis for interoperability between these parties.

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": {}

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

Copy link
Contributor Author

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!

@emackey
Copy link
Member

emackey commented Aug 1, 2024

I think removing the two extraneous KHR_texture_transform from line 7355 and line 7397 will be enough to get validation to pass here. The texture transform is only allowed on textureInfo of course, not on whole materials, which is where the validation error comes from.


Known exporters that support `KHR_animation_pointer` at the time of writing are:

- [UnityGltf (prefrontal cortex' fork)](https://github.com/prefrontalcortex/unitygltf)
Copy link
Member

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.

Copy link
Member

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.

@UX3D-haertl
Copy link

UX3D-haertl commented Aug 20, 2024

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.
It feels a bit strange that one can not animate weights if they are not initialized to 0 beforehand but if you follow the extension definition strictly, this is the case.

@lexaknyazev
Copy link
Member

It feels a bit strange that one can not animate weights if they are not initialized to 0 beforehand but if you follow the extension definition strictly, this is the case.

This is intentional. The size of the weights array depends on the number of morph targets and therefore the property cannot have a standalone default value.

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.

PR support KHR_animation_pointer
9 participants