Implement generalized float accessor data decoding

[zeux]

This change introduces GLTFResourceReader::ReadFloatData that, in
contrast with ReadBinaryData, doesn't require knowledge of the specific
data type and instead decodes the data into floating-point values in
accordance with ES3 rules that are used by glTF specification for
animation and geometry data.

This makes the code much more concise, because there's no need to
enumerate individual types and implement custom fragile conversions, and
trivially makes the code compatible with additions to the geometry
encodings provided by KHR_mesh_quantization extension.

Note that I had to fix the animation utils tests that incorrectly created an
unnormalized accessor and relied on animation decoding to always normalize
the data.

Fixes #48.

[bghgary]

It would be good to get input from @agrittmsft which may be a while due to holidays.

[agrittmsft]

I think it looks good in principle? I'm a bit rusty as I haven't looked at this code in 6 months.

I think you should leave in the checks which are restricting certain types of data, it looks like in its current iteration it would allow any componentType for certain types of data even when they're not allowed by the spec.

Also it looks like the current iteration is failing the CMake build https://gltfsdk.visualstudio.com/build/_build/results?buildId=135&view=results

[zeux]

I think you should leave in the checks which are restricting certain types of data, it looks like in its current iteration it would allow any componentType for certain types of data even when they're not allowed by the spec.

Can you explain why this is necessary? Adding these checks back will prevent KHR_mesh_quantization from working so it will need explicit support, and right now there is no complete validation in terms of accessor setup (e.g. see normalized behavior in animation accessors that this change fixes).

The CMake build you link has been fixed in 8c18d63. I don't know whether Azure Pipelines require a manual build trigger after this fix, or how to do this.

[chriche-ms]

I know this code was copy-pasted but can you use std::numeric_limits instead of hardcoding the literal values

[zeux]

Isn't hardcoding the literal values correct? These have to be exactly as written in the source code to follow the spec.

[chriche-ms]

The source code can be whatever we want it to be as long as it is equivalent to the specified 'equations'. Aside from the ridiculous verbosity std::numeric_limits<uint16_t>::max() is at least slightly self-documenting

[agrittmsft]

I originally wrote it like this to match the spec (attached). I would leave it as-is.

[chriche-ms]

Overall I think these are good changes and worth making even disregarding that they are required to support your KHR_mesh_quantization extension.

However, I would like the new functionality to be opt-in rather than the default behaviour as clients of the SDK should be explicitly supporting the quantized geometry extension rather than it being a side-effect of upgrading to the latest SDK code. Also, some component types supported by the core spec for certain attributes (e.g. colors with unsigned byte/short data) are now converted to floats and back which isn't ideal. In these particular cases I think the old code path should be reinstated.

Finally, there were a few minor issues for which I have added comments to the PR. Also, some of the changes used tabs, rather than spaces, for whitespace.

[zeux]

I would like the new functionality to be opt-in rather than the default behaviour as clients of the SDK should be explicitly supporting the quantized geometry extension rather than it being a side-effect of upgrading to the latest SDK code.

I feel like this runs counter to the purpose of this change. Is there a reason why you would like the clients to opt in? Note that at the interface level, this change is fully compatible with existing clients.

Also, some component types supported by the core spec for certain attributes (e.g. colors with unsigned byte/short data) are now converted to floats and back which isn't ideal.

Is this specifically for cases where the component types match the output type? Are you worried about performance, exactness, anything else? In my opinion the code along the lines of PackColorsRGBA that did uint16->uint8 repacking is just fragile and unnecessary. This change is a massive simplification to the implementation, so I'd like to restore the absolute minimum of removed code required.

Also, some of the changes used tabs, rather than spaces, for whitespace.

It would be nice if there was a .clang-format file here... I converted the new .cpp file to spaces, I'll check to see if any others are left.

[zeux]

If there's discomfort around unconditional support of the new types, I would propose the following change:

1. I'll add a set of internal Validate functions that will be called internally from MeshPrimitiveUtils
2. These functions will check the accessor type and component type vs the core spec
3. These functions will also check if document.IsExtensionRequired("KHR_mesh_quantization") and, if it's required, relax the checks according to the extension

This would mean that files that require the extension are transparently supported, but files that don't are still validated vs the spec.

Additionally I'm open to adding explicit fast-paths for no-op conversions for colors and joint weights if this seems necessary, that bypass the float de/encoding. I would prefer to not add this to any path that isn't a no-op conversion though since that doesn't seem valuable.

[chriche-ms]

I feel like this runs counter to the purpose of this change. Is there a reason why you would like the clients to opt in? Note that at the interface level, this change is fully compatible with existing clients.

Yes, we have explicitly designed the SDK to only support the core spec unless client code explicitly specifies otherwise. This is why support for the other KHR extensions is opt-in as well.

In my opinion the code along the lines of PackColorsRGBA that did uint16->uint8 repacking is just fragile and unnecessary.

I looked a little closer at the existing uint16_t -> uint8_t code and noticed that it (unsurprisingly) also converts to float and back. So I guess using ReadFloatData would also be ok for this conversion. However, converting uint8_t -> uint32_t only requires bit shifting so I still want to use the existing code for this conversion.

These functions will also check if document.IsExtensionRequired("KHR_mesh_quantization") and, if it's required, relax the checks according to the extension

I wouldn't make this change right away. I don't think it's the best approach (due to the repeated map lookups and the inability to override if necessary) and I'd also like to hear the thoughts of some other devs on whether support should be opt-in or not.

[zeux]

I've updated the PR to unify ComponentToFloat definitions and to restore the 8->32 packing for bone weights and colors.

As for opt-in vs providing this support by default, I don't really understand the reasoning for the clients to opt in into extension support in general - and also I'm not sure that it works like this right now for other supported extensions, my understanding is that the recommended path includes using GetKHRExtensionDeserializer() without opting in into each specific extension?

For some other extensions I could see the argument that, since the client has to implement extra code to process the extension data, it doesn't truly matter whether extra opt-in is required. But KHR_mesh_quantization is explicitly designed to be minimal and trivial to support. There's no need to change the client code if the reader can decode the data as all quantization transforms are using established glTF mechanisms such as node transforms. So in this case, requiring opt-in for this just leads to extra friction and I don't see what the benefits are.

Wrt error handling, I discovered that Validation namespace has a few validation functions that could be called to validate the data; some of them, like ValidateAccessor, are called by GLTF reader. So it seems to me that the best route is to either omit extra checks in the reading functions and expect Validate* functions to be called manually (I'll need to teach them about KHR_mesh_quantization but this can be a separate PR), or add Validate* functions for geometry accessors that are called from MeshPrimitiveUtils functions. Let me know if this makes sense and what the preference is.

[agrittmsft]

I agree with @chriche-ms that the new extension/behavior should be opt-in. If we do anything differently then it will make this a pain to integrate. This is also the way we have implemented other breaking changes.

[zeux]

Can you please elaborate what does "a pain to integrate" mean? I'm not sure why this change is considered breaking. As far as I can tell, other extensions aren't integrated in a piece-wise opt-in manner either, instead relying on bulk GetKHRExtensionDeserializer.

[agrittmsft]

Well prior to this PR, various methods would fail if the componentType was not in the list described in the core spec. For example std::vector MeshPrimitiveUtils::GetTexCoords will now allow COMPONENT_BYTE where it previously did not.

If we allow this PR as-is, it may break unit tests or even actual functionality (hopefully not) in our other codebases that were relying on this behavior. In these cases the integration would be non-trivial, this could become problematic for some products. @bghgary would be the best person to comment.

[zeux]

I don't understand why "a file that previously could not be loaded can now be loaded" is a breaking change.

Above I outlined one potential way forward, which is that the validation in the accessors is going to be conditional on the glTF file requiring KHR_mesh_quantization. If even this is not acceptable, I don't understand how this SDK can get support for KHR_mesh_quantization in the first place.

[bghgary]

Disclaimer: I have only briefly scanned the changes in this PR.

I don't understand why "a file that previously could not be loaded can now be loaded" is a breaking change.

Generally speaking, loosening the inputs of an API can cause tests to fail (e.g. negative tests that ensure invalid input is rejected). From an client implementation perspective, this change will allow certain glTF assets that used to be rejected, which then can cause issues down stream. I don't think all client implementations will necessarily support all transform types automatically. glTF-SDK has been rather strict in the past in what it accepts and tries to limit to the core spec as much as possible. Thus any changes to this is a breaking change.

Maybe you didn't mean this statement generally and mean it specific to the extensions required mechanism you proposed. If that's the case, then see my comment below.

opt-in

Assuming I understood everything properly, I think what @zeux is proposing by checking the extension required makes sense. If the KHR_mesh_quantization extension is required in a glTF, it is guaranteed that the asset is supposed to support the extra types. It should have no effect on glTF files that don't have this extension and should not break any existing tests and is not a breaking change.

Does the changes in this PR already do this?

[zeux]

Does the changes in this PR already do this?

No, this isn't implemented yet, as I'm waiting to resolve the discussion to pick the implementation strategy that can get approved.

[zeux]

Specifically, my proposal to this effect was in #51 (comment) but it didn't sound like there's agreement that this is the appropriate route.

[agrittmsft]

I guess there is a bit of ambiguity around "opt-in", I think @chriche-ms means that the opt-in should be through some code change on the part of the caller of the glTF-SDK, and @zeux means that the opt-in should be through the glTF file having "KHR_mesh_quantization" in extensionsRequired.

@zeux would you consider adding back in the componentType checks for now, so that we can get this PR completed? We could then have a separate PR to figure out the "opt-in" on how to get the KHR_mesh_quantization extension to work with MeshPrimitiveUtils.

[zeux]

Ok - let's do that. I'd like to put these checks into Validation namespace next to existing Validation functions, would that work or would you prefer adding these directly to MeshPrimitiveUtils members?

[zeux]

(note that this would mean that I will need to disable the tests that test the new functionality - I will keep the tests themselves in the source tree if that's all right, with a comment)

[bghgary]

opt-in should be through some code change on the part of the caller of the glTF-SDK

The point of having the extension is to loosen this requirement. I think it's okay if we give the user the option to support or not support specific extensions, but I don't think it makes sense to have a flag that specifically calls out whether extra types are supported.

[zeux]

To move this change forward, I've restored the original validity checks. They are insufficient for validating core files (they don't check normalized bit) and they are too restrictive for KHR_mesh_quantization, but my proposal would be to merge this PR - since it now adds functionality to the reader and cleans up the mesh parsing code without introducing new behaviors - and separately tackle the validation part. I can open a separate issue with the proposed solution.

[bghgary]

@zeux Sorry for the slow movement here. We had an internal discussion about this earlier and will post some suggestions (perhaps in a separate issue). But I agree we can merge this now and move forward. @agrittmsft @chriche-ms What do you think?

[chriche-ms]

I'm happy with these changes - discussing the rest of the changes required for the extension separately is a good idea!