Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement UniqueEntitySlice #17589

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

Victoronz
Copy link
Contributor

Objective

Follow-up to #17549 and #16547.

A large part of Vecs usefulness is behind its ability to be sliced, like sorting f.e., so we want the same to be possible for UniqueEntityVec.

Solution

Add a UniqueEntitySlice type. It is a wrapper around [T], and itself a DST.

Because mem::swap has a Sized bound, DSTs cannot be swapped, and we can freely hand out mutable subslices without worrying about the uniqueness invariant of the backing collection!
UniqueEntityVec and the relevant UniqueEntityIters now have methods and trait impls that return UniqueEntitySlices.
UniqueEntitySlice itself can deref into normal slices, which means we can avoid implementing the vast majority of immutable slice methods.

Most of the remaining methods:

  • split a slice/collection in further unique subsections/slices
  • reorder the slice: sort, rotate_*, swap
  • construct/deconstruct/convert pointer-like types: Box, Arc, Rc, Cow
  • are comparison trait impls

As this PR is already larger than I'd like, we leave several things to follow-ups:

  • UniqueEntityArray and the related slice methods that would return it
    • denoted by "chunk", "array_*" for iterators
  • Methods that return iterators with UniqueEntitySlice as their item
    • windows, chunks and split families
  • All methods that are capable of actively mutating individual elements. While they could be offered unsafely, subslicing makes their safety contract weird enough to warrant its own discussion.
    • fill_with, swap_with_slice, iter_mut, split_first/last_mut, select_nth_unstable_*

Note that Arc, Rc and Cow are not fundamental types, so even if they contain UniqueEntitySlice, we cannot write direct trait impls for them.
On top of that, Cow is not a receiver (like self: Arc<Self> is) so we cannot write inherent methods for it either.

@Victoronz Victoronz added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 28, 2025
@@ -709,27 +849,160 @@ impl<'a, T: TrustedEntityBorrow + Copy + 'a> Extend<&'a T> for UniqueEntityVec<T
}
}

impl<T: TrustedEntityBorrow> Index<(Bound<usize>, Bound<usize>)> for UniqueEntityVec<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be impl<T: TrustedEntityBorrow, I: SliceIndex<[T]>> Index<I> for UniqueEntityVec<V>? And similarly for the ones on UniqueEntitySlice. You're already doing that for the get methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah I wish this would work. However it seems that outside of singular blanket impls, an "implement this trait wherever this other type implements it" doesn't really work, it causes overlapping problems.
What we'd want here is specifically impl<T: TrustedEntityBorrow, I: SliceIndex<[T], Output = [T]>> Index<I> because we can only wrap the slice output type.
However, Index<usize> overlaps with this. Even if its output is just T, that T may be [U].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I'd been thinking it could include the Index<usize> case, but I overlooked the fact that you don't do the slice wrapping in that case!

Does that mean that get doesn't work with usize? That's too bad, since I think it's a little more common to index with a single value. It might be worth renaming the get methods something like get_slice so that get(0) still works through deref, although I suppose you can always do .as_inner().get(0) if you want that.

Copy link
Contributor Author

@Victoronz Victoronz Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, get does work with usize! The sweet thing here is that if the signature doesn't work with UniqueEntitySlice, it'll deref to [T] instead! And since we do not wrap &T, this is the exact behavior we want!

Edit: Or so I thought, it seems you're right that it doesn't auto-deref :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error when I try it:

UniqueEntityVec::<Entity>::new().get(0);
error[E0271]: type mismatch resolving `<usize as SliceIndex<[Entity]>>::Output == [Entity]`
    --> crates\bevy_ecs\src\entity\unique_vec.rs:1012:42
     |
1012 |     UniqueEntityVec::<Entity>::new().get(0);
     |                                      --- ^ expected `[Entity]`, found `Entity`
     |                                      |
     |                                      required by a bound introduced by this call
     |
     = note: expected slice `[entity::Entity]`
               found struct `entity::Entity`
note: required by a bound in `unique_slice::UniqueEntitySlice::<T>::get`
    --> crates\bevy_ecs\src\entity\unique_slice.rs:140:28
     |
137  |     pub fn get<I>(&self, index: I) -> Option<&Self>
     |            --- required by a bound in this associated function
...
140  |         I: SliceIndex<[T], Output = [T]>,
     |                            ^^^^^^^^^^^^ required by this bound in `UniqueEntitySlice::<T>::get`

For more information about this error, try `rustc --explain E0271`.

Copy link
Contributor Author

@Victoronz Victoronz Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some way to combine both range and usize indexing, as long as we can write a generic cast for converting the output for I: SliceIndex<[T]> to the output of Self: Index<I>, but I haven't found a way to do so thus far.
(the stdlib implements slice::get by calling index.get(self), but this requires unstable slice_index_methods)

This might be the first time I've encountered a situation where there genuinely might be no other option besides a transmute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bad idea that we probably shouldn't attempt, but that I want to share anyway. :)

It might be possible to exploit the fact that T must be sized for [T] to be valid, but [T] is unsized. This nearly works, but has a lifetime error that I shouldn't spend any more time trying to understand.

pub trait WrapSliceIndex {
    type Output: ?Sized;
    unsafe fn as_output_unchecked(&self) -> &Self::Output;
}

impl<T> WrapSliceIndex for T {
    type Output = T;
    unsafe fn as_output_unchecked(&self) -> &T {
        self
    }
}

impl<T: TrustedEntityBorrow> WrapSliceIndex for [T] {
    type Output = UniqueEntitySlice<T>;
    unsafe fn as_output_unchecked(&self) -> &UniqueEntitySlice<T> {
        unsafe { UniqueEntitySlice::from_slice_unchecked(self) }
    }
}

impl<T: TrustedEntityBorrow, O: WrapSliceIndex, I: SliceIndex<[T], Output = O>> Index<I>
    for UniqueEntitySlice<T>
{
    type Output = O::Output;
    fn index(&self, key: I) -> &O::Output {
        unsafe { self.0.index(key).as_output_unchecked() }
    }
}

I think the good ideas are either:

  1. Do nothing! .as_inner().get(0) works fine.
  2. Rename get to get_slice so that get(0) works by deref but get_slice(..) is still available.

Copy link
Contributor Author

@Victoronz Victoronz Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also share that I've tried to resolve the original overlap with a Sized bound, but for some reason the compiler didn't accept that, which I myself don't really understand.
I think it is possible if we were to write another trait, but then we'd be writing another trait for a single method! That is quite the heavy cost, so I didn't consider it...
With 2. we get the inverse issue where get works, but suddenly we have to do something different to get UniqueEntitySlices back.
I think documenting and leaving it as is for now is best!

(If you or anyone else comes up with some cursed solution though, I'd still be interested in hearing it)

Copy link
Contributor Author

@Victoronz Victoronz Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most generically useful trait that would solve this would be some unsafe FromUnchecked trait, so we could refer generically to "this type can be losslessly constructed from this other one, when safety contract applies"

crates/bevy_ecs/src/entity/unique_slice.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/unique_slice.rs Show resolved Hide resolved
@chescock
Copy link
Contributor

Oh, would it be useful to have versions of slice::from_ref and slice::from_mut, like

pub const fn from_ref(value: &T) -> &Self {
    // SAFETY: The slice always has exactly one element, so it is always unique
    unsafe { Self::from_slice_unchecked(slice::from_ref(value)) }
}

?

@Victoronz
Copy link
Contributor Author

Oh, would it be useful to have versions of slice::from_ref and slice::from_mut, like

pub const fn from_ref(value: &T) -> &Self {
    // SAFETY: The slice always has exactly one element, so it is always unique
    unsafe { Self::from_slice_unchecked(slice::from_ref(value)) }
}

?

you're right! I had missed those functions in the slice module

@Victoronz
Copy link
Contributor Author

Victoronz commented Jan 29, 2025

I added from_ref, from_mut, from_raw_parts and from_raw_parts_mut, so all (stable) freestanding functions in std::slice.
ptr::slice_from_raw_parts/mut also exist, but I am split on whether to add them too. We can't write methods for *const UniqueEntitySlice and *mut UniqueEntitySlice so it might be better to just have people manually manage slice pointers?

@chescock
Copy link
Contributor

I added from_ref, from_mut, from_raw_parts and from_raw_parts_mut, so all (stable) freestanding functions in std::slice.

Looks good!

I had been imagining making them member functions. I assume they're only free functions now because <[_]>::from_ref would be awkward to type, and UniqueEntitySlice::from_ref doesn't have that issue. This is more consistent with std, though!

ptr::slice_from_raw_parts/mut also exist, but I am split on whether to add them too. We can't write methods for *const [T] and *mut [T] so it might be better to just have people manually manage slice pointers?

I vote to leave them out. There isn't anything right now that needs a *const UniqueEntitySlice<T>, and if you're using raw pointers then you're using unsafe anyway and it's not too much trouble to make a final call to from_slice_unchecked at the end.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 31, 2025
crates/bevy_ecs/src/entity/unique_slice.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/unique_slice.rs Show resolved Hide resolved
crates/bevy_ecs/src/entity/unique_slice.rs Show resolved Hide resolved
crates/bevy_ecs/src/entity/unique_slice.rs Show resolved Hide resolved
crates/bevy_ecs/src/entity/unique_slice.rs Show resolved Hide resolved
It is a method that can mutate, which are ones we punt on for now
@Victoronz Victoronz added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 4, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 5, 2025
Merged via the queue into bevyengine:main with commit be9b38e Feb 5, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! D-Unsafe Touches with unsafe code in some way S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants