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

.glTF / .bin mismatches? #560

Closed
jbherdman opened this issue Apr 7, 2016 · 10 comments
Closed

.glTF / .bin mismatches? #560

jbherdman opened this issue Apr 7, 2016 · 10 comments

Comments

@jbherdman
Copy link

I've been reading the spec, and it strikes me as fairly dangerous that the '.bin' files are effectively unstructured. What happens when somebody accidentally deploys a '.gltf' file alongside the wrong/mismatched '.bin' file? Or, perhaps more likely, one of the files gets updated with a new version (say, with different vertex-counts), but the other one doesn't? Am I missing some mechanism to detect that sort of mismatch, or has this just not been considered?

@pjcozzi
Copy link
Member

pjcozzi commented Apr 7, 2016

The structure of the .bin is defined by the .gltf so the app generating the asset has maximum flexibility, e.g., if they want to organize data for streaming.

In practice, I have yet to hear about users running into the mismatch you describe. If it is a concern, binary data can be embedded in data uris inside the .gltf or the binary glTF extension can be used to pack everything into one asset, KHR_binary_glTF.

@jbherdman
Copy link
Author

I doubt that users would ever "know" that they ran into this kind of mismatch, because it would be hard for them to detect and diagnose, which is my concern. In small-scale production or testing, it would probably manifest itself as the user seeing "something weird", which went away the next time they ran collada2gltf (or other tool) to generate their glTF data-set. It is also the sort of issue that would be hard to reproduce, and thus might never get reported.

In terms of "maximizing flexibility", is there a use case that you're considering where a simple "chunk-based" structure to the '.bin' format would be insufficient? By "chunk-based", I mean a format where some of the structural data like buffer length (and possibly type, or other buffer / bufferView meta-data) could be embedded within the '.bin' file itself.

At a minimum, what about some mechanism for embedding the 'buffer length' as the first 4 bytes of data inside the data referenced by a "buffer" element, and then having readers/consumers cross-check the buffer-length "expected" via the '.gltf'/JSON vs the "actual" buffer-length stored in the '.bin'? That strikes me as the bare minimum to validate that the buffer-data is somewhere in the right ballpark. I also suspect it might be more useful to support that on the 'bufferView' level instead of, or in addition to, the 'buffer' level?

@pjcozzi
Copy link
Member

pjcozzi commented Apr 12, 2016

At a minimum, what about some mechanism for embedding the 'buffer length' as the first 4 bytes of data inside the data referenced by a "buffer" element, and then having readers/consumers cross-check the buffer-length "expected" via the '.gltf'/JSON vs the "actual" buffer-length stored in the '.bin'?

If a mismatch becomes a problem in practice, we could add this to the spec in a backwards compatible way.

In the meantime, your loader could include a header in the buffer that you could validate with. Since this header could be skipped by all buffer views, the asset would still confirm to the spec; I don't believe that the spec indicates that the entire buffer needs to be accessed by buffer views.

@jbherdman
Copy link
Author

I'm coming at this problem from the opposite direction, involved with a software company looking to add "glTF export" to our list of features.

Even with the "glTF Validator" project that Khronos Group is funding, there will be no way to detect this sort of problem, because the necessary data simply doesn't exist in the .bin file. If it's not an official part of the spec, then no third-party loaders are going to be able to validate and detect this sort of mismatch problem.

Our major concern is that this is a "land mine" just waiting to consume hours and hours of our tech support. The instant that a user gets into a mismatch situation, our tech support would have no way to determine whether the problem is caused via file-mismatch or whether there is an actual problem in our exporter that needs to be fixed.

From my own experience, I know that this sort of mismatch is inevitably going to happen once the user base grows large enough. Why not head off this issue now? The longer you wait, the more loaders and exporters are going to exist "in the wild", and they may never update to the improved/updated spec.

@lexaknyazev
Copy link
Member

@jbherdman There is a byteLength property in a buffer object. Actual file size is almost always known from the HTTP response or file system. So why do you think that there is no way for loader to detect file mismatch?

@jbherdman
Copy link
Author

@lexaknyazev If I'm reading the spec properly, the byteLength property of a buffer object is optional. Even if the byteLength is present, there is no way for a loader to validate whether the '.bin' file follows the expected structure/layout.

For example, if an exporter happens to generate the same size output '.bin' file, but with a different internal structure, then there would be no way to detect that type of mismatch. This might sound unlikely, but all it would take is something subtle in the source data file to change in such a way that only the order of nodes being written out changed. Or, perhaps an advanced exporter might have an option to 'stride' vs 'not stride' the various positions/normals/etc within a mesh. In either case, the '.bin' file being output would likely have the same size, but a radically different internal structure, and there would be no way to detect the potential '.gltf'/'.bin' mismatch.

On the other hand, directly checking the buffer.byteLength may result in undesired behaviour when dealing with texture data. I mention this because it strikes at the heart of the deeper issue. A referenced texture-file is essentially "stand-alone"; it has all of its necessary data inside itself, so that it forms a single "unit" that can be replaced on-the-fly as needed. It is, and should remain, perfectly valid to substitute one texture file for another, and not have the '.gltf' file care at all. In the 3D world, this also happens with, say, the LWS/LWO file format (LWS is the 'scene' file, referencing 'LWO' files which each contain the entire geometry description for a single mesh), or various CAD assembly + part file structures. This is very different from the '.gltf'/'.bin' split, where the two files are very tightly coupled, and the '.bin' file is missing any sense of its own structure/layout.

@lexaknyazev
Copy link
Member

buffer.byteLength is 0 by default so I think it is actually required (as well as bufferView.byteLength). Otherwise we can't be sure about accessor's validity.
@pjcozzi What does zero-length buffer/bufferView mean?

Btw, image hasn't byteLength at all. So individual textures in separate files can be substituted without touching .gltf.

And speaking of the .gltf/.bin consistency, I think it can be achieved by external means. For example the .bin filename can contain some hash of its internal structure, so .gltf will have to reference a new file with each structure change.

@pjcozzi pjcozzi added the 2.0 label Apr 25, 2016
@pjcozzi
Copy link
Member

pjcozzi commented Apr 25, 2016

buffer.byteLength is 0 by default so I think it is actually required (as well as bufferView.byteLength). Otherwise we can't be sure about accessor's validity.
@pjcozzi What does zero-length buffer/bufferView mean?

This doesn't seem right to me. I think it should be required. @tparisi do you know?

I labeled this as 1.0.1 for a minor spec update.

@jbherdman you may have already considered this, but you could use embedded resources or the binary glTF extension to just write out a single file and not worry about any mismatch.

@pjcozzi
Copy link
Member

pjcozzi commented Jun 1, 2016

In fa86971, we made buffer.byteLength and bufferView.byteLength required in the 1.0.1 branch. Is there anything else we want to consider in this issue for the 1.0.1 spec update?

@pjcozzi
Copy link
Member

pjcozzi commented Jun 15, 2017

Updated in #826

@pjcozzi pjcozzi closed this as completed Jun 15, 2017
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

3 participants