Invalid byte stride computed for certain assets

[javagl]

According to the specification, the byteStride must always be >=4. This does not match the result of getAccessorByteStride in addDefaults.

I think that the following test, to be added in addDefaultsSpec, should pass:

it('adds no invalid bufferView byteStride defaults', () => {
    const gltf = {
        meshes: [
            {
                primitives: [
                    {
                        attributes: {
                            POSITION: 0,
                            _BATCHID: 1
                        },
                        indices: 2,
                    }
                ]
            }
        ],
        accessors: [
            {
                bufferView: 0,
                componentType: WebGLConstants.FLOAT,
                count: 24,
                type: 'VEC3',
                min: [-1.0, -1.0, -1.0],
                max: [1.0, 1.0, 1.0]
            },
            {
                bufferView: 1,
                componentType: WebGLConstants.UNSIGNED_SHORT,
                count: 24,
                type: 'SCALAR',
                min: [0],
                max: [24]
            },
            {
                bufferView: 2,
                componentType: WebGLConstants.UNSIGNED_SHORT,
                count: 36,
                type: 'SCALAR',
                min: [0],
                max: [24]
            }
        ],
        bufferViews: [
            {
                buffer: 0,
                byteLength: 288
            },
            {
                buffer: 0,
                byteLength: 48,
                byteOffset: 288
            },
            {
                buffer: 0,
                byteLength: 72,
                byteOffset: 336
            }
        ]
    };

    const gltfWithDefaults = addDefaults(gltf);

    const positionAccessor = gltfWithDefaults.accessors[0];
    const batchIdAccessor = gltfWithDefaults.accessors[1];
    const indicesAccessor = gltfWithDefaults.accessors[2];

    const positionBufferView = gltfWithDefaults.bufferViews[0];
    const batchIdBufferView = gltfWithDefaults.bufferViews[1];
    const indicesBufferView = gltfWithDefaults.bufferViews[2];
    
    expect(positionAccessor.byteOffset).toBe(0);
    expect(positionAccessor.normalized).toBe(false);
    expect(positionBufferView.byteOffset).toBe(0);
    expect(positionBufferView.byteStride).toBe(12);
    expect(positionBufferView.target).toBe(WebGLConstants.ARRAY_BUFFER);

    expect(batchIdAccessor.byteOffset).toBe(0);
    expect(batchIdAccessor.normalized).toBe(false);
    expect(batchIdBufferView.byteStride).toBe(4);

    expect(indicesAccessor.byteOffset).toBe(0);
    expect(indicesAccessor.normalized).toBeUndefined();
    expect(indicesBufferView.byteStride).toBeUndefined();
    expect(indicesBufferView.target).toBe(WebGLConstants.ELEMENT_ARRAY_BUFFER);
});

with the critcial line being

    expect(batchIdBufferView.byteStride).toBe(4);

Can someone confirm this?

(Of course, just changing the function to return Math.max(4, originalResult) causes everything else to break down - if this really is an issue, fixing it might not be trivial, so I wanted to ask someone to confirm before sinking more time into that...)

[lilleyse]

Can confirm this is a problem, not a problem with getAccessorByteStride but a problem with the data.

Some older 3D Tilesets used UNSIGNED_SHORT batch ids before glTF had a 4-byte requirement for byte stride. For glTF 1.0 models this could be fixed in updateVersion by creating a new buffer view and adding padding between batch id elements.

Or maybe the issue can be fixed in https://github.com/CesiumGS/3d-tiles-validator/blob/2.0-tools/tools/lib/upgradeTileset.js by converting batch ids from UNSIGNED_SHORT to FLOAT.

[javagl]

That's another dimension of the difficulty.

On the one hand, it's good to know that upgradeTileset should be able to make such an asset valid, but ... I wonder how: I originally encountered this invalid byte stride exactly in an asset that was generated with upgradeTileset.

Further fragments of my mental model:

• The 3d tiles spec does not explicitly name any type requirements for the _BATCHID, although the given example uses 5126 (FLOAT).
• I think that purely from a glTF perspective, the type could be UNSIGNED_SHORT, even though it would require padding, as in XX..XX..XX...
• It's very likely that an asset where the _BATCHID is UNSIGNED_SHORT does not introduce such a padding, and just stores the elements tightly packed. But admittedly, I haven't looked into that for the asset that sparked this issue.

---

Updated: I checked the input asset as well, and the validator says

 "code": "MESH_PRIMITIVE_ACCESSOR_UNALIGNED",
 "message": "Vertex attribute data must be aligned to 4-byte boundaries.",

So it already contains an error, which, via the upgradeTileset, apparently "propagates" and shows up as a different error.

So we could call this a case of GIGO, unless there's a trick to fix these batch IDs, as you mentioned...