Fixed sparse accessor validation

[javagl]

For the sparse accessor sample that I created for KhronosGroup/glTF-Sample-Models#99 (comment) , the validator reported an error:

       "ACCESSOR_MAX_MISMATCH": [
            {
                "type": "ACCESSOR_MAX_MISMATCH",
                "path": "#/accessors/1/max",
                "message": "Declared maximum value for component `1` (`4`) does not match actual one (`3`)."
            }
        ]

According to my understanding, the values that are stored in the sparse accessors are really displacements. They are not supposed to replace the original values. Instead, they should be added to the original values.

(Maybe the spec could be reworded slightly. It refers to this process as "substitution", which sounds like the values should be replaced. Referring to it as "displacement" or "deformation" might be less confusing. I also stumbled over this, and fixed it in javagl/JglTF@6eac4bc )

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[lexaknyazev]

According to my understanding, the values that are stored in the sparse accessors are really displacements. They are not supposed to replace the original values. Instead, they should be added to the original values.

Where does this come from? Here's an initial discussion:
KhronosGroup/glTF#820 (comment)

[javagl]

Maybe it's a basic misunderstanding on my side. As you can see, I originally interpreted it in the way that you now referred to it. In hindsight, cannot say for sure where this confusion came from. But I remember that my interpretation of storing displacements originally came up when creating the "Morph Targets" section of the glTFOverview. And I think that it stems from this part of the spec: https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#meshes

It says (emphasis by me)

A Morph Target is a morphable Mesh where primitives' attributes are obtained by adding the original attributes to a weighted sum of targets attributes.

For instance, the Morph Target vertices POSITION for the primitive at index i are computed in this way:

 primitives[i].attributes.POSITION + 
     weights[0] * primitives[i].targets[0].POSITION +
     weights[1] * primitives[i].targets[1].POSITION +
     weights[2] * primitives[i].targets[2].POSITION + ...

If the targets store new positions, this does not make sense. Having an original mesh containing a single vertex at (1,0,0) and combining it, with a weight of 1.0, with single a target that also stores the position of (1,0,0) would yield the position (2,0,0). A result of (2,0,0) would only be correct if the target stored a displacement.

All this is somehow related to the NORMALIZED vs. RELATIVE discussion about the morph targets. The COLLADA spec (1.5) says in the "morph" section:

There are different methods available to combine morph targets; the two common methods are:

• NORMALIZED
  • (Target1, Target2, ...)*(w1, w2, ...) = (1-w1-w2-...)*BaseMesh + w1*Target1 + w2*Target2 + ...
• RELATIVE
  • (Target1, Target2, ...) + (w1, w2, ...) = BaseMesh + w1*Target1 + w2*Target2 + ...

and the conclusion in #210 seemed to be that only RELATIVE should be used, which matches the code snippet from the spec, but IMHO implies that the targets to not store positions, but only displacements.

---

So to summarize, I think that storing real positions (instead of displacements) in sparse accessors does not make sense together with the RELATIVE computation scheme. I don't have a strong opinion about

• storing real positions and using NORMALIZED morphs vs.
• storing displacements and using RELATIVE morphs

but think that the current state is at least confusing (if not plainly wrong - but if there is a basic misunderstanding on my side, please let me know).

---

(EDIT: Obviously, this would rather belong into the glTF repo, and not the validator repo, but maybe we can figure out here whether it is an issue at all, or just caused by me not properly reading the spec...)

[lexaknyazev]

I can see now how could relations between morph calculations and sparse storage be confusing.
First of all, morph targets don't have to use sparse accessors, especially in cases like cube-to-sphere transformations (i.e., when almost all vertices are affected).

If the targets store new positions, this does not make sense.

Only when using initial positions as base accessor. I'd expect that sparse accessors that are used for morph targets will have undefined accessor.bufferView.

One could argue that it would be cleaner to use accessor with initial positions as "base" for sparse accessors and apply NORMALIZED weighted sum, but this approach seems to require a bit more runtime processing.

[javagl]

Yes, in the meantime, I also realized that I mixed up two completely unrelated concepts (and was just about to update my previous comment, but you already summarized it).

The point is that

• how the accessor.sparse.values are used to create the actual accessor values and
• how multiple accessors (via morph targets) are combined to create one morphed mesh

are completely unrelated.

---

So my conclusion for now is that the morph targets should only contain displacements.

(The fact these displacements will usually (but not necessarily) be stored using sparse accessors is unrelated and independent of the fact that the accessor.sparse.values are supposed to replace the original entries. So the accessor.sparse.values really store the new values, and not displacements. This PR can be closed and ignored.)