-
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
Describe numbers encoding in JSON #1574
Conversation
specification/2.0/README.md
Outdated
@@ -178,6 +178,10 @@ To simplify client-side implementation, glTF has additional restrictions on JSON | |||
|
|||
> **Implementation Note:** This allows generic glTF client implementations to not have full Unicode support. Application-specific strings (e.g., values of `"name"` properties or content of `extras` fields) may use any symbols. | |||
3. Names (keys) within JSON objects must be unique, i.e., duplicate keys aren't allowed. | |||
4. Numbers defined as integers in the schema must be written without fractional part (i.e., `.0`). Exporters should not produce integer values greater than 2<sup>53</sup> because some client implementations will not be able to read them correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of prohibiting a fractional part? It may be parsed differently in a typed language, I suppose? If any mainstream JSON serializer doesn't do this, it won't be within a developer's ability to change it. If it's already universal in common libraries, as it is for JS, that's fine.
Note that JSON also allows an exponent part: https://stackoverflow.com/a/19554986/1314762. Should that be mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From JSON-Schema spec:
Some programming languages and parsers use different internal representations for floating point numbers than they do for integers.
For consistency, integer JSON numbers SHOULD NOT be encoded with a fractional part.
We had this issue with files made by the Blender exporter at one point. JSON.stringify
is fine. Note, that this affects only properties marked as "integer" in the schema such as:
"buffers": [
{
"byteLength": 25.0
}
]
I'm fine with changing "must" to "should" to align better with JSON-Schema language. Exponent notation is usually associated with floats, so it shouldn't be used for integers as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might only be remotely related, but related: There has been some discussion about this at KhronosGroup/glTF-Validator#8
The point that people might not be able to modify the behavior of JSON serializers may be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that loaders using typed languages that rely on "integer" schema type are a bigger concern here rather than half-baked serializers. For example, "componentType":5123.0
crashes some loaders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i'm fine with either "must" or "should", whichever you prefer. Let's also mention exponents here.
@@ -178,6 +178,10 @@ To simplify client-side implementation, glTF has additional restrictions on JSON | |||
|
|||
> **Implementation Note:** This allows generic glTF client implementations to not have full Unicode support. Application-specific strings (e.g., values of `"name"` properties or content of `extras` fields) may use any symbols. | |||
3. Names (keys) within JSON objects must be unique, i.e., duplicate keys aren't allowed. | |||
4. Numbers defined as integers in the schema must be written without fractional part (i.e., `.0`). Exporters should not produce integer values greater than 2<sup>53</sup> because some client implementations will not be able to read them correctly. | |||
5. Floating-point numbers must be written in a way that preserves original values when these numbers are read back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an implementer I don't know what to do with this as a normative requirement... no one should be writing a JSON serializer from scratch, and I would not know how to evaluate this requirement on a JSON library, other than (as you suggest below) looking for something common and well-tested. Some developers (e.g. on the web) will not have a choice in their JSON implementation.
Perhaps this should just be an implementation note – that implementers should be aware of this when choosing a serialization method, and that common JSON libraries handle it automatically.
Is this something we can detect in validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All "reasonable" approaches (such as JSON.stringify
on the Web, platform-provided JSON implementations in languages like Python, commonly-used C++ libraries) are already aligned with this requirement.
Validating it would require comparing glTF JSON with some "source" data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevertheless, I think this is too vague to be a normative requirement, and should probably be an implementation note.
It is probably also worth saying at the top of this section that all of these "additional restrictions on JSON" are already implemented by common JSON libraries, even if they are not required by the JSON spec.
Follow-up from KhronosGroup/glTF-Blender-IO#843. There's no integer data type in JSON. All numbers are just This inevitably leads to several edge cases (the PR should be updated to cover all of these):
|
@@ -178,6 +178,10 @@ To simplify client-side implementation, glTF has additional restrictions on JSON | |||
|
|||
> **Implementation Note:** This allows generic glTF client implementations to not have full Unicode support. Application-specific strings (e.g., values of `"name"` properties or content of `extras` fields) may use any symbols. | |||
3. Names (keys) within JSON objects must be unique, i.e., duplicate keys aren't allowed. | |||
4. Numbers defined as integers in the schema must be written without a fractional part (e.g., `.0`). Exporters should not produce integer values greater than 2<sup>53</sup> because some client implementations will not be able to read them correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the "without a fractional part" requirement, could you include the motivation for that? A value of 1.0
would pass JSON validation for { "type": "number", "multipleOf": 1.0 }
, so this requirement is really because some (many?) JSON libraries for typed languages depend on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think the rationale should be included explicitly in the specification – otherwise it sounds like we're inventing our own subset of JSON, which would be a bad look. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the issues that are related to the original integer
type have already been linked to in other comments. Some of the related hiccups can probably be explained by the fact that earlier versions of the ("official") JSON-Schema included the integer
type, but it has been removed in the meantime (cf. json-schema-org/json-schema-spec#146 ).
One specific issue is caused by the combination of a typed language and automatic code generation from the schema. In doubt, the code generation has to be tweaked and configured so that a { "type": "number", "multipleOf": 1.0 }
is translated into a long
, and that the resulting parser does a parseLong
instead of a parseDouble
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"type": "integer"
was added back at some point. Latest definitions are:
-
concept of "integer" is from a vocabulary, not the data model
-
For consistency, integer JSON numbers SHOULD NOT be encoded with a fractional part.
-
String values MUST be one of the six primitive types ("null", "boolean", "object", "array", "number", or "string"), or "integer" which matches any number with a zero fractional part.
Sources:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there's an important distinction between "SHOULD NOT" (the JSON spec wording) and "must be written without a fractional part" (this PR). The JSON wording doesn't outright prevent encodings like 1.1e1
for integers, although they are strongly discouraged.
It may still make sense for glTF to have stronger enforcement, for the sake of strongly-typed glTF readers that don't wish to take extra steps of parsing floats and rounding. But I agree with Don's concern that this needs to avoid becoming a subset of JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, they update the JSON schema draft regularly, and I wonder about their roadmap for a ratification - without that, #929 remains a moving target... :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should copy the JSON wording here, changing "must be written without a fractional part" to "should not be written without a fractional part."
Couple more edge cases from today's call.
|
The latter example would also work with JS array indices (think all glTF internal references): const arr = [];
arr[8192] = 41;
arr[8192.0000000000001]; // == 41 |
Specifying "JSON itself" cannot be in the responsibility of Khronos or the working group. For the particular example of numbers: Even the JSON RFC remains irritatingly vague here, and focusses on "interoperability", saying that ~"most tools will use 64bit IEEE754"... This issue was only about the numbers, but the spec currently also disallows duplicate keys. The JSON RFC just says:
This is not something that I'd like to see in a real specification. This is exactly the vagueness that has led to JSON parsers/writers that are not as "interoperable" as one would like them to be. But generally: When adding constraints for the JSON part of glTF, we could try to distinguish between things that are in the control of the user, and the parts that are not. The constraint to disallow duplicate keys is reasonable. There probably are few writers that can do that anyhow. But trying to iron out the kinks of the JSON specs for valid In the best case, we could/should be able to say: "The JSON part of glTF is JSON (whatever that means)", but ... here we are now. Making these constraints implicit, by pointing to the JSON spec and saying that ~"the general rules for 'interoperability' that are mentioned there SHOULD be followed" might be a reasonable solution for a problem that, strictly speaking, is beyond our control. |
That also depends. Although it's very unlikely that any commonly-used JSON library would export such an asset, readers usually have no control over the parsed result. FWIW, I'm inclined to rephrase most restrictions from this section as interoperability suggestions and/or best practices while explicitly listing the corner cases that implementations should assess themselves. WDYT? |
That sounds reasonable for me. This would come close to the desired solution of just pointing to the JSON spec and leaving the burden of interpreting that correctly not to the implementors of glTF libs, but to implementors of JSON libs. (One reason why I'm hesitant to add constraints is related to a very old issue, where you suggested that "writers / converters MUST write If nobody else has objections, I'd agree that this could be written as recommendations and interoperability hints. Just to get an idea about the corner cases that you mentioned: Would this be roughly something like
(with better wording, but along that line)? |
Sounds good to me. |
Resolved in #1997. |
No description provided.