Improved binary read and write support #7

vpenades · 2017-09-11T22:50:53Z

Something that was missing from the current interface was the ability to read the binary buffer chunk from a glb file, and the ability to write glb files.

Notice that with the current API, we need to open the file two times, because the json chunk and the binary chunk are read separately (as if they were separate files)

I added these functions, along with a test that reads and writes binary files.

Also, I added an additional check for the byte padding for the json chunk.

I noticed some glb files don't have any padding on the binary buffer chunk. I understand it's because it's the last chunk... but, if in the future more chunk types are added, this might be an issue.

Finally, I added a unit test for the new methods.

…er packages, so tests will show up on IDEs that don't have the NUnit extension installed.

lexaknyazev · 2017-09-12T13:55:12Z

glTFLoaderUnitTests/SampleModelsTest.cs

+
+                    var buffer = jsonChunk.Buffers[0];
+                    Assert.IsTrue(buffer.ByteLength <= binChunk.Length);
+                    // should we check padding as with jsonChunk? some reference files fail!


See KhronosGroup/glTF#1026. We should clarify spec on this issue.

Added a test that demonstrates how to load the buffers and the images for any gltf.

vpenades · 2017-09-12T18:58:01Z

I took the liberty to add two additional helper methods to help load buffers and images, regardless of how they're stored. These methods greatly simplify loading all the data of a gltf as shown in the unit test.

public static Stream OpenImageFile(this Gltf model, string gltfFilePath, int imageIndex)

public static Byte[] LoadBinaryBuffer(this Gltf model, string gltfFilePath, int bufferIndex)

Both methods work the same way, you pass the model, the file path from where the model was originally loaded, and the index of the buffer or image. Internally, they will handle embedded or buffered content, so how the content has been stored is transparent to the end user.

bghgary · 2017-09-12T19:46:21Z

glTFLoader/Interface.cs

+
+            if (buffer.Uri == null) return LoadBinaryBuffer(gltfFilePath);
+
+            if (buffer.Uri.StartsWith("data:application/octet-stream;base64"))


Did you mean to replace this with EMBEDDEDOCTETSTREAM?

bghgary · 2017-09-12T19:52:56Z

glTFLoader/Interface.cs

+            {
+                string content = null;
+
+                if (image.Uri.StartsWith(EMBEDDEDPNG)) content = image.Uri.Substring(EMBEDDEDPNG.Length + 1);


glTF only support PNG and JPG. None of the other formats are supported, so you don't need the other mime types.

https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#file-extensions-and-mime-types

bghgary · 2017-09-12T19:55:12Z

glTFLoader/Interface.cs

@@ -17,6 +17,13 @@ public static class Interface
        const uint JSON = 0x4E4F534A;
        const uint BIN = 0x004E4942;

+        const string EMBEDDEDOCTETSTREAM = "data:application/octet-stream;base64";


I would recommend adding the trailing comma to this constant so that you are actually checking for the comma instead of ignoring the character using + 1.

Okey, I'll do that change.

Btw, detecting embedded images is quite tricky and I'm aware there's room for improvement. For example, I'm sure some people will want to use hardware specific images, like DDS or ASTC , I don't know if there's specific mime types for these formats... in which case, is it possible to find images stored as data:application/octet-stream;base64 ?

There have been lots of discussions around this already, but the short answer, at least for core glTF 2.0, is that hardware specific textures are not supported. Thus, you don't have to worry about them for now.

Okey then.

About the Interface API, there's a few more methods I would like to add, but I don't know if you think it's too much already...

Usually, when I work with file formats, I try to make everything to work with Streams, certainly, it gets complicated when you have multiple files around. The use case is when you try to load a file from within a ZIP or any other package container (which is quite common to use in videogames)

The idea I had in mind was to replicate the previous methods used to load buffers and images, but using lambdas, so the end user could intercept/inject file accesses to whatever package container is using.

Let's do that in a separate change.

…ction

bghgary · 2017-09-12T20:37:32Z

glTFLoader/Interface.cs

-                    throw new NotImplementedException($"The first chunk must be format 'JSON': {chunkFormat}");
+                if ((chunkLength & 3) != 0)
+                {
+                    throw new NotImplementedException($"The second chunk must be padded to 4 bytes: {chunkLength}");


NotImplementedException seems weird for this. Perhaps use the InvalidDataException you are using elsewhere?

bghgary · 2017-09-12T20:37:43Z

glTFLoader/Interface.cs

+                uint chunkFormat = binaryReader.ReadUInt32();
+                if (chunkFormat != BIN)
+                {
+                    throw new NotImplementedException($"The second chunk must be format 'BIN': {chunkFormat}");


bghgary · 2017-09-12T20:42:27Z

glTFLoader/Interface.cs

+            fullLength += 8 + buffer.Length + binPadding;
+
+            binaryWriter.Write(GLTF);
+            binaryWriter.Write((UInt32)2);


nit: This might be good in a constant also

bghgary · 2017-09-12T20:58:52Z

glTFLoader/Interface.cs

                }

-                return Encoding.UTF8.GetString(binaryReader.ReadBytes((int)chunkLength));
+                uint chunkFormat = binaryReader.ReadUInt32();


This technically is incorrect. The spec does not explicitly say that the second chunk must be the BIN chunk. It says this:

https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#chunks

Client implementations must ignore chunks with unknown types.

This is to support extensions that add additional chunks to the file.

okey, going to change all that 👍

You also need to consider when the BIN chunk is not present.

bghgary · 2017-09-12T21:00:59Z

glTFLoader/Interface.cs

+            }
+        }
+
+        public static void SaveBinaryModel(this Gltf model, byte[] buffer, BinaryWriter binaryWriter)


Consider allowing buffer to be null which will then not write out the BIN chunk. BIN chunk is not always required for all glTF files.

bghgary

Overall, looks good.

Added support for writing glb files without binary chunk Added image and buffer loader funcions that use lambdas, to support loading gltf/glb from package files.

vpenades · 2017-09-12T21:47:33Z

Okey, I think all the changes are in place, I might think to add a few more unit tests, but so far, in terms of loading/saving, I've covered most offline scenarios.

lexaknyazev · 2017-09-12T21:47:53Z

glTFLoader/Interface.cs

+
+            if (buffer.Uri == null) return externalReferenceSolver(null);
+
+            if (buffer.Uri.StartsWith(EMBEDDEDOCTETSTREAM))


While spec says that binary buffers use application/octet-stream, some tools may start using application/gltf-buffer after IANA approval. See KhronosGroup/glTF#944.

We can fix this once the spec is updated.

Yeah, it's something easy to add

bghgary · 2017-09-12T21:51:26Z

glTFLoader/Interface.cs

+        {
+            string content = null;
+
+            if (image.Uri.StartsWith(EMBEDDEDPNG)) content = image.Uri.Substring(EMBEDDEDPNG.Length);


You still have more types than just PNG and JPG. Remove BMP, GIF, and TIFF here and the constants.

You mean, only PNG and JPG are supported?

Yes.

https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#imageuri

Well... that surprised me!, I guess it's a matter of time they include more formats

vpenades · 2017-09-12T22:51:26Z

I'll have a look at documenting some of my methods in a few days, time to bed!

realvictorprm · 2017-09-12T22:52:29Z

Good, if I have enough time I might do some tuff here too. Looks funny here 😃

vpenades · 2017-09-13T08:05:25Z

I've just noticed there's a bug I introduced in the unit tests... I've just created a new fork... I'll fix that one and also add some documentation... pull request soon

vpenades added 5 commits September 12, 2017 00:42

Improved binary read and write support

1ba801a

added missing padding for the binary chunk

1e8e4cf

Added missing padding check for the binary chunk

7f5b21d

JSON chunk must be padded with 0x20 white spaces, not zeroes

00b432f

Upgraded NUnit nuget packages from 3.7 to 3.8, also added NUnit Adapt…

3225eb8

…er packages, so tests will show up on IDEs that don't have the NUnit extension installed.

lexaknyazev reviewed Sep 12, 2017

View reviewed changes

Added more load methods to easily load buffers and images.

cc646fc

Added a test that demonstrates how to load the buffers and the images for any gltf.

bghgary reviewed Sep 12, 2017

View reviewed changes

Added a comma after mime type strings to improve embedded base64 dete…

bf2e3cb

…ction

bghgary reviewed Sep 12, 2017

View reviewed changes

bghgary requested changes Sep 12, 2017

View reviewed changes

Added support for custom binary chunks (by skipping them)

5344517

Added support for writing glb files without binary chunk Added image and buffer loader funcions that use lambdas, to support loading gltf/glb from package files.

lexaknyazev reviewed Sep 12, 2017

View reviewed changes

bghgary reviewed Sep 12, 2017

View reviewed changes

Removed BMP,TIFF,and GIF since they're not supported by the spec yet.

214c161

bghgary approved these changes Sep 12, 2017

View reviewed changes

bghgary merged commit cb1721e into KhronosGroup:master Sep 12, 2017

bghgary mentioned this pull request Sep 12, 2017

More Documentation #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved binary read and write support #7

Improved binary read and write support #7

vpenades commented Sep 11, 2017 •

edited

Loading

lexaknyazev Sep 12, 2017

vpenades commented Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017 •

edited

Loading

vpenades Sep 12, 2017

bghgary Sep 12, 2017

vpenades Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

vpenades Sep 12, 2017

bghgary Sep 12, 2017

bghgary Sep 12, 2017

bghgary left a comment

vpenades commented Sep 12, 2017

lexaknyazev Sep 12, 2017

bghgary Sep 12, 2017 •

edited

Loading

vpenades Sep 12, 2017

bghgary Sep 12, 2017

vpenades Sep 12, 2017

bghgary Sep 12, 2017

vpenades Sep 12, 2017

vpenades commented Sep 12, 2017

realvictorprm commented Sep 12, 2017

vpenades commented Sep 13, 2017


		if (buffer.Uri == null) return LoadBinaryBuffer(gltfFilePath);

		if (buffer.Uri.StartsWith("data:application/octet-stream;base64"))


		if (buffer.Uri == null) return externalReferenceSolver(null);

		if (buffer.Uri.StartsWith(EMBEDDEDOCTETSTREAM))

Improved binary read and write support #7

Improved binary read and write support #7

Conversation

vpenades commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

vpenades commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bghgary Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bghgary left a comment

Choose a reason for hiding this comment

vpenades commented Sep 12, 2017

Choose a reason for hiding this comment

bghgary Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpenades commented Sep 12, 2017

realvictorprm commented Sep 12, 2017

vpenades commented Sep 13, 2017

vpenades commented Sep 11, 2017 •

edited

Loading

bghgary Sep 12, 2017 •

edited

Loading

bghgary Sep 12, 2017 •

edited

Loading