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

[Merged by Bors] - Add component_id function to World and Components #5066

Closed

Conversation

GarettCooper
Copy link
Contributor

Objective

Solution

  • Add a component_id::<T: Component>(&self) function to both World and Components to retrieve the ComponentId associated with T from a immutable reference.

Changelog

  • Added World::component_id::<C>() and Components::component_id::<C>() to retrieve a Component's corresponding ComponentId if it exists.

@GarettCooper GarettCooper force-pushed the component_id_access branch from 684e166 to df727b0 Compare June 22, 2022 01:00
/// A [`ComponentId`] is tightly coupled to its parent [`World`](crate::world::World).
/// Attempting to use a [`ComponentId`] from one [`World`](crate::world::World) to access the metadata
/// of a [`Component`] in a different [`World`](crate::world::World) is undefined behaviour and should
/// not be attempted.
#[derive(Debug, Copy, Clone, Hash, Ord, PartialOrd, Eq, PartialEq)]
pub struct ComponentId(usize);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider talking a bit about the relationship with TypeId here too.

Basically, for Rust types, there's a one-to-one mapping between TypeId and ComponentId. But non-Rust components can be used (well, only kind of so far), and these also get their own ComponentId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new section to explain the relationship. I'm not totally happy with the last sentence "Each Rust type ... or other advanced use cases" since it's pretty clunky, but after fiddling with it for a bit I couldn't come up with anything that wasn't quite a bit more verbose. Suggestions appreciated.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Excellent work, these are really well written docs and the methods are simple and useful.

A few small pieces of doc feedback for you.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 22, 2022
@GarettCooper GarettCooper force-pushed the component_id_access branch from 4e7ba81 to ab8cfc4 Compare June 22, 2022 03:36
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Good job, your descriptions are very nice overall.

I added some nits that helps significantly improve the form of the rendered docs.

Also, keep in mind that adding a link to the same item multiple times within the same item will lower the consulting experience (it looks like there are a lot of connections while actually there are a lot less).

You can view the rendered docs by yourself by using the following command, in case you didn't know:

cargo doc -p bevy_ecs --no-deps --open

Still great work anyway 😀

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
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
@GarettCooper GarettCooper force-pushed the component_id_access branch from b560c74 to ad5120e Compare June 22, 2022 23:53
@GarettCooper GarettCooper force-pushed the component_id_access branch from ad5120e to 25a6e3f Compare June 22, 2022 23:55
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Much better 😁

I added some more nits that I probably didn't catch in the first review pass. I also added redundant link removal.

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/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

I added a couple of suggestions about the style of unit struct declaration. Anyway, it looks good to me. 👍🏻

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Jun 25, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jun 25, 2022
# Objective

- Simplify the process of obtaining a `ComponentId` instance corresponding to a `Component`.
- Resolves #5060.

## Solution

- Add a `component_id::<T: Component>(&self)` function to both `World` and `Components` to retrieve the `ComponentId` associated with `T` from a immutable reference.

---

## Changelog

- Added `World::component_id::<C>()` and `Components::component_id::<C>()` to retrieve a `Component`'s corresponding `ComponentId` if it exists.
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 25, 2022
@bors bors bot changed the title Add component_id function to World and Components [Merged by Bors] - Add component_id function to World and Components Jun 25, 2022
@bors bors bot closed this Jun 25, 2022
@GarettCooper GarettCooper deleted the component_id_access branch June 25, 2022 22:47
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- Simplify the process of obtaining a `ComponentId` instance corresponding to a `Component`.
- Resolves bevyengine#5060.

## Solution

- Add a `component_id::<T: Component>(&self)` function to both `World` and `Components` to retrieve the `ComponentId` associated with `T` from a immutable reference.

---

## Changelog

- Added `World::component_id::<C>()` and `Components::component_id::<C>()` to retrieve a `Component`'s corresponding `ComponentId` if it exists.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Simplify the process of obtaining a `ComponentId` instance corresponding to a `Component`.
- Resolves bevyengine#5060.

## Solution

- Add a `component_id::<T: Component>(&self)` function to both `World` and `Components` to retrieve the `ComponentId` associated with `T` from a immutable reference.

---

## Changelog

- Added `World::component_id::<C>()` and `Components::component_id::<C>()` to retrieve a `Component`'s corresponding `ComponentId` if it exists.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Simplify the process of obtaining a `ComponentId` instance corresponding to a `Component`.
- Resolves bevyengine#5060.

## Solution

- Add a `component_id::<T: Component>(&self)` function to both `World` and `Components` to retrieve the `ComponentId` associated with `T` from a immutable reference.

---

## Changelog

- Added `World::component_id::<C>()` and `Components::component_id::<C>()` to retrieve a `Component`'s corresponding `ComponentId` if it exists.
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

Add a simple method to get a ComponentId for a given Component type
4 participants