KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.2k stars 1.14k forks source link

Clarify mixing morphed and non-morphed primitives #2154

Open lexaknyazev opened 2 years ago

lexaknyazev commented 2 years ago

Originally reported by @scurest in https://github.com/KhronosGroup/glTF-Validator/issues/182.

The spec says:

All primitives MUST have the same number of morph targets in the same order.

Given that glTF schemas do not allow empty arrays, it is a bit unclear whether a mesh primitive with an undefined targets array has zero or undefined number of morph targets. Therefore the spec is not clear whether it is allowed to mix morphed and non-morphed primitives within a single mesh.

/cc @emackey @javagl @bghgary

lexaknyazev commented 2 years ago

When given a mesh with mixed morphed and non-morphed entries in mesh.primitives, glTF-Sample-Viewer, three.js, and PlayCanvas seem to proceed as usual. Babylon yields an error.

@bghgary WDYT?

emackey commented 2 years ago

glTF-Sample-Viewer, three.js, and PlayCanvas seem to proceed as usual.

I'm not sure this is a complete test, because the morph target doesn't have an assigned animation. Would we really see animated morphing in all of these different engines with such a model?

it is a bit unclear whether a mesh primitive with an undefined targets array has zero or undefined number of morph targets.

I think it's a stretch to say that a primitive has undefined number of morph targets, and that's the reason it's considered to have "the same number of morph targets in the same order" as another primitive with a concrete list.

emackey commented 2 years ago

Here's another morph test: AnotherMorphTest.zip

This is a yellow cube (primitive 1) on a brick-colored base (primitive 0). The cube has an animated morph target that makes it taller and more "pointy." The base has a morph target that does nothing, so this has been "commented out" (illegally) by renaming targets to XX_targets in the file.

The validator now says "Animation channel cannot target WEIGHTS when mesh does not have morph targets" based on that first primitive. The file appears to animate correctly in Cesium and ThreeJS, but fails in Babylon and Filament.

lexaknyazev commented 2 years ago

Would we really see animated morphing in all of these different engines with such a model?

Yes, I tested that on a model with an animation targeting morph weights.

The file appears to animate correctly in Cesium and ThreeJS, but fails in Babylon and Filament.

Thanks for more data points.

I think it's a stretch to say that a primitive has undefined number of morph targets, and that's the reason it's considered to have "the same number of morph targets in the same order" as another primitive with a concrete list.

Not "the same number" but rather "not comparable number" or "not a number", think 0 vs null vs NaN. For conformance purposes, we need to either exclude non-morphed primitives from this condition or to consider them having zero morph targets (despite the schema not allowing empty arrays).

emackey commented 2 years ago

Not "the same number" but rather "not comparable number" or "not a number", think 0 vs null vs NaN.

For what it's worth, my vote is to treat missing targets as 0 targets, disallowing this type of model. I think this is healthier for an ecosystem where at least some of the existing or legacy implementations have historically disallowed this kind of model.

lexaknyazev commented 2 years ago

As we have already two engines not supporting such models, let's proceed with treating non-morphed primitives as having zero morph targets (and asserting all conformance implications of that).

bghgary commented 2 years ago

I can very easily fix this in Babylon.js with an if check. I will probably fix this regardless of this decision. chrome_2022-05-03_14-55-17

ptasev commented 2 years ago

If my understanding of the spec is correct, then the way to handle this would be to have an empty json object, with no attributes present in the morph target, to signal that there are no changes from the base mesh. The first part highlighted in this image is what leads me to this understanding. image

Unfortunately the glTF validator flags this as invalid, so this sentence in the spec doesn't make sense.

Note that with some of the suggested changes above, it wouldn't account for an example like the following where a mesh has 3 targets, and 2 primitives: Prim1 (only makes changes in the 2nd target):

"targets": [ { }, { "POSITION": 0 }, { } ]

Prim2 (makes changes in all targets):

"targets": [ { "POSITION": 1 }, { "POSITION": 2 }, { "POSITION": 3 } ]
vpenades commented 2 years ago

So, could it be possible to get an example of how to correctly create a morphed mesh with two primitives, where one of the primitives remains unaltered?

scurest commented 2 years ago

The JSON schema requiring the objects in targets be non-empty seems like an oversight to me, perhaps it can be changed. (An ugly workaround is obviously to use a "dummy" all-zero attribute (but see #2169)).

But this is drifting off-topic since it affects even meshes whose primitive targets are all arrays with the same length.

vpenades commented 2 years ago

@scurest yes, both this issue and #2169 are related.

We've tried to set an empty object as @ptasev suggested, like this:

"targets": [ { }, { "POSITION": 0 }, { } ]

But gltf validator reports it as invalid due to empty entities. Also, it would require to hack this exception in some json loaders that have that rule in-built.

Creating an Accessor with no bufferview as discussed in #2169 would imply that the expected buffer is NOT undefined, but assumed to be filled by zeros by the loader runtime.

So... I guess the only solution right now, unless a better one is provided, is to create dummy Accessors filled with zeros.