Remove the UpdateAssets and AssetEvents schedules #11986
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james7132
added
A-Assets
alice-i-cecile
approved these changes
Feb 20, 2024
MiniaczQ
reviewed
Feb 26, 2024
crates/bevy_asset/src/lib.rs
Outdated
Comment on lines
419
to
427
| /// Schedule where [`Assets`] resources are updated. | ||
| #[derive(Debug, Hash, PartialEq, Eq, Clone, ScheduleLabel)] | ||
| #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] | ||
| pub struct UpdateAssets; | ||
|
|
||
| /// Schedule where events accumulated in [`Assets`] are applied to the [`AssetEvent`] [`Events`] resource. | ||
| /// | ||
| /// [`Events`]: bevy_ecs::event::Events | ||
| #[derive(Debug, Hash, PartialEq, Eq, Clone, ScheduleLabel)] | ||
| #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] | ||
| pub struct AssetEvents; |
There was a problem hiding this comment.
Currently unused, could be used to maybe keep backwards compatibility, but the migration should be very simple
That would mean this has to wait until 0.14 though
MiniaczQ
approved these changes
Feb 26, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
ghost
mentioned this pull request
msvbg
pushed a commit
to msvbg/bevy
that referenced
this pull request
Feb 26, 2024
# Objective Fix bevyengine#11845. ## Solution Remove the `UpdateAssets` and `AssetEvents` schedules. Moved the `UpdateAssets` systems to `PreUpdate`, and `AssetEvents` systems into `First`. The former is meant to run before any of the event flushes. ## Future Work It'd be ideal if we could manually flush the events for assets to avoid needing two, sort of redundant, systems. This should at least let them potentially run in parallel with all of the systems in the schedules they were moved to. --- ## Changelog Removed: `UpdateAssets` schedule from the main schedule. All systems have been moved to `PreUpdate`. Removed: `AssetEvents` schedule from the main schedule. All systems have been move to `First` with the same system sets. ## Migration Guide TODO
msvbg
pushed a commit
to msvbg/bevy
that referenced
this pull request
Feb 26, 2024
# Objective Fix bevyengine#11845. ## Solution Remove the `UpdateAssets` and `AssetEvents` schedules. Moved the `UpdateAssets` systems to `PreUpdate`, and `AssetEvents` systems into `First`. The former is meant to run before any of the event flushes. ## Future Work It'd be ideal if we could manually flush the events for assets to avoid needing two, sort of redundant, systems. This should at least let them potentially run in parallel with all of the systems in the schedules they were moved to. --- ## Changelog Removed: `UpdateAssets` schedule from the main schedule. All systems have been moved to `PreUpdate`. Removed: `AssetEvents` schedule from the main schedule. All systems have been move to `First` with the same system sets. ## Migration Guide TODO
This was referenced Mar 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fix #11845.
Solution
Remove the
UpdateAssetsandAssetEventsschedules. Moved theUpdateAssetssystems toPreUpdate, andAssetEventssystems intoFirst. The former is meant to run before any of the event flushes.Future Work
It'd be ideal if we could manually flush the events for assets to avoid needing two, sort of redundant, systems. This should at least let them potentially run in parallel with all of the systems in the schedules they were moved to.
Changelog
Removed:
UpdateAssetsschedule from the main schedule. All systems have been moved toPreUpdate.Removed:
AssetEventsschedule from the main schedule. All systems have been move toFirstwith the same system sets.Migration Guide
TODO