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

[binary_glTF] First Version #400

Merged
merged 25 commits into from
Sep 15, 2015

Conversation

mlimper
Copy link
Contributor

@mlimper mlimper commented Aug 21, 2015

Hi all binary_glTF contributors and maintainers :-)

This first pull request contains our modifications on the binary file layout of binary glTF, as done for our SRC format. The changes can be previewed here:
https://github.com/mlimper/glTF/blob/header-preamble-naming/extensions/binary_glTF/README.md

Changes:

  • renamed from CESIUM_binary_glTF to binary_glTF
  • Added contributors and references to SRC resources
  • Changed wording for file layout: preamble, header, body
  • Added entry for header format in preamble, added note that currently only JSON is allowed (value '0')
  • Made header directly follow the preamble (removed jsonOffset field), added some sentence on that

Of course, all of these points are open for discussion right now (maybe directly inside this pull request?).


## Resources

* Discussion - [#357](https://github.com/KhronosGroup/glTF/issues/357)
* base64-encoded data in glTF - [#68](https://github.com/KhronosGroup/glTF/issues/68)
* [Faster 3D Models with Binary glTF](http://cesiumjs.org/2015/06/01/Binary-glTF/) article on the Cesium blog
* SRC project page (paper, background, basic writer) - [http://x3dom.org/src/](http://x3dom.org/src/)

<a name="BenchData">
* [1] Raw data for benchmarks using compression available in [BenchData](BenchData/README.md) supplemental.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tweak BenchData/README.md to say that the tests were done with CESIUM_binary_glTF, but the difference is insignificant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, I added a remark at the end of the "Experimental Results" section:
https://github.com/mlimper/glTF/tree/header-preamble-naming/extensions/CESIUM_binary_glTF#SRCPaper

@pjcozzi
Copy link
Member

pjcozzi commented Aug 25, 2015

Made header directly follow the preamble (removed jsonOffset field), added some sentence on that

@tfili can you confirm that we are OK with removing the jsonOffset field? We originally added it to ease the tool implementation, but I believe we were able to workaround needing it. Right?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 25, 2015

Added entry for header format in preamble, added note that currently only JSON is allowed (value '0')

Do you think we still need this in the context of glTF? What else would the header be?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 25, 2015

Just those comments. It is awesome how remarkably similar our approaches are!

@tfili
Copy link

tfili commented Aug 26, 2015

@pjcozzi We didn't still needed the offset and length in our extension. Making the json first should let us remove that.

@pjcozzi pjcozzi mentioned this pull request Aug 27, 2015
@pjcozzi pjcozzi mentioned this pull request Aug 27, 2015
@pjcozzi
Copy link
Member

pjcozzi commented Aug 27, 2015

Made header directly follow the preamble (removed jsonOffset field), added some sentence on that

@mlimper do you have an elegant approach for assigning the glTF bufferview.byteOffset property with this approach since the byte offset depends on the size of the glTF, which varies depending on the number of characters needed for the byteOffset properties. Do you add padding?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 27, 2015

@mlimper we're also not sure that we should call the glTF section the header. AFAIK a file header usually comes first. Is preamble/header widely used? Can you point me to some examples?

@mlimper
Copy link
Contributor Author

mlimper commented Aug 31, 2015

Added entry for header format in preamble, added note that currently only JSON is allowed (value '0')
Do you think we still need this in the context of glTF? What else would the header be?

In the paper we mentioned binary JSON (http://bsonspec.org/) and protocol buffers.
We felt that having something different than JSON was not necessary in the first working implementation. But there have been requests and discussions, from the very beginning, to keep the possibility of having different, more efficient header encodings. Therefore, I think it would make sense to have this possibility, and I would vote for allowing only JSON in the first version.

@mlimper
Copy link
Contributor Author

mlimper commented Aug 31, 2015

@mlimper we're also not sure that we should call the glTF section the header. AFAIK a file header usually comes first. Is preamble/header widely used? Can you point me to some examples?

There is something called header in BMP, for example:
https://en.wikipedia.org/wiki/BMP_file_format#Bitmap_file_header

Wikipedia's definition:
https://en.wikipedia.org/wiki/Header_(computing)

The word "preamble" for the "pre-header" was proposed by Leonard in his SRC spec draft, and I liked it, so I just used it in this initial pull request.

We are totally open for other proposals. Initially, we just called the glTF-like information "header" as we thought about it as a bit of additional information about the actual data, which was called the "body". We were very focused on transporting raw data (buffers for mesh geometry, plus textures) and the JSON really just served as a description of the data. We have different JSON formats (or X3D) to describe the scene data.
This is a conceptual difference between glTF and SRC: With glTF, it is possible to describe a whole scene, including lighting, node hierarchy, etc. inside the JSON - with this in mind, calling the JSON part just "header" really sounds a bit weird, I agree.

In the CESIUM_binary_glTF proposal, what we called "preamble" is called "header":
https://github.com/KhronosGroup/glTF/blob/new-extensions/extensions/CESIUM_binary_glTF/layout.png

Should I just change my fork to go back to this original idea? Then the question would be how the structured glTF data ("JSON part") is called. I don't like calling it JSON because I would like to keep the possibility to use different (text or binary) encodings later on. Any ideas?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 31, 2015

Added entry for header format in preamble, added note that currently only JSON is allowed (value '0')

Do you think we still need this in the context of glTF? What else would the header be?

In the paper we mentioned binary JSON (http://bsonspec.org/) and protocol buffers.

We felt that having something different than JSON was not necessary in the first working implementation. But there have been requests and discussions, from the very beginning, to keep the possibility of having different, more efficient header encodings. Therefore, I think it would make sense to have this possibility, and I would vote for allowing only JSON in the first version.

Ah, I suppose msgpack is another option; however, I would hesitate to use anything that is decoded in JavaScript instead of by the browser.

How would this work when adding a new format? Wouldn't we have to release a new binary glTF extension with a new version? Instead of overbuilding this extension, why not wait until we have an implementation that shows the benefit of multiple formats, and then introduce it with a new version of the binary glTF extension?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 31, 2015

Should I just change my fork to go back to this original idea? Then the question would be how the structured glTF data ("JSON part") is called. I don't like calling it JSON because I would like to keep the possibility to use different (text or binary) encodings later on. Any ideas?

Open to suggestions, but perhaps [header][scene][body]?

@mlimper
Copy link
Contributor Author

mlimper commented Sep 2, 2015

How would this work when adding a new format? Wouldn't we have to release a new binary glTF extension with a new version? Instead of overbuilding this extension, why not wait until we have an implementation that shows the benefit of multiple formats, and then introduce it with a new version of the binary glTF extension?

I think the main argument for introducing the format word with the very first version of binary_glTF would be that old .bgltf data, and respective parsers, can remain unchanged. Backwards compatibility with older parsers is probably important.

The alternative would be that we define "Binary glTF version 1.0 uses JSON and has no word for specifying a different header encoding", but if we define such a word later on, parsers have to adapt to a different header size for this.

Clearly both is possible, I would personally vote for an explicit "header format" word right from the beginning.

@mlimper
Copy link
Contributor Author

mlimper commented Sep 2, 2015

@pjcozzi
Copy link
Member

pjcozzi commented Sep 2, 2015

I think the main argument for introducing the format word with the very first version of binary_glTF would be that old .bgltf data, and respective parsers, can remain unchanged. Backwards compatibility with older parsers is probably important.

Ah, I see, so it would allow current Binary glTF content to be forward compatible with a future Binary glTF version assuming we don't change the header for some other reason.

OK with me.

Is there anything else to discuss for this? Have you implemented it?

@mlimper
Copy link
Contributor Author

mlimper commented Sep 8, 2015

No, we haven't implemented the current proposal right now. It should not be too much work to adjust our existing implementation. It seems for now there are no open points. I'll use "header-scene-body" for the proposal now, and we'll have a "scene format" entry inside the header.

We will now adjust the implementation. We can provide a binary_glTF example file along with this pull request - the ultimate goal would then probably be that we can export this and you can load it, and vice versa ;-)

@pjcozzi
Copy link
Member

pjcozzi commented Sep 8, 2015

All sounds good. @tfili and I will start work on the Cesium loader ASAP.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

@mlimper I added the schema in dd6fcf9. Please link to it from the README.md.

@mlimper
Copy link
Contributor Author

mlimper commented Sep 15, 2015

Here's a first updated version of the README.md for review.

One TODO / question:
Should there be a more sophisticated way of specifying the version, such as major-minor-maintenance, or major-minor? Maybe major-minor as two 16 bit values?
The current proposal just uses a uint32 to specify a single unsigned integer.

@mlimper
Copy link
Contributor Author

mlimper commented Sep 15, 2015

.. ah, and now I also updated the mimetype to model/gltf+binary, but of course this can be changed after we got more feedback

@mlimper
Copy link
Contributor Author

mlimper commented Sep 15, 2015

... one more thing, do you have an up-to-date example file (glTF or binary_glTF) that uses the upcoming glTF 1.0 schema? Just looking for a very simple example (collada duck, stanford bunny or so) to ease my writer implementation.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

@mlimper looks great. I made minor edits in ddf0452 and 3155257. I'm going to do one final read shortly.

Should there be a more sophisticated way of specifying the version, such as major-minor-maintenance, or major-minor? Maybe major-minor as two 16 bit values?
The current proposal just uses a uint32 to specify a single unsigned integer.

I think it is total overkill for this.

one more thing, do you have an up-to-date example file (glTF or binary_glTF) that uses the upcoming glTF 1.0 schema? Just looking for a very simple example (collada duck, stanford bunny or so) to ease my writer implementation.

Not yet, but I plan to write a 0.8 to 1.0 spec guide (#406). The best thing to look at now is the diff of the schemas.

However, there is more very minor changes to come.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

I made some more minor edits in 69b48c4.

I am going to merge this so we can reduce the number of branches we have as I finalize the writeup for extensions (#404) in the spec-extensions branch.

In the near term, future pull requests for this extension can be opened into that branch, and then into spec-1.0 once we merge spec-extensions - hopefully tonight.

Thanks for your contribution, @mlimper. This is better than the initial CESIUM_binary_glTF extension. I'm looking forward to working with you on more extensions with you.

pjcozzi added a commit that referenced this pull request Sep 15, 2015
@pjcozzi pjcozzi merged commit 1200e2e into KhronosGroup:new-extensions Sep 15, 2015
@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

@mlimper we usually delete branches after we merge them, but I'll leave it up to you to delete your header-preamble-naming branch.

@shunter
Copy link

shunter commented Sep 15, 2015

@shunter any chance you can advise on the MIME type naming?

The proposed new types seem logical to me. They would need to be registered with IANA, however. vnd is not necessarily a problem - e.g. Collada uses model/vnd.collada+xml as the official type.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

Thanks @shunter. @mlimper do you or any of your collaborators have experience registering MIME types?

@pjcozzi
Copy link
Member

pjcozzi commented Sep 15, 2015

@mlimper @tfili I added the following in 112a232:

The start of body is 4-byte aligned to ease its use with JavaScript Typed Arrays.

This was in the original CESIUM_binary_glTF extension. I think we still want it. See #167.

@pjcozzi pjcozzi mentioned this pull request Sep 15, 2015
8 tasks
@mlimper mlimper deleted the header-preamble-naming branch September 16, 2015 08:14
@mlimper
Copy link
Contributor Author

mlimper commented Sep 16, 2015

@mlimper @tfili I added the following in 112a232:

The start of body is 4-byte aligned to ease its use with JavaScript Typed Arrays.
This was in the original CESIUM_binary_glTF extension. I think we still want it. See #167.

Oh yeah, sorry, after checking #167 and wondering why we didn't have this problem, I figured out in our code we only create and use UInt8 TypedArray views, which we then directly pass to the GPU. We never used Float32Arrays or something like that in JS when dealing with GPU buffers.
For the general case, it makes sense to have the body aligned to 4 byte boundaries.

@mlimper
Copy link
Contributor Author

mlimper commented Sep 16, 2015

Thanks @shunter. @mlimper do you or any of your collaborators have experience registering MIME types?

Me not, but I'll ask around

@pjcozzi
Copy link
Member

pjcozzi commented Sep 16, 2015

All good, thanks.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 16, 2015

@mlimper I added a clarification about the 4-byte alignment in 550d14f based on feedback from @tfili, who is currently implementing it.

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

Successfully merging this pull request may close these issues.

5 participants