6caa1782d6
21 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Alice Cecile
|
6caa1782d6 |
Define system param validation on a per-system parameter basis (#18504)
# Objective
When introduced, `Single` was intended to simply be silently skipped,
allowing for graceful and efficient handling of systems during invalid
game states (such as when the player is dead).
However, this also caused missing resources to *also* be silently
skipped, leading to confusing and very hard to debug failures. In
0.15.1, this behavior was reverted to a panic, making missing resources
easier to debug, but largely making `Single` (and `Populated`)
worthless, as they would panic during expected game states.
Ultimately, the consensus is that this behavior should differ on a
per-system-param basis. However, there was no sensible way to *do* that
before this PR.
## Solution
Swap `SystemParam::validate_param` from a `bool` to:
```rust
/// The outcome of system / system param validation,
/// used by system executors to determine what to do with a system.
pub enum ValidationOutcome {
/// All system parameters were validated successfully and the system can be run.
Valid,
/// At least one system parameter failed validation, and an error must be handled.
/// By default, this will result in1 a panic. See [crate::error] for more information.
///
/// This is the default behavior, and is suitable for system params that should *always* be valid,
/// either because sensible fallback behavior exists (like [`Query`] or because
/// failures in validation should be considered a bug in the user's logic that must be immediately addressed (like [`Res`]).
Invalid,
/// At least one system parameter failed validation, but the system should be skipped due to [`ValidationBehavior::Skip`].
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
Skipped,
}
```
Then, inside of each `SystemParam` implementation, return either Valid,
Invalid or Skipped.
Currently, only `Single`, `Option<Single>` and `Populated` use the
`Skipped` behavior. Other params (like resources) retain their current
failing
## Testing
Messed around with the fallible_params example. Added a pair of tests:
one for panicking when resources are missing, and another for properly
skipping `Single` and `Populated` system params.
## To do
- [x] get https://github.com/bevyengine/bevy/pull/18454 merged
- [x] fix the todo!() in the macro-powered tuple implementation (please
help 🥺)
- [x] test
- [x] write a migration guide
- [x] update the example comments
## Migration Guide
Various system and system parameter validation methods
(`SystemParam::validate_param`, `System::validate_param` and
`System::validate_param_unsafe`) now return and accept a
`ValidationOutcome` enum, rather than a `bool`. The previous `true`
values map to `ValidationOutcome::Valid`, while `false` maps to
`ValidationOutcome::Invalid`.
However, if you wrote a custom schedule executor, you should now respect
the new `ValidationOutcome::Skipped` parameter, skipping any systems
whose validation was skipped. By contrast, `ValidationOutcome::Invalid`
systems should also be skipped, but you should call the
`default_error_handler` on them first, which by default will result in a
panic.
If you are implementing a custom `SystemParam`, you should consider
whether failing system param validation is an error or an expected
state, and choose between `Invalid` and `Skipped` accordingly. In Bevy
itself, `Single` and `Populated` now once again skip the system when
their conditions are not met. This is the 0.15.0 behavior, but stands in
contrast to the 0.15.1 behavior, where they would panic.
---------
Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>
Co-authored-by: Dmytro Banin <banind@cs.washington.edu>
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
|
||
|
Alice Cecile
|
7a748e6f0a |
Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER (#18454)
There are two related problems here: 1. Users should be able to change the fallback behavior of *all* ECS-based errors in their application by setting the `GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein. 2. The existing solution (#15500) for customizing this behavior is high on boilerplate, not global and adds a great deal of complexity. The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question: `Single` / `Populated` should silently skip the system, but `Res` should panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day). I've removed the existing `ParamWarnPolicy`-based configuration, and wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the various schedule executors to properly plumb through errors . Additionally, I've done a small cleanup pass on the corresponding example. I've run the `fallible_params` example, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲 1. Currently, failed system param validation will result in endless console spam. Do you want me to implement a solution for warn_once-style debouncing somehow? 2. Currently, the error reporting for failed system param validation is very limited: all we get is that a system param failed validation and the name of the system. Do you want me to implement improved error reporting by bubbling up errors in this PR? 3. There is broad consensus that the default behavior for failed system param validation should be set on a per-system param basis. Would you like me to implement that in this PR? My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart. `ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed completely. Failures during system param validation are now handled via the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs for more information. --------- Co-authored-by: MiniaczQ <xnetroidpl@gmail.com> |
||
|
Alice Cecile
|
a7e9330af9
|
Implement WorldQuery for MainWorld and RenderWorld components (#15745)
# Objective #15320 is a particularly painful breaking change, and the new `RenderEntity` in particular is very noisy, with a lot of `let entity = entity.id()` spam. ## Solution Implement `WorldQuery`, `QueryData` and `ReadOnlyQueryData` for `RenderEntity` and `WorldEntity`. These work the same as the `Entity` impls from a user-facing perspective: they simply return an owned (copied) `Entity` identifier. This dramatically reduces noise and eases migration. Under the hood, these impls defer to the implementations for `&T` for everything other than the "call .id() for the user" bit, as they involve read-only access to component data. Doing it this way (as opposed to implementing a custom fetch, as tried in the first commit) dramatically reduces the maintenance risk of complex unsafe code outside of `bevy_ecs`. To make this easier (and encourage users to do this themselves!), I've made `ReadFetch` and `WriteFetch` slightly more public: they're no longer `doc(hidden)`. This is a good change, since trying to vendor the logic is much worse than just deferring to the existing tested impls. ## Testing I've run a handful of rendering examples (breakout, alien_cake_addict, auto_exposure, fog_volumes, box_shadow) and nothing broke. ## Follow-up We should lint for the uses of `&RenderEntity` and `&MainEntity` in queries: this is just less nice for no reason. --------- Co-authored-by: Trashtalk217 <trashtalk217@gmail.com> |
||
|
Kristoffer Søholm
|
2d1b4939d2
|
Synchronize removed components with the render world (#15582)
# Objective Fixes #15560 Fixes (most of) #15570 Currently a lot of examples (and presumably some user code) depend on toggling certain render features by adding/removing a single component to an entity, e.g. `SpotLight` to toggle a light. Because of the retained render world this no longer works: Extract will add any new components, but when it is removed the entity persists unchanged in the render world. ## Solution Add `SyncComponentPlugin<C: Component>` that registers `SyncToRenderWorld` as a required component for `C`, and adds a component hook that will clear all components from the render world entity when `C` is removed. We add this plugin to `ExtractComponentPlugin` which fixes most instances of the problem. For custom extraction logic we can manually add `SyncComponentPlugin` for that component. We also rename `WorldSyncPlugin` to `SyncWorldPlugin` so we start a naming convention like all the `Extract` plugins. In this PR I also fixed a bunch of breakage related to the retained render world, stemming from old code that assumed that `Entity` would be the same in both worlds. I found that using the `RenderEntity` wrapper instead of `Entity` in data structures when referring to render world entities makes intent much clearer, so I propose we make this an official pattern. ## Testing Run examples like ``` cargo run --features pbr_multi_layer_material_textures --example clearcoat cargo run --example volumetric_fog ``` and see that they work, and that toggles work correctly. But really we should test every single example, as we might not even have caught all the breakage yet. --- ## Migration Guide The retained render world notes should be updated to explain this edge case and `SyncComponentPlugin` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Trashtalk217 <trashtalk217@gmail.com> |
||
|
Trashtalk217
|
d1bd46d45e
|
Deprecate get_or_spawn (#15652)
# Objective After merging retained rendering world #15320, we now have a good way of creating a link between worlds (*HIYAA intensifies*). This means that `get_or_spawn` is no longer necessary for that function. Entity should be opaque as the warning above `get_or_spawn` says. This is also part of #15459. I'm deprecating `get_or_spawn_batch` in a different PR in order to keep the PR small in size. ## Solution Deprecate `get_or_spawn` and replace it with `get_entity` in most contexts. If it's possible to query `&RenderEntity`, then the entity is synced and `render_entity.id()` is initialized in the render world. ## Migration Guide If you are given an `Entity` and you want to do something with it, use `Commands.entity(...)` or `World.entity(...)`. If instead you want to spawn something use `Commands.spawn(...)` or `World.spawn(...)`. If you are not sure if an entity exists, you can always use `get_entity` and match on the `Option<...>` that is returned. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
MiniaczQ
|
acea4e7e6f
|
Better warnings about invalid parameters (#15500)
# Objective System param validation warnings should be configurable and default to "warn once" (per system). Fixes: #15391 ## Solution `SystemMeta` is given a new `ParamWarnPolicy` field. The policy decides whether warnings will be emitted by each system param when it fails validation. The policy is updated by the system after param validation fails. Example warning: ``` 2024-09-30T18:10:04.740749Z WARN bevy_ecs::system::function_system: System fallible_params::do_nothing_fail_validation will not run because it requested inaccessible system parameter Single<(), (With<Player>, With<Enemy>)> ``` Currently, only the first invalid parameter is displayed. Warnings can be disabled on function systems using `.param_never_warn()`. (there is also `.with_param_warn_policy(policy)`) ## Testing Ran `fallible_params` example. --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> |
||
|
Zachary Harrold
|
d70595b667
|
Add core and alloc over std Lints (#15281)
# Objective - Fixes #6370 - Closes #6581 ## Solution - Added the following lints to the workspace: - `std_instead_of_core` - `std_instead_of_alloc` - `alloc_instead_of_core` - Used `cargo +nightly fmt` with [item level use formatting](https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#Item%5C%3A) to split all `use` statements into single items. - Used `cargo clippy --workspace --all-targets --all-features --fix --allow-dirty` to _attempt_ to resolve the new linting issues, and intervened where the lint was unable to resolve the issue automatically (usually due to needing an `extern crate alloc;` statement in a crate root). - Manually removed certain uses of `std` where negative feature gating prevented `--all-features` from finding the offending uses. - Used `cargo +nightly fmt` with [crate level use formatting](https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#Crate%5C%3A) to re-merge all `use` statements matching Bevy's previous styling. - Manually fixed cases where the `fmt` tool could not re-merge `use` statements due to conditional compilation attributes. ## Testing - Ran CI locally ## Migration Guide The MSRV is now 1.81. Please update to this version or higher. ## Notes - This is a _massive_ change to try and push through, which is why I've outlined the semi-automatic steps I used to create this PR, in case this fails and someone else tries again in the future. - Making this change has no impact on user code, but does mean Bevy contributors will be warned to use `core` and `alloc` instead of `std` where possible. - This lint is a critical first step towards investigating `no_std` options for Bevy. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
MiniaczQ
|
e312da8c52
|
Reduce runtime panics through SystemParam validation (#15276)
# Objective The goal of this PR is to introduce `SystemParam` validation in order to reduce runtime panics. Fixes #15265 ## Solution `SystemParam` now has a new method `validate_param(...) -> bool`, which takes immutable variants of `get_param` arguments. The returned value indicates whether the parameter can be acquired from the world. If parameters cannot be acquired for a system, it won't be executed, similarly to run conditions. This reduces panics when using params like `Res`, `ResMut`, etc. as well as allows for new, ergonomic params like #15264 or #15302. Param validation happens at the level of executors. All validation happens directly before executing a system, in case of normal systems they are skipped, in case of conditions they return false. Warning about system skipping is primitive and subject to change in subsequent PRs. ## Testing Two executor tests check that all executors: - skip systems which have invalid parameters: - piped systems get skipped together, - dependent systems still run correctly, - skip systems with invalid run conditions: - system conditions have invalid parameters, - system set conditions have invalid parameters. |
||
|
Tristan Guichaoua
|
b0f5d4df58
|
Enable the unsafe_op_in_unsafe_fn lint (#11591)
# Objective - Partial fix of #11590 ## Solution - Enable `unsafe_op_in_unsafe_fn` at workspace level - Fix the lint for most of the crates |
||
|
Doonv
|
189ceaf0d3
|
Replace or document ignored doctests (#11040)
# Objective There are a lot of doctests that are `ignore`d for no documented reason. And that should be fixed. ## Solution I searched the bevy repo with the regex ` ```[a-z,]*ignore ` in order to find all `ignore`d doctests. For each one of the `ignore`d doctests, I did the following steps: 1. Attempt to remove the `ignored` attribute while still passing the test. I did this by adding hidden dummy structs and imports. 2. If step 1 doesn't work, attempt to replace the `ignored` attribute with the `no_run` attribute while still passing the test. 3. If step 2 doesn't work, keep the `ignored` attribute but add documentation for why the `ignored` attribute was added. --------- Co-authored-by: François <mockersf@gmail.com> |
||
|
Bruce Mitchener
|
5e91e5f3ce
|
Improve doc formatting. (#9840)
# Objective - Identifiers in docs should be marked up with backticks. ## Solution - Mark up more identifiers in the docs with backticks. |
||
|
JoJoJet
|
3442a13d2c
|
Use UnsafeWorldCell to increase code quality for SystemParam (#8174)
# Objective The type `&World` is currently in an awkward place, since it has two meanings: 1. Read-only access to the entire world. 2. Interior mutable access to the world; immutable and/or mutable access to certain portions of world data. This makes `&World` difficult to reason about, and surprising to see in function signatures if one does not know about the interior mutable property. The type `UnsafeWorldCell` was added in #6404, which is meant to alleviate this confusion by adding a dedicated type for interior mutable world access. However, much of the engine still treats `&World` as an interior mutable-ish type. One of those places is `SystemParam`. ## Solution Modify `SystemParam::get_param` to accept `UnsafeWorldCell` instead of `&World`. Simplify the safety invariants, since the `UnsafeWorldCell` type encapsulates the concept of constrained world access. --- ## Changelog `SystemParam::get_param` now accepts an `UnsafeWorldCell` instead of `&World`. This type provides a high-level API for unsafe interior mutable world access. ## Migration Guide For manual implementers of `SystemParam`: the function `get_item` now takes `UnsafeWorldCell` instead of `&World`. To access world data, use: * `.get_entity()`, which returns an `UnsafeEntityCell` which can be used to access component data. * `get_resource()` and its variants, to access resource data. |
||
|
JoJoJet
|
2e7b915ba4
|
Increase type safety and clarity for change detection (#7905) | ||
|
Alice Cecile
|
206c7ce219 |
Migrate engine to Schedule v3 (#7267)
Huge thanks to @maniwani, @devil-ira, @hymm, @cart, @superdump and @jakobhellermann for the help with this PR. # Objective - Followup #6587. - Minimal integration for the Stageless Scheduling RFC: https://github.com/bevyengine/rfcs/pull/45 ## Solution - [x] Remove old scheduling module - [x] Migrate new methods to no longer use extension methods - [x] Fix compiler errors - [x] Fix benchmarks - [x] Fix examples - [x] Fix docs - [x] Fix tests ## Changelog ### Added - a large number of methods on `App` to work with schedules ergonomically - the `CoreSchedule` enum - `App::add_extract_system` via the `RenderingAppExtension` trait extension method - the private `prepare_view_uniforms` system now has a public system set for scheduling purposes, called `ViewSet::PrepareUniforms` ### Removed - stages, and all code that mentions stages - states have been dramatically simplified, and no longer use a stack - `RunCriteriaLabel` - `AsSystemLabel` trait - `on_hierarchy_reports_enabled` run criteria (now just uses an ad hoc resource checking run condition) - systems in `RenderSet/Stage::Extract` no longer warn when they do not read data from the main world - `RunCriteriaLabel` - `transform_propagate_system_set`: this was a nonstandard pattern that didn't actually provide enough control. The systems are already `pub`: the docs have been updated to ensure that the third-party usage is clear. ### Changed - `System::default_labels` is now `System::default_system_sets`. - `App::add_default_labels` is now `App::add_default_sets` - `CoreStage` and `StartupStage` enums are now `CoreSet` and `StartupSet` - `App::add_system_set` was renamed to `App::add_systems` - The `StartupSchedule` label is now defined as part of the `CoreSchedules` enum - `.label(SystemLabel)` is now referred to as `.in_set(SystemSet)` - `SystemLabel` trait was replaced by `SystemSet` - `SystemTypeIdLabel<T>` was replaced by `SystemSetType<T>` - The `ReportHierarchyIssue` resource now has a public constructor (`new`), and implements `PartialEq` - Fixed time steps now use a schedule (`CoreSchedule::FixedTimeStep`) rather than a run criteria. - Adding rendering extraction systems now panics rather than silently failing if no subapp with the `RenderApp` label is found. - the `calculate_bounds` system, with the `CalculateBounds` label, is now in `CoreSet::Update`, rather than in `CoreSet::PostUpdate` before commands are applied. - `SceneSpawnerSystem` now runs under `CoreSet::Update`, rather than `CoreStage::PreUpdate.at_end()`. - `bevy_pbr::add_clusters` is no longer an exclusive system - the top level `bevy_ecs::schedule` module was replaced with `bevy_ecs::scheduling` - `tick_global_task_pools_on_main_thread` is no longer run as an exclusive system. Instead, it has been replaced by `tick_global_task_pools`, which uses a `NonSend` resource to force running on the main thread. ## Migration Guide - Calls to `.label(MyLabel)` should be replaced with `.in_set(MySet)` - Stages have been removed. Replace these with system sets, and then add command flushes using the `apply_system_buffers` exclusive system where needed. - The `CoreStage`, `StartupStage, `RenderStage` and `AssetStage` enums have been replaced with `CoreSet`, `StartupSet, `RenderSet` and `AssetSet`. The same scheduling guarantees have been preserved. - Systems are no longer added to `CoreSet::Update` by default. Add systems manually if this behavior is needed, although you should consider adding your game logic systems to `CoreSchedule::FixedTimestep` instead for more reliable framerate-independent behavior. - Similarly, startup systems are no longer part of `StartupSet::Startup` by default. In most cases, this won't matter to you. - For example, `add_system_to_stage(CoreStage::PostUpdate, my_system)` should be replaced with - `add_system(my_system.in_set(CoreSet::PostUpdate)` - When testing systems or otherwise running them in a headless fashion, simply construct and run a schedule using `Schedule::new()` and `World::run_schedule` rather than constructing stages - Run criteria have been renamed to run conditions. These can now be combined with each other and with states. - Looping run criteria and state stacks have been removed. Use an exclusive system that runs a schedule if you need this level of control over system control flow. - For app-level control flow over which schedules get run when (such as for rollback networking), create your own schedule and insert it under the `CoreSchedule::Outer` label. - Fixed timesteps are now evaluated in a schedule, rather than controlled via run criteria. The `run_fixed_timestep` system runs this schedule between `CoreSet::First` and `CoreSet::PreUpdate` by default. - Command flush points introduced by `AssetStage` have been removed. If you were relying on these, add them back manually. - Adding extract systems is now typically done directly on the main app. Make sure the `RenderingAppExtension` trait is in scope, then call `app.add_extract_system(my_system)`. - the `calculate_bounds` system, with the `CalculateBounds` label, is now in `CoreSet::Update`, rather than in `CoreSet::PostUpdate` before commands are applied. You may need to order your movement systems to occur before this system in order to avoid system order ambiguities in culling behavior. - the `RenderLabel` `AppLabel` was renamed to `RenderApp` for clarity - `App::add_state` now takes 0 arguments: the starting state is set based on the `Default` impl. - Instead of creating `SystemSet` containers for systems that run in stages, simply use `.on_enter::<State::Variant>()` or its `on_exit` or `on_update` siblings. - `SystemLabel` derives should be replaced with `SystemSet`. You will also need to add the `Debug`, `PartialEq`, `Eq`, and `Hash` traits to satisfy the new trait bounds. - `with_run_criteria` has been renamed to `run_if`. Run criteria have been renamed to run conditions for clarity, and should now simply return a bool. - States have been dramatically simplified: there is no longer a "state stack". To queue a transition to the next state, call `NextState::set` ## TODO - [x] remove dead methods on App and World - [x] add `App::add_system_to_schedule` and `App::add_systems_to_schedule` - [x] avoid adding the default system set at inappropriate times - [x] remove any accidental cycles in the default plugins schedule - [x] migrate benchmarks - [x] expose explicit labels for the built-in command flush points - [x] migrate engine code - [x] remove all mentions of stages from the docs - [x] verify docs for States - [x] fix uses of exclusive systems that use .end / .at_start / .before_commands - [x] migrate RenderStage and AssetStage - [x] migrate examples - [x] ensure that transform propagation is exported in a sufficiently public way (the systems are already pub) - [x] ensure that on_enter schedules are run at least once before the main app - [x] re-enable opt-in to execution order ambiguities - [x] revert change to `update_bounds` to ensure it runs in `PostUpdate` - [x] test all examples - [x] unbreak directional lights - [x] unbreak shadows (see 3d_scene, 3d_shape, lighting, transparaency_3d examples) - [x] game menu example shows loading screen and menu simultaneously - [x] display settings menu is a blank screen - [x] `without_winit` example panics - [x] ensure all tests pass - [x] SubApp doc test fails - [x] runs_spawn_local tasks fails - [x] [Fix panic_when_hierachy_cycle test hanging](https://github.com/alice-i-cecile/bevy/pull/120) ## Points of Difficulty and Controversy **Reviewers, please give feedback on these and look closely** 1. Default sets, from the RFC, have been removed. These added a tremendous amount of implicit complexity and result in hard to debug scheduling errors. They're going to be tackled in the form of "base sets" by @cart in a followup. 2. The outer schedule controls which schedule is run when `App::update` is called. 3. I implemented `Label for `Box<dyn Label>` for our label types. This enables us to store schedule labels in concrete form, and then later run them. I ran into the same set of problems when working with one-shot systems. We've previously investigated this pattern in depth, and it does not appear to lead to extra indirection with nested boxes. 4. `SubApp::update` simply runs the default schedule once. This sucks, but this whole API is incomplete and this was the minimal changeset. 5. `time_system` and `tick_global_task_pools_on_main_thread` no longer use exclusive systems to attempt to force scheduling order 6. Implemetnation strategy for fixed timesteps 7. `AssetStage` was migrated to `AssetSet` without reintroducing command flush points. These did not appear to be used, and it's nice to remove these bottlenecks. 8. Migration of `bevy_render/lib.rs` and pipelined rendering. The logic here is unusually tricky, as we have complex scheduling requirements. ## Future Work (ideally before 0.10) - Rename schedule_v3 module to schedule or scheduling - Add a derive macro to states, and likely a `EnumIter` trait of some form - Figure out what exactly to do with the "systems added should basically work by default" problem - Improve ergonomics for working with fixed timesteps and states - Polish FixedTime API to match Time - Rebase and merge #7415 - Resolve all internal ambiguities (blocked on better tools, especially #7442) - Add "base sets" to replace the removed default sets. |
||
|
JoJoJet
|
5d5a504685 |
Revise SystemParam docs (#7274)
# Objective Increase clarity in a few places for the `SystemParam` docs. |
||
|
JoJoJet
|
d9265db344 |
Implement ReadOnlySystemParam for Extract<> (#7182)
# Objective - `Extract` does not implement `ReadOnlySystemParam` even though it should. - Noticed by @hymm on discord: https://discord.com/channels/691052431525675048/749335865876021248/1063535818267963543 ## Solution Implement the trait. |
||
|
JoJoJet
|
1efdbb7e3e |
Remove the SystemParamState trait and remove types like ResState (#6919)
Spiritual successor to #5205. Actual successor to #6865. # Objective Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state. Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it. ## Solution * Merge the trait `SystemParamState` into `SystemParam`. * Remove all trivial `SystemParam` state types. * `OptionNonSendMutState<T>`: you will not be missed. --- - [x] Fix/resolve the remaining test failure. ## Changelog * Removed the trait `SystemParamState`, merging its functionality into `SystemParam`. ## Migration Guide **Note**: this should replace the migration guide for #6865. This is relative to Bevy 0.9, not main. The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`. ```rust // Before (0.9) impl SystemParam for MyParam<'_, '_> { type State = MyParamState; } unsafe impl SystemParamState for MyParamState { fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... } } unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState { type Item = MyParam<'w, 's>; fn get_param(&mut self, ...) -> Self::Item; } unsafe impl ReadOnlySystemParamFetch for MyParamState { } // After (0.10) unsafe impl SystemParam for MyParam<'_, '_> { type State = MyParamState; type Item<'w, 's> = MyParam<'w, 's>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... } fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>; } unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { } ``` The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`. ```rust // Before unsafe impl ReadOnlySystemParamFetch for MyParamState {} // After unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {} ``` |
||
|
JoJoJet
|
1af73624fa |
Simplify trait hierarchy for SystemParam (#6865)
# Objective
* Implementing a custom `SystemParam` by hand requires implementing three traits -- four if it is read-only.
* The trait `SystemParamFetch<'w, 's>` is a workaround from before we had generic associated types, and is no longer necessary.
## Solution
* Combine the trait `SystemParamFetch` with `SystemParamState`.
* I decided to remove the `Fetch` name and keep the `State` name, since the former was consistently conflated with the latter.
* Replace the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`, which simplifies trait bounds in generic code.
---
## Changelog
- Removed the trait `SystemParamFetch`, moving its functionality to `SystemParamState`.
- Replaced the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`.
## Migration Guide
The trait `SystemParamFetch` has been removed, and its functionality has been transferred to `SystemParamState`.
```rust
// Before
impl SystemParamState for MyParamState {
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
type Item = MyParam<'w, 's>;
fn get_param(...) -> Self::Item;
}
// After
impl SystemParamState for MyParamState {
type Item<'w, 's> = MyParam<'w, 's>; // Generic associated types!
fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
fn get_param<'w, 's>(...) -> Self::Item<'w, 's>;
}
```
The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.
```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}
// After
unsafe impl<'w, 's> ReadOnlySystemParam for MyParam<'w, 's> {}
```
|
||
|
Carter Anderson
|
dc3f801239 |
Exclusive Systems Now Implement System. Flexible Exclusive System Params (#6083)
# Objective The [Stageless RFC](https://github.com/bevyengine/rfcs/pull/45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move. This is an alternative to #4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns). ## Solution This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). This means you can remove all cases of `exclusive_system()`: ```rust // before commands.add_system(some_system.exclusive_system()); // after commands.add_system(some_system); ``` I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems: ```rust fn some_exclusive_system( world: &mut World, transforms: &mut QueryState<&Transform>, state: &mut SystemState<(Res<Time>, Query<&Player>)>, ) { for transform in transforms.iter(world) { println!("{transform:?}"); } let (time, players) = state.get(world); for player in players.iter() { println!("{player:?}"); } } ``` Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system. I added some targeted SystemParam `static` constraints, which removed the need for this: ``` rust fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {} ``` ## Related - #2923 - #3001 - #3946 ## Changelog - `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait. - `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems - `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam` - Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api. ## Migration Guide Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems: ```rust // Old (0.8) app.add_system(some_exclusive_system.exclusive_system()); // New (0.9) app.add_system(some_exclusive_system); ``` Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis: ```rust // Old (0.8) app.add_system(some_system.exclusive_system().at_end()); // New (0.9) app.add_system(some_system.at_end()); ``` Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons: ```rust // Old (0.8) fn some_system(world: &mut World) { let mut transforms = world.query::<&Transform>(); for transform in transforms.iter(world) { } } // New (0.9) fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) { for transform in transforms.iter(world) { } } ``` |
||
|
ira
|
2b80a3f279 |
Implement IntoIterator for &Extract<P> (#6025)
# Objective Implement `IntoIterator` for `&Extract<P>` if the system parameter it wraps implements `IntoIterator`. Enables the use of `IntoIterator` with an extracted query. Co-authored-by: devil-ira <justthecooldude@gmail.com> |
||
|
Daniel McNab
|
7b2cf98896 |
Make RenderStage::Extract run on the render world (#4402)
# Objective - Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource. - However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource. - This meant that effectively only one extract which wrote to resources could run at a time. - We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that. ## Solution - Move the extract stage to run on the render world. - Add the main world as a `MainWorld` resource. - Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`. ## Future work It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on. We need to determine how this interacts with https://github.com/bevyengine/bevy/pull/3519 It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too. ## Todo I still need to add doc comments to `Extract`. --- ## Changelog ### Changed - The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. Resources on the render world can now be accessed using `ResMut` during extract. ### Removed - `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead ## Migration Guide The `Extract` `RenderStage` now runs on the render world (instead of the main world as before). You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it. For example, if previously your extract system looked like: ```rust fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { for cloud in clouds.iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` the new version would be: ```rust fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` The diff is: ```diff --- a/src/clouds.rs +++ b/src/clouds.rs @@ -1,5 +1,5 @@ -fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) { - for cloud in clouds.iter() { +fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) { + for cloud in clouds.value().iter() { commands.get_or_spawn(cloud).insert(Cloud); } } ``` You can now also access resources from the render world using the normal system parameters during `Extract`: ```rust fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) { *render_assets = source_assets.clone(); } ``` Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met. Co-authored-by: Carter Anderson <mcanders1@gmail.com> |