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

Allow BufferView indices to be unspecified. #456

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

haroonq
Copy link
Contributor

@haroonq haroonq commented Oct 5, 2023

Allow BufferView indices for element array buffers to be unspecified to support some extensions.

Note that this is similar to how invalid array buffers are handled in order to support, for example, sparse morph targets.

@syoyo
Copy link
Owner

syoyo commented Oct 5, 2023

Thanks!

Allow BufferView indices for element array buffers to be unspecified to support some extensions

I need to confirm this fulfills glTF spec. Could you please point to glTF spec page https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html describing about it?

Also, do you have minimal reproducible glTF scene?

@syoyo
Copy link
Owner

syoyo commented Oct 5, 2023

FYI this PR breaks unit test

https://github.com/syoyo/tinygltf/actions/runs/6417703978/job/17440742346?pr=456

tester.cc:177: FAILED:
  REQUIRE_THAT( err, Catch::Contains("accessor[0] invalid bufferView") )
with expansion:
  "" contains: "accessor[0] invalid bufferView"

@haroonq
Copy link
Contributor Author

haroonq commented Oct 6, 2023

I believe that draco-compressed GLTFs will exhibit this issue. For example, glTF-Draco/Box.gltf

If you compare that asset with the "normal" GLTF glTF/Box.gltf, you'll see that it doesn't specify a bufferView value in any of its accessors.

I believe this behaviour is described in the KHR_draco_mesh_compression specification.

I realize that tinygltf does support dracro decompression (if you define TINYGLTF_ENABLE_DRACO), but for my use-case, I have my own implementation for handling the extension that I would like to use. However, because tinygltf treats an unspecified index bufferView as an error, it stops parsing the rest of the file, preventing me from decompressing my own buffers.

(Specifically, I would like all the decoded vertex data to be stored in a single, contiguous buffer, whereas tinygltf will decompress each attribute into its own separate buffer.)

Finally, it's also worth noting from the main spec that index buffers can also be sparse buffers. All I've done is copy the same logic for the vertex buffers to apply to the index buffers as well (See commit: e4132167222.)

@syoyo
Copy link
Owner

syoyo commented Oct 6, 2023

@haroonq Thank you for the info!

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#reference-accessor

Confirmed Accessor.bufferView is not a required attribute.

So do not report an error when Accessor.bufferView is -1.

Unit test failure is caused by bufferView index out-of-range, so the logic will be

if (bufferView < 0) {
  // skip
} else if (bufferView < bufferViews.size()) {
  // ok
} else {
  // report error
}

Allow BufferView indices for element array buffers to be unspecified to support some extensions.

Note that this is similar to how invalid array buffers are handled in order to support, for example, sparse morph targets.
@haroonq
Copy link
Contributor Author

haroonq commented Oct 9, 2023

Thank you! I've updated the request with your suggestions.

@syoyo syoyo merged commit 5e8a7fd into syoyo:release Oct 10, 2023
8 checks passed
@syoyo
Copy link
Owner

syoyo commented Oct 10, 2023

Thanks! Merged!

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

Successfully merging this pull request may close these issues.

2 participants