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

Simplify the ComponentIdFor type #8845

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jun 14, 2023

Objective

ComponentIdFor is a type that gives you access to a component's ComponentId in a system. It is currently awkward to use, since it must be wrapped in a Local<> to be used.

Solution

Make ComponentIdFor a proper SystemParam.


Changelog

  • Refactored the type ComponentIdFor in order to simplify how it is used.

Migration Guide

The type ComponentIdFor<T> now implements SystemParam instead of FromWorld -- this means it should be used as the parameter for a system directly instead of being used in a Local.

// Before:
fn my_system(
    component_id: Local<ComponentIdFor<MyComponent>>,
) {
    let component_id = **component_id;
}

// After:
fn my_system(
    component_id: ComponentIdFor<MyComponent>,
) {
    let component_id = component_id.get();
}

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 14, 2023
@JoJoJet JoJoJet requested a review from Aceeri June 14, 2023 19:46
@@ -1,9 +1,10 @@
//! Types for declaring and storing [`Component`]s.

use crate::{
self as bevy_ecs,
Copy link
Member

Choose a reason for hiding this comment

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

What this bein used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our derive macros don't know the difference between bevy_ecs and a third party crate, so we need to include this anytime we use a derive from within bevy_ecs. It's a very annoying issue that, as far as I know, can't be fixed without changes to rustc.

Copy link
Member

@alice-i-cecile alice-i-cecile Jun 15, 2023

Choose a reason for hiding this comment

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

Yeah I've run into this before and never found a satisfying workaround.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ya, forgot about that req, looks good to me :)

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.

Agreed: this is cleaner.

@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 15, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Had no idea this even existed 😳

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 15, 2023
Merged via the queue into bevyengine:main with commit 96a9d04 Jun 15, 2023
@JoJoJet JoJoJet deleted the component-id-for branch June 15, 2023 20:34
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

4 participants