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 panic on data slice accesses #358

Merged
merged 3 commits into from
Oct 2, 2022

Conversation

repi
Copy link
Contributor

@repi repi commented Oct 1, 2022

This could easily happen if the binary data for the glTF was too short or empty.

Ideally should return a Result and enum error for this rather than just Option::None but this is a pretty easy change at least and better than panicking.

Part of #359

This could easily happen if the binary data for the glTF was too short or empty.

Ideally should return a `Result` and enum error for this rather than just `Option::None` but this is a pretty easy change at least and better than panicking.
src/accessor/util.rs Outdated Show resolved Hide resolved
src/accessor/util.rs Outdated Show resolved Hide resolved
@IcanDivideBy0
Copy link
Collaborator

IcanDivideBy0 commented Oct 2, 2022

lgtm
Iter::new could benefit from a bit of refactoring to use more idiomatic rust and avoid some invariants option unwrap, but that's outside the scope of the PR

@IcanDivideBy0
Copy link
Collaborator

IcanDivideBy0 commented Oct 2, 2022

Can you please either allow edits from maintainers or cherry-pick d54a74a

repi and others added 2 commits October 2, 2022 10:31
@repi
Copy link
Contributor Author

repi commented Oct 2, 2022

thanks! resolved

@IcanDivideBy0 IcanDivideBy0 merged commit 1e5fc25 into gltf-rs:master Oct 2, 2022
@IcanDivideBy0
Copy link
Collaborator

thank you

@Jake-Shadle Jake-Shadle deleted the fix-slice-panics branch October 5, 2022 14:58
@repi repi mentioned this pull request Dec 2, 2022
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