Fix size hint and values read with sparse iterator #411
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derwiath
force-pushed
the
derwiath/issue-313
branch
from
9dc336d
to
c4339b0
Compare
derwiath
changed the title
derwiath
force-pushed
the
derwiath/issue-313
branch
from
c4339b0
to
77111c1
Compare
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.
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.
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.
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`.
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.
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.
derwiath
force-pushed
the
derwiath/issue-313
branch
from
77111c1
to
a195033
Compare
This was referenced Jan 8, 2024
Nehliin
approved these changes
Jan 9, 2024
|
Thanks for fixing this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #313 @wsw0108 shows that collecting positions specified using a sparse accessor triggers a panic (
attempt to subtract with overflow) inside ofsize_hint()ofSparseIter.In this PR I have rewritten the code in the issue as two unit tests, one that verifies
size_hint()and one that verifies that the positions are actually correct. Both tests fail when using latestmain.The
SimpleSparseAccessor.gltftest asset contains 14 positions specified in a sparse accessor where three of them is overrideen by values specified in the sparse section of the accessor. Look at the data layout image here for an overview: gltf-test/tutorialModels/SimpleSparseAccessor.The two tests pass after delegating the
size_hint()call to the iterator for the base accessor, it knows how many items are left.The gltf specification on Sparse Accessors mentions that a sparse accessor may not have a
bufferViewsetFor
SparseIter,basewill not be set andsize_hint()cannot rely on it anymore. The solution is simple, simply passaccessor.counttoSparseIterso that it can use it to know how many items there are. Then it simply deductsself.counterfrom it as that variable keeps track of how many items have been read from the iterator.The asset
tests/box_sparse.gltfcontains a sparse accessor without a base buffer view. This is used in two unit tests that the behavior is correct, even for these sorts of accessors.