glTF-Draco JOINT attributes decoded as float, should be int

[donmccurdy]

From FrankGalligan#2 —

See #145
for reference.

In RiggedFigure:
https://github.com/FrankGalligan/glTF-Sample-Models/tree/be470ea72ecb7adf2b30932349d1b12164af07b7/2.0/RiggedFigure/glTF-Draco

The draco decoded data is returning floats, when original data is unsigned short.

The draco gltf says the JOINTS_0 data should be unsigned int:

https://github.com/FrankGalligan/glTF-Sample-Models/blob/be470ea72ecb7adf2b30932349d1b12164af07b7/2.0/RiggedFigure/glTF-Draco/RiggedFigure.gltf#L2481

[FrankGalligan]

output_RF_comp_int.zip
@donmccurdy Can you try this model in your viewer? The compressed JOINTS_0 attribute is set to componentType 5123.

[donmccurdy]

Looks good on https://gltf-viewer.donmccurdy.com/ (Draco support is now on by default) 👍

[FrankGalligan]

@lilleyse Fixed this here: CesiumGS/gltf-pipeline@5bcd878

I think this can be closed now.

[najadojo]

@FrankGalligan in the output_RF_comp_int.zip you attached above the POSITION accessor (80) has a count == 370 but Draco decoding is telling me that num_points() == 367. Why are these different? And (more of a spec question) if this is allowed why have an accessor at all for this attribute if it doesn't provide accurate data?

[donmccurdy]

From the current spec I would certainly assume count is expected to be correct (and consistent) for both compressed and uncompressed (if fallback is used) attributes. Loaders must also rely on the accessor definition for reliable type information: the Draco decoder does not know the type of each attribute from the bufferView alone, as far as I can tell.

If any of that is incorrect, the spec should likely be clarified.

[FrankGalligan]

@donmccurdy so the count can be different for compressed and uncompressed meshes. But the count for compressed meshes we can get from the Draco decompressed mesh. So I'm proposing making this change to the spec:
KhronosGroup/glTF#1343

I also made a change to gltf-pipeline to output compressed and uncompressed fallback meshes here:
https://github.com/FrankGalligan/gltf-pipeline/tree/add_uncompressed_fallback

[donmccurdy]

I think the extension spec should be explicit about each of the accessor properties, in that case — if count and bufferView are to be ignored, what about min and max? Currently THREE.GLTFLoader relies on componentType to decode attributes as the correct type, but is that information also in the Draco bufferview somewhere, or would it ever change with encoding?

[najadojo]

Draco does have its own metadata that provides that information in its bufferView. It seems during compression the bitstream can change the type of the vertex buffer as an option to the pipeline.

I suggest we make clear in the extension that count, min, max, componentType and type are all correct in the accessor and match the data that draco would report from its bufferview decoding.

[lexaknyazev]

Regarding min/max. The main spec has a note that when accessor.bufferView is undefined min/max still represent correct bounds of data.

I would agree with @najadojo that when fallback is present it should be filled with decompressed data instead of uncompressed data. In that case all properties will match in both versions.

[donmccurdy]

... when fallback is present it should be filled with decompressed data instead of uncompressed data. In that case all properties will match in both versions.

Oh, I'd missed that distinction between decompressed and uncompressed. Yes, having count be correct for both versions seems preferable.

Draco does have its own metadata that provides that information in its bufferView. It seems during compression the bitstream can change the type of the vertex buffer as an option to the pipeline.

Hm, assuming most users of the Draco extension are going to want to just use the encoder/decoder tools and not read the bitstream spec, where would they find that information? I'm not aware of API docs or an example that would give that info. Using THREE.DRACOLoader as a reference, I would assume the user of the decoder must know attribute types from an external source.

[lexaknyazev]

where would they find that information

Assuming that glTF properties are correct, loaders don't need to care about bitstream. Otherwise, they should be able to get all metadata from the decoder API (see IDL).

[donmccurdy]

Thanks — I see the attribute.data_type() method in that IDL file, but wasn't aware of it before. I'm still not sure what to do with its output: I can't find the enum in the IDL file or in the generated JS to compare against, or any examples of doing so, only here in the C++.

Up until quite recently three.js assumed all draco attributes were float32 — we now rely on the glTF accessor componentType. It appears that BabylonJS still assumes float32.

[lexaknyazev]

I can't find the enum in the IDL file

Here they are:
https://github.com/google/draco/blob/master/src/draco/core/draco_types.h

[najadojo]

@donmccurdy the method GetAttributeFloatForAllPoints converts each value to a float as it copies the decoded data to the JS array, its safe but inefficient. I believe all BabylonJS vertex attributes are Float32 when importing glTF so it always pays this cost.

In our C++ engine I was hoping to use the accessor metadata to create the vertex data buffer first and then copy it at once via PointAttribute::buffer(). I could still do this but I'd need to refactor our loader such that the buffer creation is done later after we've determined if we are using Draco or not.

[najadojo]

Actually looking closer even if I had a guarantee from the accessor I'd still have to copy value by value because of the Point indices map.

[lexaknyazev]

even if I had a guarantee from the accessor I'd still have to copy value by value

Does it mean that you won't care about accessor.count anyway? I'd still prefer them to be correct mainly because of VAO / command buffer predictability.

[najadojo]

No I still care very much. The whether or not the points are mapped or can be copied at once I still want to build our vertex buffers first. Doing that one way is best.

[FrankGalligan]

So we can change the spec to say that the glTF accessor properties must match the decompressed data.

E.g. in RiggedFigure then we would put "367" in the "accessor.count" For both cases where there was uncompressed fallback and when there wasn't.

What about using the uncompressed fallback which has 370? Is this fine for loaders?

[najadojo]

No it wouldn't be if the fallback didn't contain 370 elements it wouldn't load. The compression pipeline should replace the input data with uncompressed data in the output so that the values would match.

[FrankGalligan]

Ahh got it. Yes that should work. I'm fine with that. Sound good to everyone else?

[bghgary]

It appears that BabylonJS still assumes float32.

Yes, this is true. We can update it once the issues have converged. I've created a BabylonJS issue for this.

[FrankGalligan]

I changed the PR for the spec to try and capture what we have been talking about. KhronosGroup/glTF#1343

Any comments? Should I add more detail?

[FrankGalligan]

Currently I have 2 PRs related to this and gltf-pipeline. Once the text lands in the extension spec, I will start making PRs to gltf-pipeline main repo.

Replace accessor properties with decompressed properties:
https://github.com/FrankGalligan/gltf-pipeline/tree/replace_original_with_decompressed

Add support for decompressed data fallback:
https://github.com/FrankGalligan/gltf-pipeline/tree/rebase_uncompressed_fallback

[FrankGalligan]

I think this was fixed a while ago. Can we close?