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

Fixed handling of TypedArrays #69

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Conversation

henrikbuchholz
Copy link
Collaborator

Problem:
When using gltf writer, some meshes appeared "imploded" like this - and were fully rejected by some standard gltf viewers:
image

Reason:
The code to write BufferViews ignored the byteOffset/byteLength values of the index array. Therefore, if a TypedArray had a byteOffset>0 or byteLength!=buffer.byteLength, the typed array was incorrectly replaced by the whole ArrayBuffer containing it. This could mess up geometry, e.g. by introducing random indices.

Solution:
Add byteOffset and byteLength to various code sections to avoid this problem.

After the fix, the exported mesh looks correct again:

image

Reason:
The code to write BufferViews ignored the byteOffset/byteLength values of the index array. Therefore, if a TypedArray had a byteOffset>0 or byteLength!=buffer.byteLength, the typed array was incorrectly replaced by the whole ArrayBuffer containing it.
This could mess up geometry, e.g. by introducing random indices.
@petrbroz
Copy link
Owner

You're the best, thank you @henrikbuchholz!

Btw I noticed that you fixed the byteOffset/byteLength in the createMeshGeometry method and in the createLineGeometry but not in the createPointGeometry. Any specific reason for that?

@petrbroz
Copy link
Owner

Alright, no reason, just in a hurry :) Let me merge this PR and fix the createPointGeometry method in the same way.

@petrbroz petrbroz merged commit 1527c64 into develop Jun 28, 2023
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