KhronosGroup / glTF-Validator

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

Images data validation #21

Open lexaknyazev opened 7 years ago

lexaknyazev commented 7 years ago

Right now, Validator checks only that image dimensions are powers-of-two.

Next checks:

Thoughts?

javagl commented 7 years ago

Regarding the first sentence: There is an implementation note at https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#samplers saying

Non-Power-Of-Two Texture Implementation Note: glTF does not guarantee that a texture's dimensions are a power-of-two.

So the power-of-two test should probably not be done at all...?

I'll look at this and the other issues ASAP in more detail (but am not sure whether I can make helpful contributions)

lexaknyazev commented 7 years ago

So the power-of-two test should probably not be done at all...?

OpenGL ES 2.0 hardware may refuse to render npot textures. I'd argue that this is a reasonable validation warning for runtime format.

javagl commented 7 years ago

The full implementation note also says something about the conditions under which a texture has to be resized to be POT (maybe I should have quoted this as well, but you're likely more aware of and familiar with this than me anyhow).

I think that an asset that is perfectly valid in terms of the spec should not cause any warnings. The renderers have to be aware of the fact that the textures may be non-POT (and have to resize them anyhow, under certain conditions). But I don't have such a strong opinion here, maybe the warning is justified in order to increase this awareness...

emackey commented 7 years ago

Some packages have levels below error and warning, such as info, suggestion, and debug. Sometimes, an API is provided to suppress messages below a user-supplied threshold, for less verbose output (for example, only show me errrors and warnings, not info or suggestions).

To me, NPOT textures sound like info level:

Info: Using non-power of 2 textures causes performance implications for some platforms.

Or something to that effect. It shouldn't be a "warning" if the spec allows it, IMHO.

emackey commented 7 years ago

For what it's worth, VSCode's DiagnosticSeverity includes four values. In descending order of severity they are: Error, Warning, Information, and Hint.

This is used for example in their language server sample to report validation errors back to the text editor. The last one, Hint, does not even place a mark in the text editor itself, it just lists the hint in a separate "Problems" tab with a full list of all four levels of messages. The other three levels do place a squiggle-underline mark below the text in the editor that triggered the validation message, and in the case of Error, the squiggle is bright red.