Replace FixedBitSet in Access and FilteredAccess with sorted vectors #16784
Conversation
Trashtalk217
added
A-ECS
alice-i-cecile
added
the
S-Needs-Review
andriyDev
left a comment
andriyDev
left a comment
There was a problem hiding this comment.
The O(1)'s in the description should be O(n). Adding more elements will always add more to iterate through.
| } | ||
| } | ||
|
|
||
| impl<const N: usize> Extend<usize> for SortedSmallVec<N> { |
There was a problem hiding this comment.
This implementation is wrong if other is unsorted. So we should either make this just a loop of inserts or add a method instead of a trait impl that makes it clear the behavior is undefined if you don't call it with a sorted input.
There was a problem hiding this comment.
I think you can handle unsorted input and still be reasonably efficient with just:
self.0.extend(other);
self.0.sort();
self.0.dedup();
| fn from(value: Vec<T>) -> Self { | ||
| Self::Individual(value.iter().map(T::sparse_set_index).collect()) | ||
| let mut conflicts = SortedSmallVec::new_const(); | ||
| for index in value.iter().map(|a| T::sparse_set_index(a)) { |
There was a problem hiding this comment.
Would it make sense to impl FromIterator so you can continue to use collect()? I think that could also replace the various From implementations. And the implementation could call sort() and dedup() rather than doing an insertion sort like this, since it wouldn't need the intermediate states to be sorted.
| } | ||
| } | ||
|
|
||
| impl<const N: usize> Extend<usize> for SortedSmallVec<N> { |
There was a problem hiding this comment.
I think you can handle unsorted input and still be reasonably efficient with just:
self.0.extend(other);
self.0.sort();
self.0.dedup();|
|
||
| /// Stores a sorted list of indices with quick implementation for union, difference, intersection. | ||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||
| pub struct SortedSmallVec<const N: usize>(SmallVec<[usize; N]>); |
There was a problem hiding this comment.
You might need to add some unit tests for this type.
|
This ci failure seems real. |
…evyengine#16769) # Objective Fixes bevyengine#12359 ## Solution Implement alternative number 4. bevyengine#12359 (comment) > I don't think that I agree with the premise of this issue anymore. I am not sure that entities "magically" despawning themselves or components removing themselves make for great defaults in an "ECS-based API". This behavior is likely to be just as surprising to people. > > I think that the lack of sink re-usability should be treated as a bug and possibly the documentation improved to reflect the current limitations if it doesn't seem like a fix is forthcoming. > -- me
…d multidraw. (bevyengine#16755) This commit resolves most of the failures seen in bevyengine#16670. It contains two major fixes: 1. The prepass shaders weren't updated for bindless mode, so they were accessing `material` as a single element instead of as an array. I added the needed `BINDLESS` check. 2. If the mesh didn't support batch set keys (i.e. `get_batch_set_key()` returns `None`), and multidraw was enabled, the batching logic would try to multidraw all the meshes in a bin together instead of disabling multidraw. This is because we checked whether the `Option<BatchSetKey>` for the previous batch was equal to the `Option<BatchSetKey>` for the next batch to determine whether objects could be multidrawn together, which would return true if batch set keys were absent, causing an entire bin to be multidrawn together. This patch fixes the logic so that multidraw is only enabled if the batch set keys match *and are `Some`*. Additionally, this commit adds batch key support for bins that use `Opaque3dNoLightmapBinKey`, which in practice means prepasses. Consequently, this patch enables multidraw for the prepass when GPU culling is enabled. When testing this patch, try adding `GpuCulling` to the camera in the `deferred_rendering` and `ssr` examples. You can see that these examples break without this patch and work properly with it. --------- Co-authored-by: Alice Cecile <[email protected]>
# Objective The doc comments and function namings for `BorderRect` feel imprecise to me. Particularly the `square` function which is used to define a uniform `BorderRect` with equal widths on each edge. But this is potentially confusing since this "square" border could be around an oblong shape. Using "padding" to refer to the border extents seems undesirable too since "padding" is typically used to refer to the area between border and content, not the border itself. ## Solution * Rename `square` to `all` (this matches the name of the similar method on `UiRect`). * Rename `rectangle` to `axes` (this matches the name of the similar method on `UiRect`). * Update doc comments. ## Migration Guide The `square` and `rectangle` functions belonging to `BorderRect` have been renamed to `all` and `axes`. --------- Co-authored-by: Alice Cecile <[email protected]>
# Objective Fixes bevyengine#16771 ## Solution Fixed typo in code. ## Testing - Did you test these changes? If so, how? I tested on my own example, that I included in the issue. It was behaving as I expected. Here is the screenshot after fix, the screenshot before the fix can be found in the issue. 
…16767) # Objective There are two of the same instruction in this example's instruction text. ## Solution Delete one ## Testing `cargo run --example lighting`
…ate), renamed const_new into just const
| } | ||
| // It's only in the difference if it's not in other, | ||
| // and this is the only place in other it could be | ||
| j < other.len() && other.0[j] != *current |
There was a problem hiding this comment.
I think you need to invert the entire condition for difference_with. For example, if other is empty then we want to keep everything and should always return true, but j < other.len() would be false.
| j < other.len() && other.0[j] != *current | |
| j == other.len() || other.0[j] != *current |
| component_read_and_writes: FixedBitSet, | ||
| component_read_and_writes: SortedVecSet<ACCESS_SMALL_VEC_SIZE>, | ||
| /// All exclusively-accessed components, or components that may not be | ||
| /// exclusively accessed if `Self::component_writes_inverted` is set. | ||
| component_writes: FixedBitSet, | ||
| component_writes: SortedVecSet<ACCESS_SMALL_VEC_SIZE>, | ||
| /// All accessed resources. | ||
| resource_read_and_writes: FixedBitSet, | ||
| resource_read_and_writes: SortedVecSet<ACCESS_SMALL_VEC_SIZE>, | ||
| /// The exclusively-accessed resources. | ||
| resource_writes: FixedBitSet, | ||
| resource_writes: SortedVecSet<ACCESS_SMALL_VEC_SIZE>, |
There was a problem hiding this comment.
Is it at all feasible, within this PR or a follow up one, to factor the x and x_inverted pairings into a utility struct that wraps the SortedVecSet and inverted boolean, encapsulating all of the logic for inverting these sets? 😮
There was a problem hiding this comment.
I vote for doing that as a follow-up! It would be a good simplification, but it's not related to the goal of this PR and I don't think we should try to do too many things at once.
BenjaminBrienen
added
S-Waiting-on-Author
11 participants
Adopted from #14385.
Objective
As described in the relations RFC.
The access bitsets are currently efficient because the ComponentIds are dense: they are incremented from 0 and should remain small.
With relations, a ComponentId will have some upper bits 1, so the amount of memory allocated in the FixedBitSet to represent the value would be non-trivial.
Solution
We replace
FixedBitSetswith sorted vectors.The vectors should remain relatively small since queries usually don't involve hundreds of components.
These allow us to do union, difference, and intersection operations in O(1).
Inserting a value, however, is O(1).
Testing
TODO (I can't figure out the bencher)
Migration Guide
TODO (is it really necessary?)