Document bevy_ecs::storage

[james7132]

Objective

Move bevy_ecs towards being able to use warn(missing_docs). Aiming to partially address #3492.

Solution

Document bevy_ecs::storage as thoroughly as possible. Added module level, API level, and type level docs wherever possible, including pub(crate) APIs for easier onboarding of newer contributors.

---

Changelog

Added: Documentation for all bevy::ecs::storage APIs.

[alice-i-cecile]

Very nice stuff: these are pretty small corrections and requests.

[Carter0]

Good addition!

[Carter0]

I like that you mentioned it was for "debugging and monitoring purposes". You clarified a question I had before I even thought to ask it.

[Carter0]

Genuinely asking, are these useful comments? Wouldn't people know what the contains function does just from a glance at the type signature?

[james7132]

Perhaps, but this is definitely needed for us to enable cfg!(warn(missing_docs)) without breaking CI.

[Carter0]

What is a “low level backing datastore”? What does low-level mean in this context?

[james7132]

Low level meaning it's meant to be the base abstraction above the language primitives. We're directly working with std/alloc/core APIs here.

Backing datastore means it's the authoritative "place" within the ECS where all of the memory for the World is allocated. It's what "backs" the rest of the user-facing interface.

[Carter0]

These might be good comments to throw in as well! Makes sense

[Carter0]

Is it a common pattern in Bevy to want to have one world use one kind of storage and then another world use another? In my game I am inserting components one frame and then removing them in another, so I would want to use SparseSets instead of tables for that.

[james7132]

The storage type for a component is meant to be universal, defined on the Component type itself. This is managed during Component registration. Though someone could make an incorrect Component registration on different worlds. It's ill advised though.

[Carter0]

Ahh, yes ofc! That's much simpler

[Carter0]

What should we reasonably expect a reviewer to know when looking at these docs? Where should we explain to new people where to go to learn about some high level concept of the engine vs where should we document more advanced details. I'm not sure how useful my comments are because I am writing from the perspective of someone who knows almost nothing, and these docs might be geared to people who already have some base level of understanding.

[Carter0]

What makes it "sparse" if you catch my meaning? How does it work? I think that's what I really want to know.

[james7132]

SparseSets are sparse because the ID space they occupy do not always have an element. An entity doesn't have to have a Transform component, so if it were stored in a ComponentSparseSet, it wouldn't store a value for every entity.

[maniwani]

Going one file at a time. Where possible, can we please mirror the language in std? Saying "returns" consistently instead of "gets" or "fetches" is one example.

Also, suggestions for BlobVec struct definition.

/// A type-erased [`Vec`].
/// 
/// A [`BlobVec`] is conceptually a [`Vec<MaybeUninit<u8>>`] that stores the [`Layout`] and
/// [`Drop`] function pointer of its element type.
///
/// Because the [`World`](bevy_ecs::world::World) cannot know the static type information of its
/// components, it cannot store them in individual, strongly-typed [`Vec<T>`].
pub(super) struct BlobVec {
    /// The layout of the element type this vector stores.
    item_layout: Layout,
    /// The total number of elements (not bytes) this vector can store without reallocating.
    capacity: usize,
    /// The number of elements (not bytes) in the vector.
    len: usize,
    /// The pointer to the allocation.
    // Note: The layout of the allocation always matches [`array_layout(item_layout, capacity)`].
    data: NonNull<u8>,
    /// The pointer to the element type's [`Drop`] function.
    /// `None` if the element type doesn't need to be dropped.
    drop: Option<unsafe fn(OwningPtr<'_>)>,
}

[maniwani]

More doc comments.

Also, side note, maybe we should rename these to match their use as replacements for HashSet and HashMap.

• SparseArray -> SparseSet
• SparseSet -> SparseMap

ComponentSparseSet can stay the same.

[rparrett]

Just want to +1 all of maniwani's feedback.

[james7132]

@maniwani thanks for helping wordsmith the docs here. Might need to update guidance on our general docs guidance to match.

Also, side note, maybe we should rename these to match their use as replacements for HashSet and HashMap.

* `SparseArray` -> `SparseSet`

* `SparseSet` -> `SparseMap`

I'm not 100% convinced since we very much still use array-like instead of set like terminology for SparseArray, and conceptually we still think of it as one wherever it's used. Happy to discuss this further, but not in this docs PR.

[Carter0]

I think its good enough at this point. Thanks for writing all this up :)

[alice-i-cecile]

Solid improvements. I'd like to see:

1. The comment about BlobVec added to the doc strings.
2. The panic comment from get_2_mut.

Once that's done, this LGTM.

[rparrett]

Note: CI failure is real due to two broken doc links.

I can't comment on the validity of specific safety comments but I've read the new docs many times now and they are looking good / make sense to me.