vpenades / SharpGLTF

glTF reader and writer for .NET Standard
MIT License
475 stars 75 forks source link

Should SharpGLTF throw an exception when an unsupported extension is required? #237

Open BoyBaykiller opened 4 months ago

BoyBaykiller commented 4 months ago

Currently SharpGLTF throws when an unsupported extension is required by a glTF file: https://github.com/vpenades/SharpGLTF/blob/b775097d57ad10db51d1ac2030e1117de220afe7/src/SharpGLTF.Core/Schema2/gltf.ExtensionsFactory.cs#L303

Personally I think this is a bit of an overreaction. What if the user wants to process the extension themselves. What if it's a custom extension that will never be supported by SharpGLTF. glTF-Validator doesn't care if I specify an unknown required extension.

What do you think of this?

vpenades commented 4 months ago

@BoyBaykiller I removed my previous comment since it was inaccurate.

glTF makes a distinction between used and required extensions.

unknown used extensions should be fine to load, and if the library throws when loading them, then it is indeed a bug on the library side.

required extensions usually change the behavior of the model data in a way that, if the library does not handle that extension, evaluating the model would be impossible or would lead to a corrupted or incorrect representation of the model, in that case the library must report this model as unloadable.

Looking at the code, _LinkThrow("Extensions", iex); throws when it finds an extensions that the model declares as required, but the library does not support it, so it's definitely not an overreaction. The only way to load these models is to implement the extension in the library, or if it's a custom extension, consider whether should be exported as required or not.

BoyBaykiller commented 4 months ago

Thats what I am currently doing with my custom extension - only put it in used and not required. Preferably it would go in required and at the same time not have SharpGLTF refuse to load the model. The extension itself doesn't make any glTF schema updates btw, it's just used as a marker for the engine to do certain things. The details are irrelevant.

Other custom extensions might make schema updates. By throwing an exception, you are not allowing the user to process unknown required extensions manually while also using SharpGLTF to load the rest of the model.

But it's ok. I just wanted to hear your opinion on this. This is not supposed to be a bug report. You may close this.

vpenades commented 4 months ago

whether an extension is declared as used or required is really up to whether the model is correctly renderable on an engine unable to understand that extensions, but also to prevent the engine to crash with unrecognizable data, so how would you signal that to the engine that loads the model?

The rule of thumb is that if it is safe for an engine to load your model without understanding your extension, it's probably fine to declare the extension as used.

But if your extension changes things in a way that the engine would crash, or render the model so wrong that it would be unrecognizable, then it's when you would use the required.

There's a tweak in the library, you can remove the validation at load time with one of the load extra parameters... but it disables -all the validation- which means you're in your own if there's issues.

BoyBaykiller commented 4 months ago

Ok. I've tried new ReadSettings() { Validation = SharpGLTF.Validation.ValidationMode.Skip }, but the exception is still thrown.

vpenades commented 4 months ago

hmm... i'll take that into account, that validation is somewhat special