diff --git a/CHANGELOG.md b/CHANGELOG.md index d590f662..56363ab3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ The `gltf` crate adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## Unreleased +### Fixed +- Fix `attemt to to subtract with overflow`-panic in `size_hint()` of sparse accessor when collecting items. +- Fix incorrect values returned from `size_hint()` in sparse accessor +- Add support to read items from sparse accessor without base buffer view + ## [1.4.0] - 2023-12-17 ### Added diff --git a/src/accessor/util.rs b/src/accessor/util.rs index 9629d4b5..b6d8c872 100644 --- a/src/accessor/util.rs +++ b/src/accessor/util.rs @@ -94,6 +94,11 @@ pub struct SparseIter<'a, T: Item> { /// This can be `None` if the base buffer view is not set. In this case the base values are all zero. base: Option>, + /// Number of values in the base accessor + /// + /// Valid even when `base` is not set. + base_count: usize, + /// Sparse indices iterator. indices: iter::Peekable>, @@ -110,11 +115,13 @@ impl<'a, T: Item> SparseIter<'a, T> { /// Here `base` is allowed to be `None` when the base buffer view is not explicitly specified. pub fn new( base: Option>, + base_count: usize, indices: SparseIndicesIter<'a>, values: ItemIter<'a, T>, ) -> Self { SparseIter { base, + base_count, indices: indices.peekable(), values, counter: 0, @@ -125,11 +132,17 @@ impl<'a, T: Item> SparseIter<'a, T> { impl<'a, T: Item> Iterator for SparseIter<'a, T> { type Item = T; fn next(&mut self) -> Option { - let mut next_value = self - .base - .as_mut() - .map(|iter| iter.next()) - .unwrap_or_else(|| Some(T::zero()))?; + let mut next_value = if let Some(base) = self.base.as_mut() { + // If accessor.bufferView is set we let base decide when we have reached the end + // of the iteration sequence. + base.next()? + } else if (self.counter as usize) < self.base_count { + // Else, we continue iterating until we have generated the number of items + // specified by accessor.count + T::zero() + } else { + return None; + }; let next_sparse_index = self.indices.peek(); if let Some(index) = next_sparse_index { @@ -145,7 +158,7 @@ impl<'a, T: Item> Iterator for SparseIter<'a, T> { } fn size_hint(&self) -> (usize, Option) { - let hint = self.values.len() - self.counter as usize; + let hint = self.base_count - (self.counter as usize).min(self.base_count); (hint, Some(hint)) } } @@ -300,6 +313,7 @@ impl<'a, 's, T: Item> Iter<'s, T> { } else { None }; + let base_count = accessor.count(); let indices = sparse.indices(); let values = sparse.values(); @@ -341,7 +355,7 @@ impl<'a, 's, T: Item> Iter<'s, T> { }; Some(Iter::Sparse(SparseIter::new( - base_iter, index_iter, value_iter, + base_iter, base_count, index_iter, value_iter, ))) } None => { diff --git a/tests/test_wrapper.rs b/tests/test_wrapper.rs index 8de7ccb7..ccba9f0c 100644 --- a/tests/test_wrapper.rs +++ b/tests/test_wrapper.rs @@ -22,3 +22,107 @@ fn test_accessor_bounds() { } ); } + +/// "SimpleSparseAccessor.gltf" contains positions specified with a sparse accessor. +/// The accessor use a base `bufferView` that contains 14 `Vec3`s and the sparse +/// section overwrites 3 of these with other values when read. +const SIMPLE_SPARSE_ACCESSOR_GLTF: &str = + "glTF-Sample-Models/2.0/SimpleSparseAccessor/glTF-Embedded/SimpleSparseAccessor.gltf"; + +#[test] +fn test_sparse_accessor_with_base_buffer_view_yield_exact_size_hints() { + let (document, buffers, _) = gltf::import(SIMPLE_SPARSE_ACCESSOR_GLTF).unwrap(); + + let mesh = document.meshes().next().unwrap(); + let primitive = mesh.primitives().next().unwrap(); + let reader = primitive + .reader(|buffer: gltf::Buffer| buffers.get(buffer.index()).map(|data| &data.0[..])); + let mut positions = reader.read_positions().unwrap(); + + const EXPECTED_POSITION_COUNT: usize = 14; + for i in (0..=EXPECTED_POSITION_COUNT).rev() { + assert_eq!(positions.size_hint(), (i, Some(i))); + positions.next(); + } +} + +#[test] +fn test_sparse_accessor_with_base_buffer_view_yield_all_values() { + let (document, buffers, _) = gltf::import(SIMPLE_SPARSE_ACCESSOR_GLTF).unwrap(); + + let mesh = document.meshes().next().unwrap(); + let primitive = mesh.primitives().next().unwrap(); + let reader = primitive + .reader(|buffer: gltf::Buffer| buffers.get(buffer.index()).map(|data| &data.0[..])); + let positions: Vec<[f32; 3]> = reader.read_positions().unwrap().collect::>(); + + const EXPECTED_POSITIONS: [[f32; 3]; 14] = [ + [0.0, 0.0, 0.0], + [1.0, 0.0, 0.0], + [2.0, 0.0, 0.0], + [3.0, 0.0, 0.0], + [4.0, 0.0, 0.0], + [5.0, 0.0, 0.0], + [6.0, 0.0, 0.0], + [0.0, 1.0, 0.0], + [1.0, 2.0, 0.0], // Sparse value #1 + [2.0, 1.0, 0.0], + [3.0, 3.0, 0.0], // Sparse value #2 + [4.0, 1.0, 0.0], + [5.0, 4.0, 0.0], // Sparse value #3 + [6.0, 1.0, 0.0], + ]; + assert_eq!(positions.len(), EXPECTED_POSITIONS.len()); + for (i, p) in positions.iter().enumerate() { + for (j, q) in p.iter().enumerate() { + assert_eq!(q - EXPECTED_POSITIONS[i][j], 0.0); + } + } +} + +/// "box_sparse.gltf" contains an animation with a sampler with output of two values. +/// The values are specified with a sparse accessor that is missing a base `bufferView` field. +/// Which means that each value in it will be 0.0, except the values contained in the sparse +/// buffer view itself. In this case the second value is read from the sparse accessor (1.0), +/// while the first is left at the default zero. +const BOX_SPARSE_GLTF: &str = "tests/box_sparse.gltf"; + +#[test] +fn test_sparse_accessor_without_base_buffer_view_yield_exact_size_hints() { + let (document, buffers, _) = gltf::import(BOX_SPARSE_GLTF).unwrap(); + + let animation = document.animations().next().unwrap(); + let sampler = animation.samplers().next().unwrap(); + let output_accessor = sampler.output(); + let mut outputs_iter = + gltf::accessor::Iter::::new(output_accessor, |buffer: gltf::Buffer| { + buffers.get(buffer.index()).map(|data| &data.0[..]) + }) + .unwrap(); + + const EXPECTED_OUTPUT_COUNT: usize = 2; + for i in (0..=EXPECTED_OUTPUT_COUNT).rev() { + assert_eq!(outputs_iter.size_hint(), (i, Some(i))); + outputs_iter.next(); + } +} + +#[test] +fn test_sparse_accessor_without_base_buffer_view_yield_all_values() { + let (document, buffers, _) = gltf::import(BOX_SPARSE_GLTF).unwrap(); + + let animation = document.animations().next().unwrap(); + let sampler = animation.samplers().next().unwrap(); + let output_accessor = sampler.output(); + let output_iter = gltf::accessor::Iter::::new(output_accessor, |buffer: gltf::Buffer| { + buffers.get(buffer.index()).map(|data| &data.0[..]) + }) + .unwrap(); + let outputs = output_iter.collect::>(); + + const EXPECTED_OUTPUTS: [f32; 2] = [0.0, 1.0]; + assert_eq!(outputs.len(), EXPECTED_OUTPUTS.len()); + for (i, o) in outputs.iter().enumerate() { + assert_eq!(o - EXPECTED_OUTPUTS[i], 0.0); + } +}