KhronosGroup / glTF

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

Tighten asset spec version definition #2452

Open lexaknyazev opened 1 month ago

lexaknyazev commented 1 month ago

The spec currently says the following about the asset.version property:

The glTF version in the form of <major>.<minor> that this asset targets.

Additionally, the JSON schema implies the following regexp pattern for the string value: ^[0-9]+\.[0-9]+$.


Neither of these definitions is precise enough to require exactly "2.0" spelling for glTF 2.0 assets. For example, should it be valid to use "02.00" as a version string?

@emackey @javagl @bghgary WDYT?

javagl commented 1 month ago

One could be tempted to say that some decisions here could have been governed by "what usual parsers do" (like the JavaScript Number(s) conversion or Java Float.parseFloat). On the other hand, one could make a case that people should not even try to parse that string like that. (Well, maybe try, but not try). The ones that I mentioned safely consume that "02.00" and create a 2.0 from that, but others may not.

While I'd be in favor of having a more constrained pattern here, there are some difficulties/drawbacks with introducing that constraint now:

  1. It's not clear what the pattern should be. The leading zero for the major version number may look odd, and may be disallowed. Allowing leading zeros for the minor version number, like 2.01, could be reasonable. Forbidding trailing zeros for the minor version could be hard to justify.
  2. The RegEx might become complicated (depending on the details of point 1)
  3. Previously valid assets would become invalid!

The last one may be the most obvious and most critical one. I'm pretty sure that 99% of tools and libraries are just hard-wiring that "2.0" there. And there might be consumers of assets that try to go down the easy path, and make this assumption as well, like if (version !== "2.0") throw up;. But I'd consider this as a flaw in the implementation.

That's not really a strong opinion. But "making valid assets invalid" is a big deal, IMHO.

(Besides... we all know that nothing's gonna happen here. The spec could just have said "This string must always be "2.0"" 😁 )

lexaknyazev commented 1 month ago

In some cases, leading zeros may change the number's base to eight. For example, 010 == 8 in JavaScript for number literals (although this does not apply to parseInt or implicit string-to-number type coercion).

FWIW, the validator allows leading zeros for both parts currently.

There are two consistent options here:

Since it's an ambiguity in the spec, it must be addressed so doing nothing is not an option.

lexaknyazev commented 1 month ago

By the way, parsing the version string as a float is a very bad idea because it would be precision-dependent and leading zeros in the minor part would change the semantics:

javagl commented 1 month ago

Since it's an ambiguity in the spec, it must be addressed so doing nothing is not an option.

I don't see an ambiguity here. It's very clear what is allowed and what is not. The string currently can be "02.00". That's valid - but not desirable (and I assume this iswhat you meant).

By the way, parsing the version string as a float is a very bad idea

I tried to emphasize that in the introductory paragraph. The approach should be to split the string at . and check the tokens individually. (It could start by checking against the regex, but at some point, the tokens will have to be analyzed.). Realistically, given that in practice it only has to check for the string being "2.0", people will get away with a simple parsing approach. But the question about leading zeros still stands.

I also thought about the different base due to the leading 0 (which is pretty common, but distressingly inconsistent between literals and the parsing/conversion functions).

(I actually looked up how I'm currently handling this. I tried to be generic there, roughly inspired by the "semver" patterns. It will accept things that are not allowed by the glTF spec. And maybe most importantly: It will not properly handle leading zeros - meaning that it will not differentiate between "2.1" and "2.01")

However, the latter can be justified with https://semver.org/#spec-item-2 , which disallows leading zeros in general. This could be seen as a "precedent" to disallowing that for glTF as well.

lexaknyazev commented 1 month ago

It's very clear what is allowed and what is not.

It's clear only to those who looked at the schema and noticed that the format there allows leading zeros. The spec language does not mention this and all examples use only "2.0" spelling.

The string currently can be "02.00".

Technically yes. The practical question is whether to disallow that and invalidate existing assets (if any), which don't work with, e.g., glTF-Transform, or to encourage implementations to handle that property as currently specified.

/cc @donmccurdy

donmccurdy commented 1 month ago

I would tend to prefer that leading zeroes be disallowed, as we expect this means "no change" in any known tool — and supporting leading zeroes would require many tools to change, and to abandon any pre-existing libraries for parsing version strings. Consistency with version string standards like SemVer is a plus, too, I think.

javagl commented 1 month ago

A very quick sample:

(One could look at many more, I was just curious here...)

bghgary commented 1 month ago

FWIW

For additional context, we have a dot version out there for glTF 1.0 (namely 1.0.1) but have yet to introduce a dot version for 2.0 and probably never will.

Practically speaking, I don't see a good reason to allow leading zeroes. I doubt tightening the spec to indicate this is going to cause actual problems.

lexaknyazev commented 1 month ago

Assuming that:

The updated format would then be: ^([1-9][0-9]*)\.(0|[1-9][0-9]*)$. In practice though, tools will probably keep using the fixed "2.0" string for the foreseeable future.

emackey commented 1 month ago

Does that Regex allow 2.0.1? Do we want it to? (Maybe we don't want that...)

lexaknyazev commented 1 month ago

Does that Regex allow 2.0.1? Do we want it to? (Maybe we don't want that...)

It does not. The current Regex does not allow it either.

zeux commented 1 month ago

fwiw:

lexaknyazev commented 1 month ago

cgltf uses atof(version) < 2.0 to reject unsupported glTF 1.0 assets (which would work for "02.00")

FWIW, atof("1.9999999999999999") == 2.0.

zeux commented 1 month ago

Sure. Note that alternative parsing scheme mentioned above that uses "parseInt" (actually returns a double) in Dart/JS would also change the minor version here to "10000000000000000". When using Dart Native, adding a few more 9's will throw from int.parse because there ints are 64-bit integers, not 64-bit doubles, so an overflow happens.

If we assume versions like that are valid and expected, then certainly they should be preserved exactly without changing their meaning, which means any application that isn't using strings (or infinite precision decimals) is "broken". But given that the minor version is still at 0, all of this seems theoretical.

javagl commented 1 month ago

While I'm also curious about corner cases and limitations, I think that when someone writes a version number like 2.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001 to try hitting the limit of "the minimum value that a 64bit floating point number can have", we can safely assume malicious intent.

lexaknyazev commented 1 month ago

A difference between 1.9999999999999999 and 1.10000000000000000 is purely theoretical but a difference between 1.9999999999999999 and 2.0 is very practical. It goes without saying that tools should gracefully reject assets that cause runtime crashes.

Besides disallowing leading zeros, we can further restrict the version format based on common runtime limitations. For example, JS numbers cannot precisely represent integers greater than 9007199254740992. I think it's a strong enough reason to limit the number of digits in each part to 15 and therefore cap the maximum possible glTF version at 999999999999999.999999999999999.

The goal of such spec clarification is to limit the parsing scope so that implementations could explicitly not worry about numeric limits.

^([1-9][0-9]{0,14})\.(0|[1-9][0-9]{0,14})$