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] - Remove ExclusiveSystemParam::apply #7489

Closed
wants to merge 2 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Feb 3, 2023

Objective

The trait method SystemParam::apply allows a SystemParam type to defer world mutations, which is internally used to apply Commands at the end of the stage. Any operations that require &mut World access must be deferred in this way, since parallel systems do not have exclusive access to the world.

The ExclusiveSystemParam trait (added in #6083) has an apply method which serves the same purpose. However, deferring mutations in this way does not make sense for exclusive systems since they already have &mut World access: there is no need to wait until a hard sync point, as the system is a hard sync point. World mutations can and should be performed within the body of the system.

Solution

Remove the method. There were no implementations of this method in the engine.


Changelog

Note for maintainers: this changelog makes more sense if it's placed above the one for #6919.

  • Removed the method ExclusiveSystemParamState::apply.

Migration Guide

Note for maintainers: this migration guide makes more sense if it's placed above the one for #6919.

The trait method ExclusiveSystemParamState::apply has been removed. If you have an exclusive system with buffers that must be applied, you should apply them within the body of the exclusive system.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR labels Feb 3, 2023
@alice-i-cecile
Copy link
Member

I'm in favor of removing this footgun.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This is a good call

@cart
Copy link
Member

cart commented Feb 4, 2023

bors r+

bors bot pushed a commit that referenced this pull request Feb 4, 2023
# Objective

The trait method `SystemParam::apply` allows a `SystemParam` type to defer world mutations, which is internally used to apply `Commands` at the end of the stage. Any operations that require `&mut World` access must be deferred in this way, since parallel systems do not have exclusive access to the world.

The `ExclusiveSystemParam` trait (added in #6083) has an `apply` method which serves the same purpose. However, deferring mutations in this way does not make sense for exclusive systems since they already have `&mut World` access: there is no need to wait until a hard sync point, as the system *is* a hard sync point. World mutations can and should be performed within the body of the system.

## Solution

Remove the method. There were no implementations of this method in the engine.

---

## Changelog

*Note for maintainers: this changelog makes more sense if it's placed above the one for #6919.*

- Removed the method `ExclusiveSystemParamState::apply`.

## Migration Guide

*Note for maintainers: this migration guide makes more sense if it's placed above the one for #6919.*

The trait method `ExclusiveSystemParamState::apply` has been removed. If you have an exclusive system with buffers that must be applied, you should apply them within the body of the exclusive system.
@bors bors bot changed the title Remove ExclusiveSystemParam::apply [Merged by Bors] - Remove ExclusiveSystemParam::apply Feb 4, 2023
@bors bors bot closed this Feb 4, 2023
@JoJoJet JoJoJet deleted the no-exclusive-apply branch February 5, 2023 03:09
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants