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

Tighten asset spec version definition #2452

Open
lexaknyazev opened this issue Oct 27, 2024 · 16 comments
Open

Tighten asset spec version definition #2452

lexaknyazev opened this issue Oct 27, 2024 · 16 comments
Assignees
Milestone

Comments

@lexaknyazev
Copy link
Member

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

javagl commented Oct 27, 2024

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

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:

  • Disallow leading zeros (for both parts) in the spec language and update the regexp. This may invalidate some assets somewhere but they are very likely not portable anyway.
  • Explicitly allow leading zeros (for both parts) in the spec language, keep the regexp, and add a few sample assets to cover this. At the very least, they should cover "2.00", "02.0", and "02.00" spellings. Some engines may need to be updated to accept such assets.

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

@lexaknyazev
Copy link
Member Author

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:

  • 2.1 != 2.01
  • 2.0000001 == 2.0 (for float32)
  • 2.0000000000000001 == 2.0 (for float64)

@javagl
Copy link
Contributor

javagl commented Oct 28, 2024

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

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

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

javagl commented Oct 28, 2024

A very quick sample:

  • glTF-Transform:

    • uses default string "2.0" when writing (link)
    • checks for fixed string "2.0" when reading (link) - I think this will break for "2.00" (but haven't verified that)
  • JglTF:

    • uses fixed string "2.0" when writing (link)
    • parses the part before the first "." as an integer (ignoring leading zeros), and checks it to be 2 when reading (link) (Note that this also supports glTF 1.0 ...)
  • tinygltf:

    • uses default string "2.0" for writing (link)
    • seems to parse the version when reading, but not check it in any way (link) (Maybe there is a check that I overlooked)
  • CesiumGltf:

    • seems to write the version string that was set by the user, and ignore the version when reading...

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

@bghgary
Copy link
Contributor

bghgary commented Oct 28, 2024

FWIW

  • Babylon.js:
    • uses fixed string "2.0" when writing (link)
    • splits by "." then calls parseInt on each part (link)

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 lexaknyazev added this to the 2.0.x milestone Oct 28, 2024
@lexaknyazev lexaknyazev self-assigned this Oct 28, 2024
@lexaknyazev
Copy link
Member Author

Assuming that:

  • Major version 0 is not valid.
  • Leading zeros are disallowed in both parts.
  • Both major and minor version parts may have any number of digits.

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

emackey commented Oct 28, 2024

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

@lexaknyazev
Copy link
Member Author

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

zeux commented Oct 28, 2024

fwiw:

  • cgltf uses atof(version) < 2.0 to reject unsupported glTF 1.0 assets (which would work for "02.00")
  • cgltf_write preserves the source version string as is (so if the source asset had "02.00" then the tools using it would write "02.00")
  • gltfpack always writes fixed string "2.0" (it uses cgltf parser but not cgltf_write) regardless of the source asset

@lexaknyazev
Copy link
Member Author

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

zeux commented Oct 29, 2024

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

javagl commented Oct 29, 2024

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

lexaknyazev commented Oct 29, 2024

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})$

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

No branches or pull requests

6 participants