Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BoyBaykiller opened this issue May 18, 2024 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@BoyBaykiller
Copy link

Currently SharpGLTF throws when an unsupported extension is required by a glTF file:

validate._LinkThrow("Extensions", iex);

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?

@BoyBaykiller BoyBaykiller added the bug Something isn't working label May 18, 2024
@vpenades
Copy link
Owner

vpenades commented May 18, 2024

@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
Copy link
Author

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
Copy link
Owner

vpenades commented May 20, 2024

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
Copy link
Author

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

@vpenades
Copy link
Owner

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

@BoyBaykiller
Copy link
Author

Blender puts KHR_materials_variants in required so I can't load any models outputted by blender using this extension. I still think SharpGLTF shouldn't refuse to load the model when encounting unsupported required extension.

@vpenades
Copy link
Owner

vpenades commented Oct 4, 2024

Do these models actually use the extension? Or blender exporter blindly adds the extension requirement even if it's not used?

@BoyBaykiller
Copy link
Author

Yes, the model defines two variants. Use this Dragon Attenuation model to reproduce. It has KHR_materials_variants only in extensionsUsed but after exporting with blender it's in extensionRequired.

@vpenades
Copy link
Owner

vpenades commented Oct 5, 2024

I can't find any information that KHR_materials_variants should be set as "required".

I still think SharpGLTF shouldn't refuse to load the model when encounting unsupported required extension.

I understand that it could be fine to load these models in this specific use case, but the glTF specification has "used" and "required" fields specifically to signal when you are safe to load the model and when not. So SharpGLTF is following the specification of refusing to load a model when it does not support an extension the model is signaling as "required". You might think that these models are safe to load anyway, but the library cannot know in advance when the model is "lying" about it, there's many required extensions that are really required and would crash or produce incorrect results downstream if loaded.

It's responsability of the writer library to correctly signal what's used and what's required.

As I said, I could consider removing the check when the Skip flag is set, but you must understand that what you're asking me is to break a feature that is working correctly in my library, because some other library is not following the standard correctly.

Is there an open issue in the blender exporter library requesting to save Variants as used and not as required?

@BoyBaykiller
Copy link
Author

I agree with everything you have said. The reason I would not let SharpGLTF throw an exception for unsupported extensions is not because I think that it's more correct, but as a form of leniency to avoid cases like this.

I've opended a issue regarding KHR_materials_variants on the blender project.

@vpenades vpenades added enhancement New feature or request and removed bug Something isn't working labels Oct 6, 2024
@vpenades
Copy link
Owner

vpenades commented Oct 9, 2024

Reviewing the issue, there's an additional problem about allowing glTFs with unsupported but required extensions to load.

When SharpGLTF encounters an unsupported extension, it has no way to deserialize it. , so instead, it stores the extension code as an "undefined" json.Node.

This is fairly safe when the given extension is only "used".

But for "required" extensions it means part of the data you might need to fully traverse the scene is "hidden" in these unknown json nodes.

I've opended a issue regarding KHR_materials_variants on the blender project.

Could you point me to the issue? so I can track it too?

@BoyBaykiller
Copy link
Author

https://projects.blender.org/blender/blender/issues/128647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants