[Merged by Bors] - Split time functionality into bevy_time #4187
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
alice-i-cecile
added
C-Code-Quality
|
I definitely like reducing the chaos of our crates further!
I think I like that: it would be nice to be able to swap it out or remove it for niche use cases. |
|
as it is, this would increase build time as it split a new crate without increasing parallelism |
|
As it turns out, bevy_diagnostic is the only in-tree user of "bevy_time", and only uses bevy_time, not the rest of the bevy_core functionality. As such, I've gone ahead and made it depend on bevy_time directly; this also means that the compilation dependency graph is reordered and no longer strictly just a pessimization. I can pull out a |
Alright, sounds good to me. |
|
Things continue to be easier to do than I expect (and that's a good thing! Good job everyone!) so I just went all the way to separate bevy_time from bevy_core. I am honestly surprised at how little actually needed to change to pull TimePlugin out of CorePlugin. |
alice-i-cecile
added
the
M-Needs-Migration-Guide
alice-i-cecile
requested changes
Mar 11, 2022
There was a problem hiding this comment.
Small nit about system timing: I think this fixes an existing bug, but I'd rather not let it continue now that I've seen it.
Other than that, this LGTM: the port went very smoothly, I don't see any mistakes and I'm pretty firmly in favor of the general direction of splitting out bevy_core into more useful parts.
Cart, if and when you merge this, please add a A - Time label.
|
Only CI error seems unrelated tree |
Yep, not your fault. |
|
bors try |
tryMerge conflict. |
|
Rebased. This will almost certainly fail but might as well try: bors try=@alice-i-cecile |
|
🔒 Permission denied Existing reviewers: click here to make CAD97 a reviewer |
mockersf
approved these changes
Mar 12, 2022
IceSentry
approved these changes
Mar 12, 2022
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors try |
tryMerge conflict. |
|
@CAD97 can you rebase? I'd like to get this in, in order to make future PRs easier. |
alice-i-cecile
added
the
X-Controversial
|
(gah, I missed an unused dep in the rebase. I'll get to it tomorrow when I'm back at desktop.) |
|
Fixed the udep. |
CAD97
force-pushed
the
bevy_time
branch
2 times, most recently
from
6faf3e8
to
e458e96
Compare
|
Rebased for #4469 and removed an accidental unused |
|
(Meta note: without a ping when a merge conflict arises, which GitHub does not provide by default, noticing that there's a merge conflict in order to rebase relies on the PR author happening to revisit the PR to see the notice, or a reviewer giving the ping.) |
CAD97
force-pushed
the
bevy_time
branch
2 times, most recently
from
705e107
to
8bb2433
Compare
5 tasks
|
"dead link" on this rebase was a 503 response from https://opensource.org/licenses/MIT |
|
bors try |
cart
approved these changes
May 26, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 26, 2022
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by #3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit. A step in addressing #2931 and splitting bevy_core into more specific locations. ## Solution Pull the time module of bevy_core into a new crate, bevy_time. # Migration guide - Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`. - If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`. - The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
bors
bot
changed the title
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Jun 7, 2022
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by bevyengine#3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit. A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations. ## Solution Pull the time module of bevy_core into a new crate, bevy_time. # Migration guide - Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`. - If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`. - The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by bevyengine#3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit. A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations. ## Solution Pull the time module of bevy_core into a new crate, bevy_time. # Migration guide - Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`. - If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`. - The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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
Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by #3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit.
A step in addressing #2931 and splitting bevy_core into more specific locations.
Solution
Pull the time module of bevy_core into a new crate, bevy_time.
Migration guide
Time,Timer,Stopwatch,FixedTimestep, etc.) should be imported frombevy::time::*rather thanbevy::core::*.CorePluginmanually, you'll also want to addTimePluginfrombevy::time.bevy::core::CorePlugin::Timesystem label is replaced withbevy::time::TimeSystem.