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

Fix logical errors in byteStride calculations #751

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

Yeah-Zi
Copy link
Contributor

@Yeah-Zi Yeah-Zi commented Nov 7, 2023

Fixed loading failure issue when byteStride description is 0 in buffer view

@kring
Copy link
Member

kring commented Nov 7, 2023

@Yeah-Zi can you explain more why this is needed? If a bufferView has byteStride explicitly specified to be zero, that's a broken glTF. The value is required to be greater than or equal to 4:
https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#_bufferview_bytestride

@Yeah-Zi
Copy link
Contributor Author

Yeah-Zi commented Nov 7, 2023

Thank you for providing the standard format specification for gltf.
Why I need this is because the customer provides data with a byteStride of 0, it can be loaded normally in CesiumJS, while Cesium For Unreal cannot

@Yeah-Zi
Copy link
Contributor Author

Yeah-Zi commented Nov 7, 2023

@kring Perhaps some log information can be added here, as loading failure does not result in any errors. It took me some time to debug this error

@Yeah-Zi
Copy link
Contributor Author

Yeah-Zi commented Nov 7, 2023

@kring
Copy link
Member

kring commented Nov 7, 2023

Perhaps some log information can be added here, as loading failure does not result in any errors.

I'm not opposed to adding some more logging but, in general, cesium-native is a loader, not a validator. A proper glTF validator (like this one: https://github.khronos.org/glTF-Validator/) should easily detect invalid glTF files like this.

However, in this case, since your change matches what CesiumJS is already doing, and it has very little cost, I'm happy to merge it. Can you please sign the Contributor License Agreement first:
https://github.com/CesiumGS/cesium/blob/master/CONTRIBUTING.md#contributor-license-agreement-cla

Thank you for the PR!

@Yeah-Zi
Copy link
Contributor Author

Yeah-Zi commented Nov 7, 2023

@kring I have signed the CLA

@kring
Copy link
Member

kring commented Nov 7, 2023

Thanks @Yeah-Zi!

@kring kring merged commit d3a2bbf into CesiumGS:main Nov 7, 2023
7 checks passed
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