Make byte_offset optional for sparse accessors #381
Conversation
This fixes validation of sparse accessors without buffer_views, which must not have a byte_offset. This is not explicit in the spec but is in the official validator (see KhronosGroup/glTF-Validator#207)
src/accessor/mod.rs
Outdated
| self.json.byte_offset as usize | ||
| /// | ||
| /// This may be `None` if the corresponding accessor is sparse. | ||
| pub fn offset(&self) -> Option<usize> { |
There was a problem hiding this comment.
I think that's fine by me. But this PR already breaks semver for gltf-json. Let me know how you would like to proceed and I'll make the adjustment.
Edit: I should add, that if we do return zero, we should probably still change it to return Option<usize> for the next version.
There was a problem hiding this comment.
@alteous any decision on this? I'm happy to make the change, otherwise feel free to make changes to this PR directly.
There was a problem hiding this comment.
I'm not so concerned about breaking semver for gltf-json. It's not the first time this has happened. As long as the top-level gltf crate is unaffected. I suggest we go with the 'return zero' approach for now.
Edit: I should add, that if we do return zero, we should probably still change it to return Option for the next version.
I agree with you. This is something that could be missed easily. I'm not how that could be prevented easily though.
|
Well spotted and thanks for the PR. |
This change depends on landing a change in gltf-rs: gltf-rs/gltf#381
This is to avoid breaking semver for gltf
|
Should be good to go, @alteous. |
This fixes validation of sparse accessors without buffer_views, which must not have a byte_offset. This is not explicit in the spec but is in the official validator (see KhronosGroup/glTF-Validator#207)