KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.21k stars 1.14k forks source link

Schema defines minimum sizes for optional arrays in some cases #916

Closed peteroupc closed 7 years ago

peteroupc commented 7 years ago

Some array properties of the glTF 2.0 schema define a minItems of 1 even though they're optional and even though some of the sample assets include empty arrays for these properties. A notable example of this is nearly all the properties of the glTF root (glTF.schema.json). I find this quite problematic.

bghgary commented 7 years ago

Edited: See @lexaknyazev's response for the reason it was changed https://github.com/KhronosGroup/glTF/issues/916#issuecomment-295002229

I think the idea with the minItems of 1 for optional arrays to prevent empty arrays like the following (since it doesn't accomplish anything and bloats the JSON):

{
    "materials": []
}

The materials property should be omitted instead of an empty array. Not all the samples are up-to-date with the latest 2.0 spec yet. I have a PR with a few updated sample models if you want to take a look. https://github.com/KhronosGroup/glTF-Sample-Models/pull/49

If we have optional arrays that don't have minItems > 0 specified in the schema, we should probably fix them.

peteroupc commented 7 years ago

As I see it, the advantages to allowing empty arrays to appear on the affected properties are that:

The only disadvantage is that the file size will be slightly higher.

I'm afraid the disadvantages don't outweigh the advantages in this respect, in my opinion.

By the way, this issue currently occurs in the samples Suzanne, TwoSidedPlane, and Cube as well. To keep empty arrays for those properties from propagating "in the wild", these three samples should be fixed as well. Thankfully, however, only the animations property is affected in these files. However, there is no guarantee that future generators will not still create glTF files with empty arrays for the affected properties (see the first advantage above).

bghgary commented 7 years ago

generators can have a simpler implementation

Agree this is a benefit for some generators. Some JSON serializers take care of this by not writing out properties with default values, which I would encourage generators to always do to avoid increasing the size of the JSON and parsing time.

parsers can assume an empty array if the property is present but empty

I don't know what you mean here. This is true whether we disallow or allow empty arrays? I can see a benefit for the parser if we always require the array property and also allow empty array. Is that what you mean?

peteroupc commented 7 years ago

I don't know what you mean here. This is true whether we disallow or allow empty arrays? I can see a benefit for the parser if we always require the array property and also allow empty array. Is that what you mean?

Yes. What I mean is that parsers don't have to check whether an array is empty when they encounter it, since empty arrays would be allowed for the affected properties.

bghgary commented 7 years ago

empty arrays would be allowed for the affected properties

The properties would also need to be required for this to be true all the time which means you need an empty array for every top-level property even if they are not used.

Like this:

{
    "extensionsUsed": [ ],
    "extensionsRequired": [ ],
    "accessors": [ ],
    "animations": [ ],
    "buffers": [ ],
    "bufferViews": [ ],
    "cameras": [ ],
    "images": [ ],
    "materials": [ ],
    "meshes": [ ],
    "nodes": [ ],
    "samplers": [ ],
    "scenes": [ ],
    "skins": [ ],
    "textures": [ ]
}
lexaknyazev commented 7 years ago

It was decided in issue #815, that scenes array can't be empty when it exists. Other arrays were changed for consistency.

peteroupc commented 7 years ago

@bghgary:

I don't believe that's true. A counterexample is the node.children property, which doesn't include a minItems in its schema. Not having a minItems (or having a minItems of 0) in its schema is what allows a certain property to either appear as an empty array or not appear at all, while still being optional.

bghgary commented 7 years ago

@peteroupc My point is not that it's not possible to do in the schema. You listed in your second bullet above that doing this will be an advantage to parsers. I don't understand this. Parsers will still have to deal with omitted properties either way. It is not a benefit to the parser unless the property is always there. Am I missing something?

peteroupc commented 7 years ago

What I mean is that, by setting a minItems to 0 or omitting minItems for the affected properties, parsers that follow the schema will not reject a glTF document because the property appears as an empty array (just as they won't if the property is optional, but omitted), especially if the default value for the property in question (if that property is omitted) is an empty array.

bghgary commented 7 years ago

I see. It depends on the strictness of the parser and how it's validating against the schema. For a strict parser, if it implements the validation manually then it will require more checking like you are describing. If the parser uses the schema directly to validate, then there is no additional work to implement. For a parser that assumes the glTF is well-formed, it makes no difference.

lexaknyazev commented 7 years ago

Current state of array-based properties across glTF 2.0 schema:

Required & non-empty

Optional & non-empty

Optional & could-be-empty

Required & could-be-empty


Parsers may use default values when an optional property isn't provided. This isn't a big deal with simple types (such as numbers or strings) but could lead to unnecessary memory allocations for optional empty arrays (node.children is my main concern here).

Also, some lazy exporters may always write all properties (incl. optional empty arrays) thus generating bloated assets.

Proposal


CC @pjcozzi @bghgary @javagl

javagl commented 7 years ago

One could dive deeply into possible pros and cons of writing default values in general. The only semantic difference appears when a default value changes in a future glTF version (but this should be avoided anyhow, for an awful number of reasons). Regardless of that: I think that the spec should be consistent as far as reasonably possible (and as Alexey pointed out: Currently, it isn't).

So some highly subjective comments:

I don't like empty arrays in the JSON file in the cases where the array is optional. I see that allowing empty arrays may simplify the implementation of exporters in some cases. But even if it was allowed, it may be a matter of "geek's pride" to not bloat the file with unnecessary data...

For the case of a parser, one could argue that it should be "resilient" in that regard (and not choke on an empty array even if minItems>0), but of course, this cannot be enforced, and does not apply to real validators and to parsers that are auto-generated from the schema.

I'd strive for a "clean" file that does not have unnecessary empty arrays, and thus, would vote for the proposal given above.

But: I know that there are different mindsets and conditions, depending on the languages and APIs. (Odd null/undefined handling in JavaScript, no null-references in C++, ... etc). If there are compelling reasons to allow the empty arrays, then this could be OK (if it was allowed consistently). (And by "compelling", I mean something that goes beyond the necessity to insert one or the other if (materials.empty()) statement here and there in an exporter...)

peteroupc commented 7 years ago

Just as it can't be enforced that a parser is "resilient" against glTF documents that contain empty arrays on properties with minItems equal to 1, neither can it be prevented that some glTF documents will nevertheless contain such arrays. So long as empty arrays on such properties still appear on any of the glTF 2.0 sample models (or on any significant number of other glTF documents -- especially before glTF 2.0 becomes final), such a danger will persist, which is why I have opened an issue on this matter.

pjcozzi commented 7 years ago

Added in #826