Consider breaking bevy_core up #2931
Comments
CAD97
added
C-Feature
mockersf
added
A-Core
|
to be looked at after #2535 |
|
@mockersf saw this just now and your note that you wanted to follow up on this issue after 0.6. So here is your reminder I guess :) |
|
@colepoirier I didn't want to follow up on that, just that whoever wanted to should wait. Go ahead 😄 |
|
Ah my bad! I thought I was being helpful lol. |
|
As this start to be done, I wonder if it would need a more thorough examination before starting to split it. Maybe with a target dependency graph on the changes that could be done? About the two with started PRs:
To be fair, the compilation impacts should be minimal as the code involved is very small and simple. But I think it makes sense to have "grab-bag" crates, and it makes sense to have a small one (current bevy_core) with the more basic components, and a larger one (bevy_utils) that is needed later in the compilation. |
This sentiment is reasonable, but incorrectly informed; in fact, reversed. Every crate which uses All of the below ignores development/test dependencies.
|
| crate | uses core | uses utils |
|---|---|---|
| bevy_app | ✗ | ✓ |
| bevy_asset | ✗ | ✓ |
| bevy_audio | ✗ | ✓ |
| bevy_core | ≡ | ✓ |
| bevy_core_pipeline | ✓ | ✓ |
| bevy_diagnostic | ✓ | ✓ |
| bevy_ecs | ✗ | ✓ |
| bevy_gilrs | ✗ | ✓ |
| bevy_gltf | ✓ | ✓ |
| bevy_input | ✗ | ✓ |
| bevy_internal | ✓ | ✓ |
| bevy_log | ✗ | ✓ |
| bevy_pbr | ✓ | ✓ |
| bevy_reflect | ✗ | ✓ |
| bevy_render | ✓ | ✓ |
| bevy_scene | ✗ | ✓ |
| bevy_sprite | ✓ | ✓ |
| bevy_text | ✓ | ✓ |
| bevy_transform | ✗ | ✓ |
| bevy_ui | ✓ | ✓ |
| bevy_window | ✗ | ✓ |
| bevy_winit | ✗ | ✓ |
The bevy_time split pulls the timer functionality out of bevy_core; bevy_diagnostic no longer uses bevy_core, and uses bevy_time instead.
The FloatOrd move to bevy_utils means that five (5) crates on that list now no longer use bevy_core, and only use bevy_utils: bevy_core_pipeline, bevy_pbr, bevy_sprite, bevy_text, and bevy_ui.
The list of crates using bevy_core after these two PRs is just three (3):
bevy_gltf:bevy_core::Name.bevy_render: bytemuck (cast_slice,Pod).bevy_internals: duh.
Imho, bevy_utils should own Name; it already owns Label. The only reason I didn't make a PR for that already is that Name derives bevy_reflect::Reflect, and bevy_reflect uses bevy_utils. This looks to be an invertible dependency; bevy_reflect only uses bevy_utils::{AHasher, Duration, Entry, HashMap, HashSet, Uuid}, all of which are upstream reexports. Or Reflect could be implemented manually in bevy_reflect and keep the dependency edge the current direction. This is a bigger discussion/decision, so I haven't done it yet.
bevy_render can and probably should just use bytemuck directly. Or perhaps the bytemuck dependency and reexport should be lifted to bevy_utils rather than bevy_core.
And after that, all that remains is the CorePlugin, which does two things:
- Creates the default task pools.
- Imho, this feels like it should live in
bevy_task(I have not looked into the practicality of moving this plugin functionality there). - Cart suggested it could go in
bevy_ecsor just inbevy_internal.
- Imho, this feels like it should live in
- Registers a bunch of types to the type registry:
bevy_mathtypes, primitive types,Range<f32>,String,Option<String>,HashSet<String>,Entity, andName.- NB:
FloatOrdisn't even type registered. - Imho, the primitive types should be inserted into the type registry by default upon creation in
bevy_reflect. - The rest of the type registration I don't have a strong opinion on where they should logically go.
- NB:
And because I installed the cargo plugin to generate these, enjoy some depgraphs (with transitive dependencies deduplicated to make it readable, and focused to the crates we're talking about):
[main ≡]> cargo depgraph --focus bevy_utils bevy_core bevy_time --dedup-transitive-deps
[bevy_nore]> cargo depgraph --focus bevy_utils bevy_core bevy_time --dedup-transitive-deps
(Note: any crate which doesn't transitively use bevy_utils does not show up in this graph, if any exist.)
|
nice, thanks for the great writeup! |
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by moving FloatOrd to bevy_utils. A step in addressing #2931 and splitting bevy_core into more specific locations. ## Solution Move FloatOrd into bevy_utils. Fix the compile errors. As a result, bevy_core_pipeline, bevy_pbr, bevy_sprite, bevy_text, and bevy_ui no longer depend on bevy_core (they were only using it for `FloatOrd` previously).
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by moving FloatOrd to bevy_utils. A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations. ## Solution Move FloatOrd into bevy_utils. Fix the compile errors. As a result, bevy_core_pipeline, bevy_pbr, bevy_sprite, bevy_text, and bevy_ui no longer depend on bevy_core (they were only using it for `FloatOrd` previously).
|
Love the writeup! I like this general direction. Give that bytemuck is our go-to byte lib (and shows up in some of our user facing apis), I think it should ultimately be re-exported somewhere in bevy (the bevy crate should be usable "standalone" for most scenarios). Although that isn't really possible atm because we can't re-export the bytemuck derives without forking. Barring major UX hiccups I'm forgetting, I'm actually cool with removing the re-exports and just pulling in the crate manually when its needed (ex: in bevy_render). Then we can sort out the "right" way to re-export it, if we decide thats possible and necessary. In #4339 we've pioneered a new way to make upstream proc macros re-exportable from bevy. We might be able to convince the bytemuck authors to adopt that pattern. CorePlugin currently fills the "setup random core bevy stuff" niche. I think we need something to do that, but bevy_core is the wrong place because it exists at the "bottom" of the dependency tree instead of near the "top". I think |
# 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 <[email protected]>
# Objective - Users of bevy_reflect probably always want primitive types registered. ## Solution - Register them by default. --- This is a minor incremental change along the path of [removing catch-all functionality from bevy_core](#2931).
# Objective - Users of bevy_reflect probably always want primitive types registered. ## Solution - Register them by default. --- This is a minor incremental change along the path of [removing catch-all functionality from bevy_core](bevyengine#2931).
# 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 <[email protected]>
# Objective - Users of bevy_reflect probably always want primitive types registered. ## Solution - Register them by default. --- This is a minor incremental change along the path of [removing catch-all functionality from bevy_core](bevyengine#2931).
# Objective - Users of bevy_reflect probably always want primitive types registered. ## Solution - Register them by default. --- This is a minor incremental change along the path of [removing catch-all functionality from bevy_core](bevyengine#2931).
# Objective - Users of bevy_reflect probably always want primitive types registered. ## Solution - Register them by default. --- This is a minor incremental change along the path of [removing catch-all functionality from bevy_core](bevyengine#2931).
# Objective Reduce the catch-all grab-bag of functionality in bevy_core by moving FloatOrd to bevy_utils. A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations. ## Solution Move FloatOrd into bevy_utils. Fix the compile errors. As a result, bevy_core_pipeline, bevy_pbr, bevy_sprite, bevy_text, and bevy_ui no longer depend on bevy_core (they were only using it for `FloatOrd` previously).
# 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 <[email protected]>
What problem does this solve or what need does it fill?
What solution would you like?
What alternative(s) have you considered?
Status quo
Additional context
#2900 (comment); blockquotes are from @cart in linked comment.
The text was updated successfully, but these errors were encountered: