-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Tighten asset spec version definition #2452
Comments
One could be tempted to say that some decisions here could have been governed by "what usual parsers do" (like the JavaScript While I'd be in favor of having a more constrained pattern here, there are some difficulties/drawbacks with introducing that constraint now:
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 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 |
In some cases, leading zeros may change the number's base to eight. For example, 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. |
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:
|
I don't see an ambiguity here. It's very clear what is allowed and what is not. The string currently can be
I tried to emphasize that in the introductory paragraph. The approach should be to split the string at I also thought about the different base due to the leading (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 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. |
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
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 |
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. |
A very quick sample:
(One could look at many more, I was just curious here...) |
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. |
Assuming that:
The updated format would then be: |
Does that Regex allow |
It does not. The current Regex does not allow it either. |
fwiw:
|
FWIW, |
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. |
While I'm also curious about corner cases and limitations, I think that when someone writes a version number like |
A difference between 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 The goal of such spec clarification is to limit the parsing scope so that implementations could explicitly not worry about numeric limits.
|
The spec currently says the following about the
asset.version
property: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?
The text was updated successfully, but these errors were encountered: