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

Flag glTF "common problems" #147

Open
4 tasks
emackey opened this issue Sep 23, 2020 · 13 comments
Open
4 tasks

Flag glTF "common problems" #147

emackey opened this issue Sep 23, 2020 · 13 comments

Comments

@emackey
Copy link
Member

emackey commented Sep 23, 2020

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:

  • An image (larger than 2x2) consists of a single solid color, and could be replaced by a glTF factor. (Small images can be ignored as they are sometimes used as placeholders for runtime-generated images).

  • Images with a dimension larger than... 2048? 4096? could be flagged for reduced compatibility.

  • A given material that uses more than 10? 12? texture references could be given a warning that some implementations won't be able to handle that many textures in a material.

  • 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. The number of possible combinations in PBR Next will likely be quite large. Typically only PNG users would be interested in combining like this, as other formats may be susceptible to channel cross-talk.

See also Scene Complexity Limits in glTF.

@lexaknyazev
Copy link
Member

lexaknyazev commented Sep 23, 2020

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

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

  • Decoding JPEG requires parsing the bitstream, building Huffman tables, applying DCT, and doing YCrCb to RGB transform. Implementing this from scratch is absolutely out of scope. So to have optimal performance we'd have to use several platform-dependent backends: DOM for the web, some kind of WASM JPEG implementation for npm, and dynamic linking to a native library for the CLI tool. WebP or AVIF would be similar but even more complex. While implementing all of this is certainly possible, I'd prefer that we steer the ecosystem away from CPU-only lossy compression.
  • Doing this for PNG is much simpler as it uses only zlib/deflate with a relatively simple scanline filter. Support for PNG headers is already present in the Validator.
  • The same check for Basis Universal textures would be relatively easy as soon as we have an optimized BasisLZ/ETC1S implementation here.

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

elalish commented Sep 23, 2020

  • A corollary of the first one: if the occlusion channel is all black (or the factor is zero) this does not make physical sense. This is oddly common (not sure which exporter is producing these).
  • Using DoubleSided or BackFacing usually indicates triangle winding problems.
  • Using a non-default minFilter or maxFilter is nearly always an error (probably the most common problem I see).
  • If more than some small % of vertices are duplicated then they probably have triangle soup.
  • Folded edges (adjacent triangle normals point opposite ways) are bad.
  • If pixels per meter varies a lot it can indicate poor UV unwrapping.

@emackey
Copy link
Member Author

emackey commented Sep 23, 2020

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

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

@emackey
Copy link
Member Author

emackey commented Sep 23, 2020

@elalish Mostly agreed, but:

  • I don't think using doubleSided should be enough by itself to issue a message. Some of us use this intentionally 😄

  • There is no default behavior defined for minFilter or magFilter, it's just a "clients can do whatever they think is best" kind of thing. One school of thought says you should always specify a value here, for that reason. (But, I could maybe be talked into an info for NEAREST saying you might see pixels, or something like that).

@donmccurdy
Copy link
Contributor

donmccurdy commented Sep 23, 2020

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:

  • Including multiple copies of the same image data.

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

@elalish
Copy link

elalish commented Sep 23, 2020

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

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

@elalish
Copy link

elalish commented Sep 23, 2020

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

emackey commented Sep 23, 2020

@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: #14 (comment)

@lexaknyazev
Copy link
Member

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

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

@emackey
Copy link
Member Author

emackey commented Nov 3, 2020

And KHR_texture_transform applied, with all default values.

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

No branches or pull requests

4 participants