KHR_mesh_quantization support

[zeux]

Now that #51 is merged I'd like to discuss how to add support for KHR_mesh_quantization (https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_mesh_quantization) to this SDK.

All of the code that could support reading quantized accessors is here, but we're missing good validation. Specifically:

• When KHR_mesh_quantization is set, certain attribute types are now valid to use for mesh geometry inputs
• When KHR_mesh_quantization is not set, I'd like to improve the current validation code to properly check normalized attribute.

My suggestion is as follows.

1. Change the existing accessor validation to use tables of admissible component types, e.g.

struct GeometryAccessor
{
    bool quantized;
    ComponentType type;
    bool normalized;
};

std::vector<GeometryAccessor> kPositionAccessors =
{
    { false, COMPONENT_FLOAT, false },
    { true, COMPONENT_SHORT, true }, // etc.
};

2. When loading the document, cache the presence of required KHR_mesh_quantization extension in the document field to alleviate concerns around string comparisons (alternatively we could look this up when reading accessor data)

3. Instead of current ad-hoc code, use somewhat more generic code e.g. Validation::ValidateGeometryAccessor(accessor, kPositionAccessors, doc.RequiresMeshQuantization()));

As a result, files that don't require KHR_mesh_quantization will be validated using the existing validation rules (+ a normalized check); files that require KHR_mesh_quantization will be automatically de-quantized on load.

Thoughts/suggestions welcome.

[bghgary]

Sorry again for the slow response @zeux. This sounds reasonable to me. Chatted with @agrittmsft a bit. The main concern with changes like this is that we can't break backwards compatibility. Perhaps add a flag similar to this so that SDK users have to opt-in to this new behavior? Feel free to submit a PR so we can discuss the changes further.

[zeux]

There's two questions I'd like to clarify on the backwards compatibility, and one on the way to configure this, before implementing it.

1. Right now, there are some glTF files that are invalid as per spec but are accepted. For example, the colors are always read assuming the attributes are normalized. Should fixing that have a flag? Should it be part of this change at all?

2. Right now, glTF files that require KHR_mesh_quantization and use e.g. integer position data, are valid as per spec (with a draft extension), but are not accepted. Should fixing that have a flag?

3. Assuming answer to MeshPrimitiveUtils now contains methods for serializing triangulated and segmented indices. #2 is "yes", should the flag be specified as part of the deserialization flags, or is there a way to specify them as part of the extension loader somehow? Is there an existing precedence within this library of opting into a specific extension?

The reason why I'm asking these questions is that backwards compatibility, as classically understood, refers to being able to load previously valid files, may refer to being able to load previously loadable files that aren't valid, but it's odd to see this in context of enabling the use of files that previously could not be loaded.

That, and I really don't want to write any more code for this before understanding what code is likely to be accepted...

[bghgary]

I really don't want to write any more code for this before understanding what code is likely to be accepted

I understand.

For 1 and 2, I think you can gate all of the changes in one flag. Users of the SDK will read the extension and pass this extra flag to update to a new behavior for validation. At some point, we may be able to deprecate the old way, but it will take time for products to catch up.

For 3, I don't think there is precedence on this. In any case, the deserialization flags are not for validation, so it probably makes the most sense to add a new set of flags for the validation step.

Fixing anything that changes the behavior of whether something is accepted or rejected needs to be carefully considered. For example, say a product (e.g. PowerPoint) uses this SDK to persist the resulting glTF in its file container. If those files can no longer load after updating to the latest glTF-SDK, then that will be an issue. We have to be super careful.

[zeux]

probably makes the most sense to add a new set of flags for the validation step.

The issue with that though is that right now validation for geometry attributes is actually not performed by the Validate() function - it's performed by the utility functions that read the geometry data. If that code stays there, it will continue to reject the inputs.

If those files can no longer load after updating to the latest glTF-SDK, then that will be an issue. We have to be super careful.

Yes, but that's a different dimension from 2, right? It's similar to 1 - if right now glTF-SDK accepts invalid glTF files, fixing that may break backwards compat. But 2 doesn't involve rejecting any files that are currently accepted, it's the opposite.

[zeux]

Maybe a concrete example would help illustrate my confusion around my point 2. When KHR_texture_transform support was added in #31, glTF-SDK started parsing files where that extension was required. However, this didn't require the users of the SDK opting into the support of this extension. And I don't understand why KHR_mesh_quantization is treated differently.

[bghgary]

not performed by the Validate() function

Oh right, sorry about that. I need to page this all back in. We are talking about the static utility functions. But it's the same issue. Changing the behavior of these function breaks back compat.

2 doesn't involve rejecting any files that are currently accepted, it's the opposite.

That can still cause issues for existing code. If more things are accepted than before, existing code that isn't expecting those things to come through may have problems.

why KHR_mesh_quantization is treated differently

I haven't looked at #31 super carefully, but I don't think the existing behavior changes. A change to the utility functions to accept more values has a possibility of breaking existing code.

Perhaps we can add a static flag for this, add a flag per function, or create a separate set of static functions? If we decide on the latter, I think the code can be factored so that the new set of functions reuse most of the code.

[zeux]

Ok, so a possible proposal - please correct me if I'm wrong - could be:

• Expand Validation function to validate the geometry attributes in accordance to the spec & the presence of KHR_mesh_quantization extension
• Change existing utility functions to use Validation function(s) instead of existing logic, so that the validation is centralized
• Make sure that all of these changes are gated behind some flag, possibly a static runtime flag, so that applications can independently upgrade to the new version of the SDK and enable that flag to actually observe the behavior differences

Does that sound reasonable?

And I really think that this project should have a clear guideline for adding new extensions, because this process isn't what I'd normally expect a glTF reader to follow. It's up to you as to what that guideline is, but in absence of the documented guideline the natural choice is different from what's being discussed.

That is,

I haven't looked at #31 super carefully, but I don't think the existing behavior changes. A change to the utility functions to accept more values has a possibility of breaking existing code.

Unless I am missing something, for example files with KHR_texture_transform extension that have offset with just 1 element would load just fine before this change, and would throw on load with that change. So an invalid glTF file that was previously okay to load would fail to load.

I was also initially under the impression that files with KHR_texture_transform in the extensionsRequired list would fail to load before that PR and not after that PR, but it looks like this SDK doesn't act on extensionsRequired contents so that's not a problem (although that is in stark contrast with other validation decisions made by this SDK).

[bghgary]

Does that sound reasonable?

This seems reasonable to me.

I really think that this project should have a clear guideline for adding new extensions

I agree, but this code base is in maintenance mode for the most part. There is no one actively working on this project right now. 😢

So an invalid glTF file that was previously okay to load would fail to load.

If this is true, I would think this is an oversight. We may have gotten lucky that it didn't introduce problems.

it looks like this SDK doesn't act on extensionsRequired contents so that's not a problem (although that is in stark contrast with other validation decisions made by this SDK).

That does seem odd. I can't speak to the decisions on this. I took over this project from another team.

[zeux]

Ok thanks! This is currently low on my priority list but the course of action is now clear :)

[zeux]

Going to close this since I don't have bandwidth to work on this, I'll leave it up to the maintainers of this project to prioritize extension support.