Conversation
|
Please include the minimal logic(pseudo code) of your proposal. |
Not sure what you mean, the code in this PR is the minimal, working code. |
|
Standalone description of the logic. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for placeholder buffers to enable serialization of glTF files using the EXT_meshopt_compression extension. The extension requires a fallback buffer that exists in the JSON with only a byteLength property (representing uncompressed data size) but no actual binary content or URI.
Changes:
- Added
placeholderByteLengthfield to the Buffer struct to store the size for placeholder buffers - Added
SerializeGltfBufferPlaceholderfunction to serialize buffers with only byteLength (and optional metadata) - Updated buffer serialization logic in both
WriteGltfSceneToStreamandWriteGltfSceneToFileto detect and handle placeholder buffers
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I've added tests for both GLTF and GLB outputs and made the comments a bit clearer. The pseudo code logic is: |
|
Thanks! Let me give time to review the PR |
|
Any update on this @syoyo ? |
|
Recently I have improved CI build with better testing, and CI passes,so code should be ok. Could you prepare minimal example gltf/glb file of Placeholder buffer use?(not in the tester.cc code) And put files under models/ directory(https://github.com/syoyo/tinygltf/tree/release/models) |
Adds a way to serialize out a buffer declaration with a buffer size, without writing the actual buffer content. Needed for writing files that use the EXT_meshopt_compression extension
9456057 to
665b20b
Compare
I've pushed a model that was exported with the new feature. So the GLB contains two buffer declarations (buffer 0 and buffer 1), but only actually uses one of them (buffer 0). Buffer 1 has a size, but isn't part of the GLB file, as desired. Of course placeholder buffers really only make sense when used with extensions (such as meshopt compression). |
|
@syoyo do you need anything else from me here? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds a way to serialize out a buffer declaration with a buffer size, without writing the actual buffer content. Needed for writing files that use the
EXT_meshopt_compressionextension.This is only a proposal and frankly I have no idea how to make the interface for this functionality nice. It works, but I wouldn't mind if the final interface looks entirely different.
The problem is the following: When someone wants to write files that use the EXT_meshopt_compression extension, this is currently not possible with tinygltf.
The reason is, that for the extension to work, you need to write out 2 buffers. Buffer 0 with the actual compressed data, but also buffer 1. Buffer 1 is a dummy placeholder, whose sole purpose is to have a valid size, ie the size of the uncompressed data. Now buffer views will point to that buffer 1 with their uncompressed offsets and sizes, and thus validation code will determine that these offsets and sizes are correct. For this, buffer 1 does not need to actually exist, it just needs to have the correct size.
The extension then adds additional information about the actual byte offsets into the compressed buffer 0. This is described in some more detail here: https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Vendor/EXT_meshopt_compression/README.md#fallback-buffers
Currently tinygltf always writes all buffers to file. So if I create buffer 1 with the proper size, it will write those bytes out, which then doesn't compress the file, but rather increases its size.
If you have a better suggestion for how to support this, please let me know. If you don't want to support this use case, that's also fine, then I'll have to continue to use a custom fork.