search

KhronosGroup / gltf-asset-auditor

Check glTF file attributes against commerce application use case requirements
Apache License 2.0
20 stars 3 forks source link

Bugfix/gltf file loading #7

Closed deep9888 closed 1 year ago

deep9888 commented 1 year ago

Like gltf files are not loading with gltf-asset-auditor, I found a bug in src -> Glb.ts file. On line no. 317, the issue was with DataView.setUInt32 writes more bytes to the buffer and uses more memory. After some debugging. I replace DataView.setUInt32 with DataView.setUInt8 because gltf files contain RGB colourspace textures (also known as 24-bit RGB) and require 8 bits (or 1 byte) per channel for a total of 24 bits (or 3 bytes) per texel (pixel in texture). And why are you using setUnit32, and do you know if there is any other change I need to consider? After that, the DataView bug is resolved, and gltf files are correctly loaded.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

MikeFesta commented 1 year ago

Nice Find! I think I overlooked that because I was working with 32-bit RGBA textures. Your discovery makes sense that some 24-bit for RGB image files could end up with the wrong length, especially when they are not power of 2 dimensions. It would also impact 8-bit grayscale images. Loading the data in 8-bits at a time ensures that the length of the data read will always align. Can you update the pull request to only include that one file change (Glb.ts)? Currently there are 4 other files changed in the web-example, including the removal of index.html.

deep9888 commented 1 year ago

I changed it. Can you please check it now?

deep9888 commented 1 year ago

No issue!

MikeFesta commented 1 year ago

I just pushed the changes to npm with version 1.0.3