From edda14743f5f10c7452034de077cf765f84566e4 Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 07:29:50 +0100 Subject: [PATCH 1/7] Add test for size hint on sparse accessor Currently this fails due to a faulty size_hint() implementation in SparseIter. For the test asset it claims that 3 items will be read from the iterator, when the correct answer is 14. 3 is the number of positions overriden by the sparse values, while the total number of positions is 14. The bug itself will be fixed in an upcoming change. --- tests/test_wrapper.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/test_wrapper.rs b/tests/test_wrapper.rs index 8de7ccb7..3e83b4d8 100644 --- a/tests/test_wrapper.rs +++ b/tests/test_wrapper.rs @@ -22,3 +22,26 @@ 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(); + } +} From d09cbcb918d93122774e5edd335bb068de166277 Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 14:13:12 +0100 Subject: [PATCH 2/7] Add test for values read in sparse accessor The `collect()` in this test currently triggers a `attempt to subtract with overflow` panic inside `SparseIter::size_hint()`. This will be fixed in a following commmit. --- tests/test_wrapper.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_wrapper.rs b/tests/test_wrapper.rs index 3e83b4d8..c2ae4af9 100644 --- a/tests/test_wrapper.rs +++ b/tests/test_wrapper.rs @@ -45,3 +45,37 @@ fn test_sparse_accessor_with_base_buffer_view_yield_exact_size_hints() { 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); + } + } +} From cae9a76f1ef1bdd8585a74f745c289e87dcc58c0 Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 09:20:41 +0100 Subject: [PATCH 3/7] Fix bug in size hint of sparse iterator The fix is to delegate size hint to `self.base` as it knows how many values are left to consume. Note that `base` is only set if `accessor.bufferView` is set. If it isn't then the sparse iterator actually has no clue on how many items are left. But it at least knows that there are at least `self.values` items left. This issue will be improved in a follow up PR. The problem with the old code was that when `self.values.len()` is less than `self.counter`, a panic was triggered: `attempt to subtract with overflow`. To understand why we need to understand what the `values` and `counter` variables. `self.values` is an iterator over the sparse values that overwrites values from the base accessor. Each call to `SparseIter::next()` will consume one item and `self.values.len()` consequently decrease by 1. `self.counter` holds how many items have been consumed or seen another way the number of successfull calls to `SparseIter::next()`. It starts at 0 and increase until all values in the base accessor has been consumed. Now you hopefully see that the old implementation was just plain wrong. --- src/accessor/util.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/accessor/util.rs b/src/accessor/util.rs index 9629d4b5..996e7301 100644 --- a/src/accessor/util.rs +++ b/src/accessor/util.rs @@ -145,8 +145,11 @@ impl<'a, T: Item> Iterator for SparseIter<'a, T> { } fn size_hint(&self) -> (usize, Option) { - let hint = self.values.len() - self.counter as usize; - (hint, Some(hint)) + if let Some(base) = self.base.as_ref() { + base.size_hint() + } else { + self.values.size_hint() + } } } From f57d145c3a9a3fc314b17f7c0a87b9636b2075ef Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 17:57:31 +0100 Subject: [PATCH 4/7] Add test for size hint on sparse accessor without buffer view For this test case, the iterator gives a size hint of 1 output value. The correct answer is found in `accessor.count`, which is `2`. --- tests/test_wrapper.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/test_wrapper.rs b/tests/test_wrapper.rs index c2ae4af9..2978e91c 100644 --- a/tests/test_wrapper.rs +++ b/tests/test_wrapper.rs @@ -79,3 +79,30 @@ fn test_sparse_accessor_with_base_buffer_view_yield_all_values() { } } } + +/// "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(); + } +} From 263af2a42dc26fba194f4b442858853267cfa243 Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 18:02:31 +0100 Subject: [PATCH 5/7] Add test for values read in sparse accessor without buffer view This test triggers an infinite loop, as `next()` in `SparseIter` does not have any end condition whenever the base buffer view is not set in a sparse accessor. --- tests/test_wrapper.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test_wrapper.rs b/tests/test_wrapper.rs index 2978e91c..ccba9f0c 100644 --- a/tests/test_wrapper.rs +++ b/tests/test_wrapper.rs @@ -106,3 +106,23 @@ fn test_sparse_accessor_without_base_buffer_view_yield_exact_size_hints() { 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); + } +} From a1950339f2127cb7cbb351b28c20cbe5a4e24d8b Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Mon, 8 Jan 2024 14:02:23 +0100 Subject: [PATCH 6/7] Enable reading sparse accessor without buffer view When `accessor.bufferView` is unset the sparse iterator should use T::zero() for `accessor.count` items as base. Then possibly overwriting them with the values in specified by the accessor.sparse section. The sparse iterator is passed `accessor.count` here, so that it knows when it should stop generating items. Before it continued until the end of times. --- src/accessor/util.rs | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/accessor/util.rs b/src/accessor/util.rs index 996e7301..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,11 +158,8 @@ impl<'a, T: Item> Iterator for SparseIter<'a, T> { } fn size_hint(&self) -> (usize, Option) { - if let Some(base) = self.base.as_ref() { - base.size_hint() - } else { - self.values.size_hint() - } + let hint = self.base_count - (self.counter as usize).min(self.base_count); + (hint, Some(hint)) } } @@ -303,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(); @@ -344,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 => { From ca8655d76fb5cef0389b452e7d527ca648d210c5 Mon Sep 17 00:00:00 2001 From: Andreas Andersson Date: Tue, 9 Jan 2024 09:12:12 +0100 Subject: [PATCH 7/7] Update CHANGELOG --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) 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