KhronosGroup / glTF-CSharp-Loader

C# Reference Loader for glTF
Other
213 stars 60 forks source link

Disable Accessor.Count and BufferView.ByteStride validation #32

Open rickomax opened 3 years ago

rickomax commented 3 years ago

Some models exported from Google Poly have ´Accessor.Count´ and ´BufferView.ByteStride´ values set to 0 (zero). Even tho, majority of GLTF 2 viewers are able to load these files, while C# loader throws an error.

It is easy to compute the final stride when these values aren't given by calculating it using the ´accessor.ComponentType´ and ´accessor.Type´ fields.

Can we make these validations optional or remove them at all?

There is a model exported from Google Poly attached. archive (3).zip

bghgary commented 3 years ago

Some models exported from Google Poly have ´Accessor.Count´ and ´BufferView.ByteStride´ values set to 0 (zero).

Setting either of these properties to zero is not allowed per glTF specification. https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/accessor.schema.json#L68-L73 https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/bufferView.schema.json#L23-L31

Both will give glTF validation errors.

Can we make these validations optional or remove them at all?

Maybe. I'm not sure it's a good idea and I'm not sure how easy it will be.

Google Poly

Ideally, Google Poly should not be producing a glTF with these errors. @donmccurdy Do you have any contacts?

rickomax commented 3 years ago

Not every model exported from Poly have these issues, many I found that has these issues seem to be made in Google Blocks. This one produces errors: https://poly.google.com/view/2PDe5PSncTC Curiously, @donmccurdy viewer can load it just fine, but it points out the errors: https://gltf-viewer.donmccurdy.com/ So maybe disabling the validation for these items gives the user the chance to implement a loader for them anyway. That is what I did, using the fields I mention in the first post here.

bghgary commented 3 years ago

Don's viewer runs the glTF-Validator like other web implementations. This project generates code from the schema. It's possible add code to disable the validation, but I'm not sure it's the right thing to do. If you really want, feel free to propose the change.

vpenades commented 3 years ago

Regarding issues like this, where a mainstream exporter is producing invalid glTFs, I had to reach a compromise:

When loading a model in my library, you can pass an option to completely skip validation on loading... but validaton is enabled by default. So when someone loads a glTF and validation fails, at least they have an option to try to load the model as it is, at their own responsability. by manually disabling the validation.

And Google Poly is not the only one... Sketchfab automatic glTF exporter is full of invalid stuff: invalid bounds, plenty of NaNs, incorrect use of accessors... I reported the issues to sketchfab with little success, but I kept receiving so many complaints that in the end I had to do something on my side.

The risk of letting invalid glTFs to load, is that it hurts the whole ecosystem, as exporters producing invalid assets will have less motivation to fix their own stuff if people stops complaining to them.

You guys at khronos should have some kind of "glTF seal of approval" to be delivered only to those parties that are able to import/export correctly.

rickomax commented 3 years ago

@vpenades this is why 90% of animations I download from Sketchfab look wrong on my application while all the samples from the glTF test suite work perfectly.

donmccurdy commented 3 years ago

It's possible add code to disable the validation, but I'm not sure it's the right thing to do.

@bghgary I'm not a user of the C# loader, but could I ask why you're concerned about this loader having the option to disable validation? Surely many applications (e.g. a game) won't want to spend time validating all of their assets at runtime in production — or have I misunderstood the request here?

I certainly see the value in making validation errors prominently visible. I'd also like to do better about making it clear to users how and where to report those issues: that was part of the motivation for creating glTF-Generator-Registry, so that viewers can display links showing where to report these bugs. With any luck, that puts a bit more pressure on authoring tools and libraries to resolve these issues. But, we haven't really pushed the use of the registry much.

Whether that's a better or worse approach than @vpenades' suggestion, I don't know. I think we'd welcome suggestions of this sort on the main glTF repository. :)

Unfortunately, I'm not confident I can do much about the errors from Google Poly...

this is why 90% of animations I download from Sketchfab look wrong on my application while all the samples from the glTF test suite work perfectly.

@rickomax Hm, the large majority of animations I download from Sketchfab do include validation warnings, but still render correctly, at least in three.js and babylon.js, and we don't make any attempt to correct the original animation data before rendering. You may want to ensure that some of the more advanced skinning tests also work in your application — see https://github.com/KhronosGroup/glTF-Asset-Generator/#positive-tests.

bghgary commented 3 years ago

@bghgary I'm not a user of the C# loader, but could I ask why you're concerned about this loader having the option to disable validation? Surely many applications (e.g. a game) won't want to spend time validating all of their assets at runtime in production — or have I misunderstood the request here?

I'm not concerned about adding a flag to disable it for the right reasons. The generated validation is a very simple translation of some of the requirements in the schema. They are not expensive to check. (Note that I didn't write this code.) I'm more worried about the ecosystem having lots of invalid models as @vpenades pointed out.

rickomax commented 3 years ago

I have to agree with @bghgary , but on a commercial project, for instance, the end-user will surely complain as other systems are allowed to load such models. It is a shame that not everyone follows the schema strictly...

vpenades commented 3 years ago

@bghgary regarding improving the ecosystem, I've submited this proposal , maybe it can be useful to glTF-CSharp-Loader and the gltf validator itself