Skip to content

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

Closed
GarettCooper wants to merge 6 commits intobevyengine:mainfrom
GarettCooper:component_id_access
Closed

[Merged by Bors] - Add component_id function to World and Components#5066
GarettCooper wants to merge 6 commits intobevyengine:mainfrom
GarettCooper:component_id_access

Conversation

@GarettCooper
Copy link
Copy Markdown
Contributor

@GarettCooper GarettCooper commented Jun 22, 2022

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
GarettCooper
GarettCooper commented Jun 22, 2022
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Jun 22, 2022
View reviewed changes
crates/bevy_ecs/src/component.rs
/// 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
Copy Markdown
Member

@alice-i-cecile alice-i-cecile Jun 22, 2022

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
Copy Markdown
Contributor Author

@GarettCooper GarettCooper Jun 22, 2022

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.

alice-i-cecile
alice-i-cecile reviewed Jun 22, 2022
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Jun 22, 2022
View reviewed changes
alice-i-cecile
alice-i-cecile requested changes Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
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
Nilirad
Nilirad suggested changes Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
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 😀

@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
Nilirad
Nilirad suggested changes Jun 23, 2022
View reviewed changes
Copy link
Copy Markdown
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.

alice-i-cecile
alice-i-cecile reviewed Jun 24, 2022
View reviewed changes
Nilirad
Nilirad approved these changes Jun 24, 2022
View reviewed changes
Copy link
Copy Markdown
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. 👍🏻

@cart
Copy link
Copy Markdown
Member

cart commented Jun 25, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jun 25, 2022
@GarettCooper
Add component_id function to World and Components (#5066)
fa56a5c 
# 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
Copy link
Copy Markdown
Contributor

bors bot commented Jun 25, 2022

Pull request successfully merged into main.

Build succeeded:

@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
@Nilirad Nilirad mentioned this pull request Jul 1, 2022
Closed
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@GarettCooper @inodentry
Add component_id function to World and Components (bevyengine#5066)
3d5f10c 
# 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
@GarettCooper @james7132
Add component_id function to World and Components (bevyengine#5066)
1c3af19 
# 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
@GarettCooper @ItsDoot
Add component_id function to World and Components (bevyengine#5066)
e5f8daa 
# 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

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@Nilirad Nilirad Nilirad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

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

@GarettCooper @cart @alice-i-cecile @Nilirad