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.

Sign up for GitHub

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

Jump to bottom

Fix SystemTypeSet::system_type being out of sync with System::type_id #12030

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Fix SystemTypeSet::system_type being out of sync with System::type_id #12030

merged 1 commit into from
Feb 21, 2024

Conversation

jakobhellermann
Copy link
Contributor

Objective

Always have some_system.into_system().type_id() == some_system.into_system_set().system_type().unwrap().

System sets have a fn system_type() -> Option<TypeId> that is implemented by SystemTypeSet to returning the TypeId of the system's function type. This was implemented in #7715 and is used in bevy_mod_debugdump to handle .after(function) constraints.

Back then, System::type_id always also returned the type id of the function item, not of FunctionSystem<M, F>.

#11728 changes the behaviour of System::type_id so that it returns the id of the FunctionSystem/ExclusiveFunctionSystem wrapper, but it did not change SystemTypeSet::system_type, so doing the lookup breaks in bevy_mod_debugdump.

Solution

Change IntoSystemSet for functions to return a SystemTypeSet<FunctionSystem> / SystemTypeSet<ExclusiveFunctionSystem> instead of SystemTypeSet<F>.

@hymm hymm added this to the 0.13.1 milestone Feb 21, 2024
jakobhellermann
jakobhellermann commented Feb 21, 2024
View reviewed changes
crates/bevy_ecs/src/schedule/set.rs
F: SystemParamFunction<Marker>,
{
type Set = SystemTypeSet<Self>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically a breaking change, but IntoSystem<Marker> for F already requires Marker: 'static so I don't think any actual code would depend on a non-static marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant to add this not to the Marker: 'static, diff line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But of course you could construct pathological examples where someone depends on the exact type used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think this is an acceptable "breaking" change for a patch release.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Feb 21, 2024
@jakobhellermann jakobhellermann mentioned this pull request Feb 21, 2024
Merged
@alice-i-cecile
Copy link
Member

@bushrat011899, could I get your review here? (Also do you want to be invited to the Bevy org so I can request your review directly?)

bushrat011899
bushrat011899 approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

This seems reasonable, thanks for catching it!

@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 Feb 21, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 21, 2024
Merged via the queue into bevyengine:main with commit a491bce Feb 21, 2024
27 checks passed
@hymm
Copy link
Contributor

hymm commented Feb 21, 2024

Should this be considered a breaking change?

@bushrat011899
Copy link
Contributor

Should this be considered a breaking change?

If an example of the break could be written down in the patch notes for 0.13.1, then yeah I think it should be listed as a breaking change. I would wager that actually writing an example where a marker is not 'static is pretty contrived, so it's a "no consequence" break.

ameknite pushed a commit to ameknite/bevy that referenced this pull request Feb 22, 2024
@jakobhellermann @ameknite
Fix SystemTypeSet::system_type being out of sync with `System::type…
d64f449 
…_id` (bevyengine#12030)

## Objective

Always have `some_system.into_system().type_id() ==
some_system.into_system_set().system_type().unwrap()`.

System sets have a `fn system_type() -> Option<TypeId>` that is
implemented by `SystemTypeSet` to returning the TypeId of the system's
function type. This was implemented in
bevyengine#7715 and is used in
`bevy_mod_debugdump` to handle `.after(function)` constraints.

Back then, `System::type_id` always also returned the type id of the
function item, not of `FunctionSystem<M, F>`.

bevyengine#11728 changes the behaviour of
`System::type_id` so that it returns the id of the
`FunctionSystem`/`ExclusiveFunctionSystem` wrapper, but it did not
change `SystemTypeSet::system_type`, so doing the lookup breaks in
`bevy_mod_debugdump`.

## Solution

Change `IntoSystemSet` for functions to return a
`SystemTypeSet<FunctionSystem>` /
`SystemTypeSet<ExclusiveFunctionSystem>` instead of `SystemTypeSet<F>`.
@hymm
Copy link
Contributor

hymm commented Feb 22, 2024

I more meant the fact that the method is returning something different now. It's mostly treated as an opaque identifier, so probably not an issue, but might be worth a short migration guide so users are aware.

msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
@jakobhellermann @msvbg
Fix SystemTypeSet::system_type being out of sync with `System::type…
7b44b5c 
…_id` (bevyengine#12030)

## Objective

Always have `some_system.into_system().type_id() ==
some_system.into_system_set().system_type().unwrap()`.

System sets have a `fn system_type() -> Option<TypeId>` that is
implemented by `SystemTypeSet` to returning the TypeId of the system's
function type. This was implemented in
bevyengine#7715 and is used in
`bevy_mod_debugdump` to handle `.after(function)` constraints.

Back then, `System::type_id` always also returned the type id of the
function item, not of `FunctionSystem<M, F>`.

bevyengine#11728 changes the behaviour of
`System::type_id` so that it returns the id of the
`FunctionSystem`/`ExclusiveFunctionSystem` wrapper, but it did not
change `SystemTypeSet::system_type`, so doing the lookup breaks in
`bevy_mod_debugdump`.

## Solution

Change `IntoSystemSet` for functions to return a
`SystemTypeSet<FunctionSystem>` /
`SystemTypeSet<ExclusiveFunctionSystem>` instead of `SystemTypeSet<F>`.
msvbg pushed a commit to msvbg/bevy that referenced this pull request Feb 26, 2024
@jakobhellermann @msvbg
Fix SystemTypeSet::system_type being out of sync with `System::type…
e6548ec 
…_id` (bevyengine#12030)

## Objective

Always have `some_system.into_system().type_id() ==
some_system.into_system_set().system_type().unwrap()`.

System sets have a `fn system_type() -> Option<TypeId>` that is
implemented by `SystemTypeSet` to returning the TypeId of the system's
function type. This was implemented in
bevyengine#7715 and is used in
`bevy_mod_debugdump` to handle `.after(function)` constraints.

Back then, `System::type_id` always also returned the type id of the
function item, not of `FunctionSystem<M, F>`.

bevyengine#11728 changes the behaviour of
`System::type_id` so that it returns the id of the
`FunctionSystem`/`ExclusiveFunctionSystem` wrapper, but it did not
change `SystemTypeSet::system_type`, so doing the lookup breaks in
`bevy_mod_debugdump`.

## Solution

Change `IntoSystemSet` for functions to return a
`SystemTypeSet<FunctionSystem>` /
`SystemTypeSet<ExclusiveFunctionSystem>` instead of `SystemTypeSet<F>`.
mockersf pushed a commit that referenced this pull request Feb 27, 2024
@jakobhellermann @mockersf
Fix SystemTypeSet::system_type being out of sync with `System::type…
4022385 
…_id` (#12030)

## Objective

Always have `some_system.into_system().type_id() ==
some_system.into_system_set().system_type().unwrap()`.

System sets have a `fn system_type() -> Option<TypeId>` that is
implemented by `SystemTypeSet` to returning the TypeId of the system's
function type. This was implemented in
#7715 and is used in
`bevy_mod_debugdump` to handle `.after(function)` constraints.

Back then, `System::type_id` always also returned the type id of the
function item, not of `FunctionSystem<M, F>`.

#11728 changes the behaviour of
`System::type_id` so that it returns the id of the
`FunctionSystem`/`ExclusiveFunctionSystem` wrapper, but it did not
change `SystemTypeSet::system_type`, so doing the lookup breaks in
`bevy_mod_debugdump`.

## Solution

Change `IntoSystemSet` for functions to return a
`SystemTypeSet<FunctionSystem>` /
`SystemTypeSet<ExclusiveFunctionSystem>` instead of `SystemTypeSet<F>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bushrat011899 bushrat011899 bushrat011899 approved these changes

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

@JoJoJet JoJoJet Awaiting requested review from JoJoJet

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior 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
0.13.1
Development

Successfully merging this pull request may close these issues.
4 participants
@jakobhellermann @alice-i-cecile @hymm @bushrat011899