[Merged by Bors] - Document and lock down types in bevy_ecs::archetype #6742

james7132 · 2022-11-24T13:16:25Z

Objective

Document bevy_ecs::archetype and and declutter the public documentation for the module by making types non-pub.

Addresses #3362 for bevy_ecs::archetype.

Solution

Add module level documentation.
Add type and API level documentation for all public facing types.
Make ArchetypeId, ArchetypeGeneration, and ArchetypeComponentId truly opaque IDs that are not publicly constructable.
Make AddBundle non-pub, make Edges::get_add_bundle return a Option<ArchetypeId> and fork the existing function into Edges::get_add_bundle_internal.
Remove pub(crate) on fields that have a corresponding pub accessor function.
Removed the Archetypes: Default impl, opting for a pub(crate) fn new alternative instead.

Changelog

Added: ArchetypeGeneration now implements Ord and PartialOrd.
Removed: Archetypes's Default implementation.
Removed: Archetype::new and Archetype::is_empty.
Removed: ArchetypeId::new and ArchetypeId::value.
Removed: ArchetypeGeneration::value
Removed: ArchetypeIdentity.
Removed: ArchetypeComponentId::new and ArchetypeComponentId::value.
Removed: AddBundle. Edges::get_add_bundle now returns Option<ArchetypeId>

Migration Guide

ArchetypeId, ArchetypeGeneration, and ArchetypeComponentId are all now opaque IDs and cannot be turned into a numeric value. Please file an issue if this does not work for your use case.

Archetype and Archetypes are not constructible outside of bevy_ecs now. Use World::archetypes to get a read-only reference to either of these types.

crates/bevy_ecs/src/archetype.rs

alice-i-cecile

In favor of this! More thoughtful public APIs are really valuable here, and docs are always nice. I left suggestions / requests for more and clearer docs in a few places where I think it will be particularly valuable.

@jakobhellermann, I'd like your review on this: you're one of the folks most likely to be using these internal APIs externally and I want to be sure we don't break something critical for bevy_inspector_egui without a migration path.

jakobhellermann

Thanks for the ping @alice-i-cecile.
These changes look very reasonable, the previously public APIs all feel like they have only internal usages and nothing that's missing from outside bevy_ecs.
Docs are good, especially ArchetypeComponentId.

Carter0

This is great! Thank you so much for doing this, I learned a lot.

alice-i-cecile · 2022-11-26T17:30:26Z

Removing Ready-For-Final-Review as we wait on those typo fixes :)

Co-authored-by: Alice Cecile <[email protected]>

…ArchetypeComponentId

alice-i-cecile · 2022-11-28T13:24:10Z

bors r+

# Objective Document `bevy_ecs::archetype` and and declutter the public documentation for the module by making types non-`pub`. Addresses #3362 for `bevy_ecs::archetype`. ## Solution - Add module level documentation. - Add type and API level documentation for all public facing types. - Make `ArchetypeId`, `ArchetypeGeneration`, and `ArchetypeComponentId` truly opaque IDs that are not publicly constructable. - Make `AddBundle` non-pub, make `Edges::get_add_bundle` return a `Option<ArchetypeId>` and fork the existing function into `Edges::get_add_bundle_internal`. - Remove `pub(crate)` on fields that have a corresponding pub accessor function. - Removed the `Archetypes: Default` impl, opting for a `pub(crate) fn new` alternative instead. --- ## Changelog Added: `ArchetypeGeneration` now implements `Ord` and `PartialOrd`. Removed: `Archetypes`'s `Default` implementation. Removed: `Archetype::new` and `Archetype::is_empty`. Removed: `ArchetypeId::new` and `ArchetypeId::value`. Removed: `ArchetypeGeneration::value` Removed: `ArchetypeIdentity`. Removed: `ArchetypeComponentId::new` and `ArchetypeComponentId::value`. Removed: `AddBundle`. `Edges::get_add_bundle` now returns `Option<ArchetypeId>`

bors · 2022-11-28T13:38:55Z

Build failed (retrying...):

check-compiles

# Objective Document `bevy_ecs::archetype` and and declutter the public documentation for the module by making types non-`pub`. Addresses #3362 for `bevy_ecs::archetype`. ## Solution - Add module level documentation. - Add type and API level documentation for all public facing types. - Make `ArchetypeId`, `ArchetypeGeneration`, and `ArchetypeComponentId` truly opaque IDs that are not publicly constructable. - Make `AddBundle` non-pub, make `Edges::get_add_bundle` return a `Option<ArchetypeId>` and fork the existing function into `Edges::get_add_bundle_internal`. - Remove `pub(crate)` on fields that have a corresponding pub accessor function. - Removed the `Archetypes: Default` impl, opting for a `pub(crate) fn new` alternative instead. --- ## Changelog Added: `ArchetypeGeneration` now implements `Ord` and `PartialOrd`. Removed: `Archetypes`'s `Default` implementation. Removed: `Archetype::new` and `Archetype::is_empty`. Removed: `ArchetypeId::new` and `ArchetypeId::value`. Removed: `ArchetypeGeneration::value` Removed: `ArchetypeIdentity`. Removed: `ArchetypeComponentId::new` and `ArchetypeComponentId::value`. Removed: `AddBundle`. `Edges::get_add_bundle` now returns `Option<ArchetypeId>`

bors · 2022-11-28T14:12:09Z

Pull request successfully merged into main.

Build succeeded:

# Objective Document `bevy_ecs::archetype` and and declutter the public documentation for the module by making types non-`pub`. Addresses bevyengine#3362 for `bevy_ecs::archetype`. ## Solution - Add module level documentation. - Add type and API level documentation for all public facing types. - Make `ArchetypeId`, `ArchetypeGeneration`, and `ArchetypeComponentId` truly opaque IDs that are not publicly constructable. - Make `AddBundle` non-pub, make `Edges::get_add_bundle` return a `Option<ArchetypeId>` and fork the existing function into `Edges::get_add_bundle_internal`. - Remove `pub(crate)` on fields that have a corresponding pub accessor function. - Removed the `Archetypes: Default` impl, opting for a `pub(crate) fn new` alternative instead. --- ## Changelog Added: `ArchetypeGeneration` now implements `Ord` and `PartialOrd`. Removed: `Archetypes`'s `Default` implementation. Removed: `Archetype::new` and `Archetype::is_empty`. Removed: `ArchetypeId::new` and `ArchetypeId::value`. Removed: `ArchetypeGeneration::value` Removed: `ArchetypeIdentity`. Removed: `ArchetypeComponentId::new` and `ArchetypeComponentId::value`. Removed: `AddBundle`. `Edges::get_add_bundle` now returns `Option<ArchetypeId>`

Objective --------- - Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a `ArchetypeGeneration` - This was useful to 3rd party crate extending the base bevy ECS capabilities, such as [`bevy_ecs_dynamic`] and now [`bevy_mod_dynamic_query`] - Making `ArchetypeGeneration` opaque this way made it completely useless, and removed the ability to limit archetype updates to a subset of archetypes. - Making the `index` method on `ArchetypeId` private prevented the use of bitfields and other optimized data structure to store sets of archetype ids. (without `transmute`) This PR is not a simple reversal of the change. It exposes a different API, rethought to keep the private stuff private and the public stuff less error-prone. - Add a `StartRange<ArchetypeGeneration>` `Index` implementation to `Archetypes` - Instead of converting the generation into an index, then creating a ArchetypeId from that index, and indexing `Archetypes` with it, use directly the old `ArchetypeGeneration` to get the range of new archetypes. From careful benchmarking, it seems to also be a performance improvement (~0-5%) on add_archetypes. --- Changelog --------- - Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this allows you to get a slice of newly added archetypes since the last recorded generation. - Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should enable 3rd party crates to use the `Archetypes` API in a meaningful way. [`bevy_ecs_dynamic`]: https://github.com/jakobhellermann/bevy_ecs_dynamic/tree/main [`bevy_mod_dynamic_query`]: https://github.com/nicopap/bevy_mod_dynamic_query/

Objective --------- - Since #6742, It is not possible to build an `ArchetypeId` from a `ArchetypeGeneration` - This was useful to 3rd party crate extending the base bevy ECS capabilities, such as [`bevy_ecs_dynamic`] and now [`bevy_mod_dynamic_query`] - Making `ArchetypeGeneration` opaque this way made it completely useless, and removed the ability to limit archetype updates to a subset of archetypes. - Making the `index` method on `ArchetypeId` private prevented the use of bitfields and other optimized data structure to store sets of archetype ids. (without `transmute`) This PR is not a simple reversal of the change. It exposes a different API, rethought to keep the private stuff private and the public stuff less error-prone. - Add a `StartRange<ArchetypeGeneration>` `Index` implementation to `Archetypes` - Instead of converting the generation into an index, then creating a ArchetypeId from that index, and indexing `Archetypes` with it, use directly the old `ArchetypeGeneration` to get the range of new archetypes. From careful benchmarking, it seems to also be a performance improvement (~0-5%) on add_archetypes. --- Changelog --------- - Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this allows you to get a slice of newly added archetypes since the last recorded generation. - Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should enable 3rd party crates to use the `Archetypes` API in a meaningful way. [`bevy_ecs_dynamic`]: https://github.com/jakobhellermann/bevy_ecs_dynamic/tree/main [`bevy_mod_dynamic_query`]: https://github.com/nicopap/bevy_mod_dynamic_query/ --------- Co-authored-by: vero <[email protected]>

Objective --------- - Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a `ArchetypeGeneration` - This was useful to 3rd party crate extending the base bevy ECS capabilities, such as [`bevy_ecs_dynamic`] and now [`bevy_mod_dynamic_query`] - Making `ArchetypeGeneration` opaque this way made it completely useless, and removed the ability to limit archetype updates to a subset of archetypes. - Making the `index` method on `ArchetypeId` private prevented the use of bitfields and other optimized data structure to store sets of archetype ids. (without `transmute`) This PR is not a simple reversal of the change. It exposes a different API, rethought to keep the private stuff private and the public stuff less error-prone. - Add a `StartRange<ArchetypeGeneration>` `Index` implementation to `Archetypes` - Instead of converting the generation into an index, then creating a ArchetypeId from that index, and indexing `Archetypes` with it, use directly the old `ArchetypeGeneration` to get the range of new archetypes. From careful benchmarking, it seems to also be a performance improvement (~0-5%) on add_archetypes. --- Changelog --------- - Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this allows you to get a slice of newly added archetypes since the last recorded generation. - Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should enable 3rd party crates to use the `Archetypes` API in a meaningful way. [`bevy_ecs_dynamic`]: https://github.com/jakobhellermann/bevy_ecs_dynamic/tree/main [`bevy_mod_dynamic_query`]: https://github.com/nicopap/bevy_mod_dynamic_query/ --------- Co-authored-by: vero <[email protected]>

james7132 added 11 commits November 24, 2022 03:06

Document and lock down ArchetypeId

1a52419

Lock down ArchetypeGeneration and make it an opaque value

e72b2ab

Make ArchetypeIdentity not pub

81a262d

Make ArchetypeEntity fields not pub(crate)

6dee49f

Make ArchetypeComponentInfo not pub

5ca0f29

Document and lockdown ArchetypeComponentId

830cb4d

Remove Archetypes' Default implementation

94d8f93

Document and lockdown Archetypes

6efab22

Document Archetype

738227b

Make AddBundle pub(crate)

53e8dce

Make AddBundle non-pub

c111db9

james7132 requested a review from alice-i-cecile November 24, 2022 13:33