KhronosGroup / glTF-Validator

Tool to validate glTF assets.
Apache License 2.0
370 stars 63 forks source link

Flag glTF "common problems" #147

Open emackey opened 4 years ago

emackey commented 4 years ago

There are some typical problems that can crop up in models created by users who aren't well-versed in all the best practices for optimizing glTF assets. The validator could issue "info" or even "warning" for some of these, for example:

See also Scene Complexity Limits in glTF.

lexaknyazev commented 4 years ago

An image (larger than 2x2) consists of a single solid color

This would require performing full image decode. To put things into some perspective:

Images with a dimension larger than... 2048? 4096?

It's a low-hanging fruit as dimensions are already reported for all supported image formats.

A given material that uses more than 10? 12? texture references

With WebGL 2.0 the MAX_TEXTURE_IMAGE_UNITS is at least 16. There should be some units reserved for LUTs and IBL. The good news is that a single texture array can have at least 256 layers making this restriction obsolete when textures have the same dimensions, pixel format, and filtering modes.

Look for opportunities to pack channels together into fewer images? This could be challenging to determine, but for example if an occlusion PNG were separate from a metal/rough PNG, those could be combined.

Given that channel mapping is fixed, this is fairly straightforward.

elalish commented 4 years ago
emackey commented 4 years ago

This would require performing full image decode

Yes, I have reservations about this. For me, it's not worth it if there are costs in terms of additional 3rd-party dependencies, and/or a major reduction in runtime performance. If we have to drop this one, so be it. This is one where it's probably easier for a human to just look at a sheet of thumbnails.

lexaknyazev commented 4 years ago

Yet another list of potential issues (not all of them are problems though): https://github.com/KhronosGroup/glTF-Sample-Assets/issues/56.

emackey commented 4 years ago

@elalish Mostly agreed, but:

donmccurdy commented 4 years ago

This would require performing full image decode

If this feels out of scope for glTF-Validator, I'd be willing to implement it in https://gltf-transform.donmccurdy.com/, along with a script to fix the problem. I have already implemented PNG and JPEG decoding there, with get-pixels, which may not cover the exhaustive JPEG spec. I plan to add KTX.

Another suggestion:

^I see this occasionally in models from online 3D asset libraries. Unsure whether it's an artist mistake or a library/exporter bug.

elalish commented 4 years ago

@emackey Again, only a warning, not an error. Like, did you really mean this to be DoubleSided, because the performance will be worse and you might just be covering up for the fact your triangles are not oriented.

I would also put a warning on anything other than minFilter = LINEAR_MIPMAP_LINEAR and maxFilter = LINEAR about likely leading to artifacts. On modern hardware there's hardly any cost to these anyway. This is my single biggest cause of opened rendering issues in model-viewer (I honestly wish they weren't in the spec at all).

lexaknyazev commented 4 years ago

I remember @bghgary saying couple years ago that some artists intentionally want to use nearest filtering.

elalish commented 4 years ago

@lexaknyazev Yes, for sure, just like DoubleSided is sometimes called for. But I think a warning of some kind is still helpful as they are vastly more often abused unintentionally, and anyone who has done it intentionally will understand they can ignore the warning.

emackey commented 4 years ago

@elalish Yes. But let's be careful of terminology here: The validator has 4 levels, error, warning, info, and hint. Currently there are no hints, but the other 3 are used.

Something like using the doubleSided flag would, in my mind, barely qualify as info, maybe it would be our first hint. I like your "folded edges" test, that could definitely be a warning as it's much more robust. False positives are more serious for higher message severities.

I'd be OK with either warning or info on nearest filtering. Unless you're exporting from Minecraft, it's probably wrong.

Here's my (old) idea for how these levels are to be used: https://github.com/KhronosGroup/glTF-Validator/issues/14#issuecomment-335961877

lexaknyazev commented 4 years ago

We took these from the VS Code DiagnosticSeverity enum.

Btw, there's a new feature there called DiagnosticTag with two more specific semantics: Unnecessary and Deprecated. I think we can use them as well.

lexaknyazev commented 4 years ago

Another common problem: a complete factor / texture pair with the factor set to 0.0.

emackey commented 4 years ago

And KHR_texture_transform applied, with all default values.