Document and lock down `bevy_ecs::component` APIs #6750

james7132 · 2022-11-25T07:26:44Z

Objective

Document and lock down bevy_ecs::component.

Addresses #3362 for bevy_ecs::component.

Solution

Make mutative APIs on Components pub(crate). Redirect users to the APIs on World instead of Components.
Remove the Components: Default implementation. Replace it with a pub(crate) new function. Forces Components to be initialized in the context of a World.
Rename Components::get_resource_id to resource_id to make it consistent with Components::component_id
Make ComponentId a fully opaque ID.
Make ComponentDescriptor constructors private except for the user-facing case for making components for foreign types.
Make ComponentDescriptor more of a parameter type than a storage type by removing it's accessors. Force users to use ComponentInfo and getting it via Components's fetch APIs.
Document as many of the undocumented types and APIs in the module as possible.

Changelog

Changed: Components::get_resource_id has been renamed to Components::resource_id.
Changed: ComponentDescriptor::new_with_layout has replaced ComponentDescriptor::new.
Removed: Components's Default implementation.
Removed: Components::init_component, Components::init_resource, Components::init_non_send, Components::init_with_component_descriptor.
Removed: ComponentId::new and ComponentId::index.
Removed: ComponentDescriptor::new_resource, ComponentData::type_id, ComponentDescriptor::name, ComponentDescriptor::storage_type.

crates/bevy_ecs/src/component.rs

jakobhellermann · 2022-11-25T08:20:53Z

crates/bevy_ecs/src/component.rs

@@ -231,12 +271,13 @@ pub struct ComponentId(usize);

 impl ComponentId {
    #[inline]
-    pub const fn new(index: usize) -> ComponentId {


I'd like to be able to round-trip ComponentId through some sort of shareable identifier.
Is there a reason for disallowing this?

Maybe we can have ComponentId::from_bits/ComponentId::into_bits (or s/bits/raw or similiar) to clearly document that they can only be created from each other and aren't stable across runs?

The primary one is to avoid users creating their own ComponentIds arbitrarily and converting them into something that might be saved for the exact same reasons you've enumerated: it's not stable across runs and shouldn't be easy to convert to something where you can easily manipulate the representation and reconstruct a valid ComponentId out of it, nor allow users to serialize it to a file or over the network.

Is the use case here to communicate this across runtime boundaries (i.e. scripting)?

Yes, we need to be able to pass component IDs to and from scripts to query components from an entity.

If the point is to cross a potential FFI boundary for scripting, we can set it to repr(transparent) if need be, is that sufficient?

repr(transparent) seems to be strictly worse to me because it's way less obvious to use (I think you have to transmute?) and it gives you no obvious place to list the limitations of round tripping through usize.

jakobhellermann · 2022-11-25T08:35:20Z

crates/bevy_ecs/src/component.rs

    }

    #[inline]
-    pub fn init_resource<T: Resource>(&mut self) -> ComponentId {
+    pub(crate) fn init_resource<T: Resource>(&mut self) -> ComponentId {


This should be left public, or be replaced with init_resource_with_descriptor.

I see no harm in having

Components::init_component_with_descriptor required for dynamic types

Components::init_resource_with_descriptor required for dynamic types

Components::init_component<T>

Components::init_resource<T>
The latter two are useful even externally, because it is impossible to use insert_resource_by_id with a non-initialized resource.

So if you have a resource that isn't automatically inserted, it would be impossible to insert that resource from a script.

Actually init_component_with_descriptor and init_resource_with_descriptor do the same thing.

If you want to have these methods not on Components but on World, then we need to expose all of them. Currently, only init_component exists, but it has only internal usages. Also I don't like the name on World, because init_resource means "insert the default value", so init_component should not sound as similar as it does.

This is going to be tough. If we want to do this on World, we also want to make storages_mut, which is a giant hole for unsoundness if left open.

We could make ComponentsMut<'a> wrap a &'a mut ComponentsInternal and &'a mut Storages, but that might be a bit overkill for just this.

crates/bevy_ecs/src/component.rs

alice-i-cecile · 2022-11-25T11:35:01Z

@jakobhellermann's comments are excellent: I'm in agreement with all of those :)

crates/bevy_ecs/src/component.rs

alice-i-cecile

More documentation feedback :)

crates/bevy_ecs/src/component.rs

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

…to document-ecs-component

Carter0 · 2022-11-30T00:08:20Z

crates/bevy_ecs/src/component.rs

-#[derive(Debug, Default)]
+/// The metadata store for all components within a [`World`].
+///
+/// A public read-only API is accessbile. To initialize a new component,


Suggested change

/// A public read-only API is accessbile. To initialize a new component,

/// A public read-only API is accessible. To initialize a new component,

I'm a little confused by what you mean here. This sentence implies the existence of a public read-only API, but I'm not sure how that relates to the components struct.

Carter0 · 2022-11-30T00:11:38Z

crates/bevy_ecs/src/component.rs

+///
+/// A public read-only API is accessbile. To initialize a new component,
+/// use [`World::init_component`], [`World::init_resource`], [`World::init_non_send_resource`] or
+/// [`World::init_component_with_descriptor`].  Initialized components cannot be removed or


❓ What is the difference between an initialized vs uninitialized component?

IIRC components have to be assigned a ComponentId on a per-world basis, which occurs during initialization.

Would be good to expland on here though.

Carter0 · 2022-11-30T00:14:17Z

crates/bevy_ecs/src/component.rs

-    indices: std::collections::HashMap<TypeId, usize, fxhash::FxBuildHasher>,
-    resource_indices: std::collections::HashMap<TypeId, usize, fxhash::FxBuildHasher>,
+    ids: HashMap<TypeId, ComponentId, fxhash::FxBuildHasher>,
+    resource_ids: HashMap<TypeId, ComponentId, fxhash::FxBuildHasher>,


❓ Sorry if this is a super basic question, but I always assumed resources were something totally different from components. But here we are storing resource ids in the components struct, which makes me think they are the same thing.

Are resources just components?

We're lacking a good word for components + resources :) They share a lot of the same infrastructure, but the naming uses Component in a few places :(

They still have the same IDs, ArchetypeComponentIds, etc. for controlling access and identification, even if the underlying methodology and storage is separated.

Carter0 · 2022-11-30T00:16:12Z

crates/bevy_ecs/src/component.rs

@@ -410,34 +493,43 @@ impl Components {
        index
    }

+    /// Gets the total number of components registered.


❓ What is a registered component? Is it the same thing as an initialized component?

crates/bevy_ecs/src/component.rs

Carter0

Thanks again, digging into this stuff always fascinates me :)

Co-authored-by: Carter Weinberg <weinbergcarter@gmail.com>

crates/bevy_ecs/src/component.rs

alice-i-cecile · 2022-12-10T21:00:44Z

I'm unclear of the comments from #6750 (comment) WRT the publicness of init_resouce have been addressed. I don't think it's acceptable to break / shut off dynamic resource types in this PR, but I'm not personally sure on the right path forward.

inodentry · 2022-12-26T12:34:43Z

I get very worried and uneasy whenever I see PRs for "make things no longer pub, because things are excessively pub".

You always run the risk of shutting down more exotic usecases that you might not have thought anyone would want to do.

You risk causing pain to future users who have a clever idea for something they want to do, but can't, because you made the things they need private.

One of Bevy's great strengths is how easy it is for people to hack on, and how so many things can be implemented in 3rd-party crates.

Please don't "lock down" Bevy!

james7132 · 2022-12-26T17:30:48Z

Please don't "lock down" Bevy!

I've stated this on Discord but I'll reiterate it here for better logging of the rationale.

I'm in general agreement here about the use of pub APIs, within reason. The primary exception lies with bevy_ecs, where the unsafe APIs are riddled with implementation specific invariants. It's borderline impossible to use them safely outside of the crate, so the few ones we do expose should be well audited, well documented, damn near stable, and well justified as to why it should be public. Making them pub means we must take them into account inside bevy_ecs too, which can combinatorially expand the number of ways both first and third party crates and bevy_ecs itself can cause unsoundness.

Even outside of unsafe use of bevy_ecs, it's imperative we stay conscious of the API surface we're exposing. The modularity of the engine demands we expose certain APIs at crate boundaries, but that also means that we can't change it once we hit 1.0. Hyrum's Law states that if it can be observed, someone will depend on it. If we are to make more of the engine public, the interface should be more curated and deter users from footguns, especially when unsafe is involved.

alice-i-cecile · 2023-06-19T16:01:24Z

@james7132 I have agreement in principle with you on these choices, but there's some outstanding controversy. This is in the 0.11 milestone but things seem to have stalled: what would you like to do with this PR?

james7132 · 2023-06-19T22:14:52Z

Let's move it to 0.12. There's quite a few things that need to be addressed here and I don't think we can rush them out before the 0.11 deadline.

james7132 · 2024-03-05T02:01:56Z

Closing this out as there are other PRs that got to this beforehand.

james7132 added 8 commits November 24, 2022 18:26

Make ComponentId opaque

7d5d72d

Remove the Components Default impl

db501d3

Make &mut Components APIs pub(crate)

0fff367

Slim down ComponentDescriptor's API

cf9aba8

Document ComponentInfo

8f81866

Document ComponentStorage and Table/SparseSetStorage

e1826af

Document Components

8dd484b

Fix CI

ccdeac1

james7132 added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 25, 2022

james7132 requested review from jakobhellermann and alice-i-cecile November 25, 2022 07:35

james7132 added this to the 0.10 milestone Nov 25, 2022

jakobhellermann suggested changes Nov 25, 2022

View reviewed changes

alice-i-cecile reviewed Nov 25, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Show resolved Hide resolved

alice-i-cecile requested changes Nov 25, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

james7132 and others added 10 commits November 25, 2022 12:17

Fix grammar in docs.

152b6c0

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>

don't -> doesn't.

9c03173

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Mention trait

71ab47a

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Mention trait

c2ef87d

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Merge branch 'main' into document-ecs-component

877a435

Revert get_resource_id -> resource_id

2d1dbe4

Descriptor -> Configuration

4e2cd00

Rework public functions on ComponentDescriptor

5b41f7c

Merge branch 'document-ecs-component' of github.com:james7132/bevy in…

4ca7aa4

…to document-ecs-component

Fix CI

f9437c2

Carter0 reviewed Nov 30, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

Carter0 reviewed Nov 30, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

Carter0 reviewed Nov 30, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved

Carter0 approved these changes Nov 30, 2022

View reviewed changes

james7132 and others added 2 commits November 30, 2022 03:24

Remove extra k.

63cc798

Co-authored-by: Carter Weinberg <weinbergcarter@gmail.com>

Apply suggestions from code review

fc012a1

Co-authored-by: Carter Weinberg <weinbergcarter@gmail.com>

Suficio reviewed Dec 1, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Show resolved Hide resolved

james7132 requested review from jakobhellermann, alice-i-cecile and Suficio December 9, 2022 23:39

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jan 8, 2023

alice-i-cecile removed this from the 0.10 milestone Jan 8, 2023

james7132 added this to the 0.11 milestone Feb 27, 2023

james7132 modified the milestones: 0.11, 0.12 Jun 19, 2023

alice-i-cecile removed this from the 0.12 milestone Sep 29, 2023

james7132 closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document and lock down `bevy_ecs::component` APIs #6750

Document and lock down `bevy_ecs::component` APIs #6750

james7132 commented Nov 25, 2022 •

edited

Loading

jakobhellermann Nov 25, 2022

james7132 Nov 25, 2022 •

edited

Loading

Suficio Dec 1, 2022

james7132 Dec 2, 2022

jakobhellermann Dec 27, 2022

jakobhellermann Nov 25, 2022

jakobhellermann Nov 25, 2022

jakobhellermann Nov 25, 2022

james7132 Nov 29, 2022

alice-i-cecile commented Nov 25, 2022

alice-i-cecile left a comment

Carter0 Nov 30, 2022

Carter0 Nov 30, 2022

Carter0 Nov 30, 2022

alice-i-cecile Nov 30, 2022

Carter0 Nov 30, 2022

alice-i-cecile Nov 30, 2022

james7132 Nov 30, 2022

Carter0 Nov 30, 2022

Carter0 left a comment

alice-i-cecile commented Dec 10, 2022

inodentry commented Dec 26, 2022

james7132 commented Dec 26, 2022 •

edited

Loading

alice-i-cecile commented Jun 19, 2023

james7132 commented Jun 19, 2023

james7132 commented Mar 5, 2024

	/// A public read-only API is accessbile. To initialize a new component,
	/// A public read-only API is accessible. To initialize a new component,

Document and lock down bevy_ecs::component APIs #6750

Document and lock down bevy_ecs::component APIs #6750

Conversation

james7132 commented Nov 25, 2022 • edited Loading

Objective

Solution

Changelog

Choose a reason for hiding this comment

james7132 Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Nov 25, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carter0 left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Dec 10, 2022

inodentry commented Dec 26, 2022

james7132 commented Dec 26, 2022 • edited Loading

alice-i-cecile commented Jun 19, 2023

james7132 commented Jun 19, 2023

james7132 commented Mar 5, 2024

Document and lock down `bevy_ecs::component` APIs #6750

Document and lock down `bevy_ecs::component` APIs #6750

james7132 commented Nov 25, 2022 •

edited

Loading

james7132 Nov 25, 2022 •

edited

Loading

james7132 commented Dec 26, 2022 •

edited

Loading