Merge pull request #411 from EmbarkStudios/derwiath/issue-313

Fix size hint and values read with sparse iterator

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

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<ItemIter<'a, T>>,
 
+    /// Number of values in the base accessor
+    ///
+    /// Valid even when `base` is not set.
+    base_count: usize,
+
     /// Sparse indices iterator.
     indices: iter::Peekable<SparseIndicesIter<'a>>,
 
@@ -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<ItemIter<'a, T>>,
+        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<Self::Item> {
-        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<usize>) {
-        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 => {

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::<Vec<_>>();
+
+    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::<f32>::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::<f32>::new(output_accessor, |buffer: gltf::Buffer| {
+        buffers.get(buffer.index()).map(|data| &data.0[..])
+    })
+    .unwrap();
+    let outputs = output_iter.collect::<Vec<_>>();
+
+    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);
+    }
+}