Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Add MorphPrimitivesTest. #200

Merged
merged 6 commits into from
Oct 31, 2018

Conversation

donmccurdy
Copy link
Contributor

Sample model provided by @ft-lab in mrdoob/three.js#15039.

@ft-lab could you confirm that you are OK with putting a CC-BY license (https://creativecommons.org/licenses/by/4.0/) on this model, for others to use and test? Thanks!

@ft-lab
Copy link

ft-lab commented Oct 20, 2018

Thank you very much for the sample glb commits!
I've confirmed.
Yes, CC-BY 4.0. I welcome others to use.

@cx20
Copy link
Contributor

cx20 commented Oct 24, 2018

I tried this model with gltf-test. For now it seems that only Babylon.js can display correctly.
https://github.com/cx20/gltf-test#extension-test-models
image

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Oct 24, 2018

Thanks @cx20, good to have a test case then. 😅

We'll have this fixed in three.js soon: mrdoob/three.js#15084

@emackey
Copy link
Member

emackey commented Oct 25, 2018

Can you add a plain glTF flavor of this, in addition to the GLBs? I think all of our other samples include at least a plain glTF version, and that helps users inspect the contents of each sample.

@donmccurdy
Copy link
Contributor Author

Done ✅

@emackey
Copy link
Member

emackey commented Oct 26, 2018

Forgive me if I'm just uninformed on morph targets, but, what's the goal here? Should implementations find the one target and just apply it by default? What if there's more than one? Should there be some user- or API-control over when the morph is applied?

Looks like @cx20 marked an X for implementations that aren't immediately applying the morph right out of the box. Is this the expected behavior? If so, what's the advantage of having the non-morphed data present in the glTF?

@donmccurdy
Copy link
Contributor Author

The mesh contains a mesh.weights property of [0.5], so according to spec an implementation is required to apply morph targets to both primitives at 50% weight:

A Morph Target may also define an optional mesh.weights property that stores the default targets weights. In the absence of a node.weights property, the primitives attributes are resolved using these weights.

@bghgary
Copy link
Contributor

bghgary commented Oct 26, 2018

If so, what's the advantage of having the non-morphed data present in the glTF?

Morphed data in glTF are displacement data (i.e. they are deltas from the original mesh) and thus you must have the original data.

@emackey
Copy link
Member

emackey commented Oct 26, 2018

The mesh contains a mesh.weights property of [0.5]

Ah, that was the missing piece for me, thanks.

As you guys know I've been lobbying for a higher standard for new models being added here. Here's where I think we should be doing better:

  • Ideally, failure patterns should be visible to users who have no knowledge of the internals of the test model. For example, a lack of application of morph targets could result in a geometric arrow pointing at an "X" instead of a checkmark, or similar. As it stands, the user must "know" that a flat plane indicates lack of morph offsets, which is not intuitive. Likewise, if the morph target is applied at 1.0 strength instead of the requested 0.5, the model should have some visual way of indicating the problem that would be apparent to casual observers.

  • The README should spell out both the intended appearance of the model, and the possible appearance for various common failure patterns, such as lack of morph target application. It could even include screenshots of what the model looks like when there's no morphing, and when the morphing is too strong, etc.

  • The README should also explain why this morph test is different from the other morph tests that are in the sample collection: Namely, this one tests mesh.weights where the others test morph animations.

If I'm asking too much, we could go ahead and merge this as-is, if you think that's best. I'm just concerned that the model is only useful by people who have taken time to understand various intentions and tech specifications, and I know it's possible to make it accessible and usable by folks with far less ecosystem knowledge.

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Oct 26, 2018

Thanks @emackey — I agree with that long-term direction for this repository, especially as glTF-Asset-Generator is better suited to cover many of these implementation edge cases. It would be nice to have glTF-Sample-Models showcase features and strengths of glTF, and keep the combinatorial low-level feature coverage to a minimum. For that matter, I'd be open to removing some of the very basic samples here (Triangle, TriangleWithoutIndices, ...) since those are trivially covered by glTF-Asset-Generator.

However... glTF-Asset-Generator doesn't currently cover Draco, morph targets, or combinations of the two. I don't know how easy it is to add new cases to glTF-Asset-Generator, but at the moment I don't think I have time to implement a generator covering this.

Given a model that identifies a bug in 12 of 13 WebGL implementations (or 6 of 7 that support morph targets), what's the right step to take to ensure it (eventually) gets fixed?

Add README details for MorphPrimitivesTest.
@donmccurdy
Copy link
Contributor Author

Thanks for the README update @emackey! I guess there may be a longer conversation to have about how we want to make the most of this repo, in general, but I think we can track that and update things through another issue.

@emackey emackey merged commit 9fc5079 into KhronosGroup:master Oct 31, 2018
@donmccurdy donmccurdy deleted the sample-morphprimitivestest branch October 31, 2018 19:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants