9ed0787d41
99 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
urben1680
|
546711b807
|
Split EntityClonerBuilder in OptOut and OptIn variants (#19649)
# Objective Further tests after #19326 showed that configuring `EntityCloner` with required components is bug prone and the current design has several weaknesses in it's API: - Mixing `EntityClonerBuilder::allow` and `EntityClonerBuilder::deny` requires extra care how to support that which has an impact on surrounding code that has to keep edge cases in mind. This is especially true for attempts to fix the following issues. There is no use-case known (to me) why someone would mix those. - A builder with `EntityClonerBuilder::allow_all` configuration tries to support required components like `EntityClonerBuilder::deny_all` does, but the meaning of that is conflicting with how you'd expect things to work: - If all components should be cloned except component `A`, do you also want to exclude required components of `A` too? Or are these also valid without `A` at the target entity? - If `EntityClonerBuilder::allow_all` should ignore required components and not add them to be filtered away, which purpose has `EntityClonerBuilder::without_required_components` for this cloner? - Other bugs found with the linked PR are: - Denying `A` also denies required components of `A` even when `A` does not exist at the source entity - Allowing `A` also allows required components of `A` even when `A` does not exist at the source entity - Adding `allow_if_new` filters to the cloner faces the same issues and require a common solution to dealing with source-archetype sensitive cloning Alternative to #19632 and #19635. # Solution `EntityClonerBuilder` is made generic and split into `EntityClonerBuilder<OptOut>` and `EntityClonerBuilder<OptIn>` For an overview of the changes, see the migration guide. It is generally a good idea to start a review of that. ## Algorithm The generic of `EntityClonerBuilder` contains the filter data that is needed to build and clone the entity components. As the filter needs to be borrowed mutably for the duration of the clone, the borrow checker forced me to separate the filter value and all other fields in `EntityCloner`. The latter are now in the `EntityClonerConfig` struct. This caused many changed LOC, sorry. To make reviewing easier: 1. Check the migration guide 2. Many methods of `EntityCloner` now just call identitcal `EntityClonerConfig` methods with a mutable borrow of the filter 3. Check `EntityClonerConfig::clone_entity_internal` which changed a bit regarding the filter usage that is now trait powered (`CloneByFilter`) to support `OptOut`, `OptIn` and `EntityClonerFilter` (an enum combining the first two) 4. Check `OptOut` type that no longer tracks required components but has a `insert_mode` field 5. Check `OptIn` type that has the most logic changes # Testing I added a bunch of tests that cover the new logic parts and the fixed issues. Benchmarks are in a comment a bit below which shows ~4% to 9% regressions, but it varied wildly for me. For example at one run the reflection-based clonings were on-par with main while the other are not, and redoing that swapped the situation for both. It would be really cool if I could get some hints how to get better benchmark results or if you could run them on your machine too. Just be aware this is not a Performance PR but a Bugfix PR, even if I smuggled in some more functionalities. So doing changes to `EntityClonerBuilder` is kind of required here which might make us bite the bullet. --------- Co-authored-by: eugineerd <70062110+eugineerd@users.noreply.github.com> |
||
|
mgi388
|
efd17f133d
|
Rename num_entities to entity_count (#19781)
As discussed in https://github.com/bevyengine/bevy/pull/19780#issuecomment-2994554024. |
||
|
Alice Cecile
|
61a5a37584
|
Improve module structure of observers code (#19779)
# Objective While working on #17607, I found myself confused and frustrated by the tangled web woven by the various modules inside of our observers code. Rather than tackle that as part of a big rewrite PR, I've decided to do the mature (if frustrating) thing where you split out your trivial but noisy refactoring first. There are a large number of moving parts, especially in terms of storage, and these are strewn willy-nilly across the module with no apparent ordering. To make matters worse, this was almost all just dumped into a multi-thousand LOC mod.rs at the root. ## Solution I've reshuffled the modules, attempting to: - reduce the size of the mod.rs file - organize structs so that smaller structs are found after the larger structs that contain them - group related functionality together - document why modules exist, and their broad organization No functional changes have been made here, although I've had to increase the visibility of a few fields from private to pub(crate) or pub(super) to keep things compiling. During these changes, I've opted for the lazy private module, public re-export strategy, to avoid causing any breakages, both within and outside of `bevy` itself. I think we can do better, but I want to leave that for a proper cleanup pass at the end. There's no sense maintaining migration guides and forcing multiple breaking changes throughout the cycle. ## Testing No functional changes; relying on existing test suite and the Rust compiler. |
||
|
Trashtalk217
|
6dbe3600ed
|
Add num_entities() to World (#19780)
# Objective There is a lot of `world.entities().len()`, especially in tests. In tests, usually, the assumption is made that empty worlds do not contain any entities. This is about to change (#19711), and as such all of these tests are failing for that PR. ## Solution `num_entities` is a convenience method that returns the number of entities inside a world. It can later be adapted to exclude 'unexpected' entities, associated with internal data structures such as Resources, Queries, Systems. In general I argue for a separation of concepts where `World` ignores internal entities in methods such as `iter_entities()` and `clear_entities()`, that discussion is, however, separate from this PR. ## Testing I replaced most occurrences of `world.entities().len()` with `world.num_entities()` and the tests passed. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Lailatova
|
5d5a95fa6e
|
Fix issue 19734: add dependency on bevy_utils for the bevy_ecs test. (#19738)
Without this dependency, the bevy_ecs tests fail with missing as_string methods. # Objective - Fixes #19734 ## Solution - add bevy_utils with feature = "Debug" to dev-dependencies ## Testing - Ran `cargo test -p bevy_ecs` - Ran `taplo fmt --check` --- |
||
|
François Mockers
|
4e694aea53
|
ECS: put strings only used for debug behind a feature (#19558)
# Objective - Many strings in bevy_ecs are created but only used for debug: system name, component name, ... - Those strings make a significant part of the final binary and are no use in a released game ## Solution - Use [`strings`](https://linux.die.net/man/1/strings) to find ... strings in a binary - Try to find where they come from - Many are made from `type_name::<T>()` and only used in error / debug messages - Add a new structure `DebugName` that holds no value if `debug` feature is disabled - Replace `core::any::type_name::<T>()` by `DebugName::type_name::<T>()` ## Testing Measurements were taken without the new feature being enabled by default, to help with commands ### File Size I tried building the `breakout` example with `cargo run --release --example breakout` |`debug` enabled|`debug` disabled| |-|-| |81621776 B|77735728B| |77.84MB|74.13MB| ### Compilation time `hyperfine --min-runs 15 --prepare "cargo clean && sleep 5" 'RUSTC_WRAPPER="" cargo build --release --example breakout' 'RUSTC_WRAPPER="" cargo build --release --example breakout --features debug'` ``` breakout' 'RUSTC_WRAPPER="" cargo build --release --example breakout --features debug' Benchmark 1: RUSTC_WRAPPER="" cargo build --release --example breakout Time (mean ± σ): 84.856 s ± 3.565 s [User: 1093.817 s, System: 32.547 s] Range (min … max): 78.038 s … 89.214 s 15 runs Benchmark 2: RUSTC_WRAPPER="" cargo build --release --example breakout --features debug Time (mean ± σ): 92.303 s ± 2.466 s [User: 1193.443 s, System: 33.803 s] Range (min … max): 90.619 s … 99.684 s 15 runs Summary RUSTC_WRAPPER="" cargo build --release --example breakout ran 1.09 ± 0.05 times faster than RUSTC_WRAPPER="" cargo build --release --example breakout --features debug ``` |
||
|
Lucas Franca
|
6f08bb84d2
|
Exposes Observer's system's name (#19611)
# Objective Fixes #18726 Alternative to and closes #18797 ## Solution Create a method `Observer::system_name` to expose the name of the `Observer`'s system ## Showcase ```rust // Returns `my_crate::my_observer` let observer = Observer::new(my_observer); println!(observer.system_name()); // Returns `my_crate::method::{{closure}}` let observer = Observer::new(|_trigger: Trigger<...>|); println!(observer.system_name()); // Returns `custom_name` let observer = Observer::new(IntoSystem::into_system(my_observer).with_name("custom_name")); println!(observer.system_name()); ``` ## TODO - [ ] Achieve cart's approval --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Alice Cecile
|
7237c2173b
|
Make observers metadata / storage publicly visible (#19608)
# Objective Our strategy for storing observers is made up of several moving parts, which are ultimately fairly simple nested HashMaps. These types are currently `pub`, but lack any meaningful way to access this data. We have three options here: 1. Make these internals not `pub` at all. 2. Make the data read-only accessible. 3. Make the data mutably accessible. ## Solution I've opted for option 2, exposing read-only values. This is consistent with our existing approach to the ECS internals, allowing for easier debugging without risking wanton data corruption. If one day you would like to mutably access this data, please open an issue clearly explaining what you're trying to do. This was a pretty mechanical change, exposing fields via getters. I've also opted to do my best to clarify some field names and documentation: please double-check those for correctness. It was hard to be fully confident, as the field names and documentation was not very clear ;) ## Testing I spent some time going through the code paths, making sure that users can trace all the way from `World` to the leaf nodes. Reviewers, please ensure the same! ## Notes for reviewers This is part of a broader observer overhaul: I fully expect us to change up these internals and break these shiny new APIs. Probably even within the same cycle! But clean up your work area first: this sort of read-only getter and improved docs will be important to replace as we work. |
||
|
Chris Russell
|
f7e112a3c9
|
Let query items borrow from query state to avoid needing to clone (#15396)
# Objective Improve the performance of `FilteredEntity(Ref|Mut)` and `Entity(Ref|Mut)Except`. `FilteredEntityRef` needs an `Access<ComponentId>` to determine what components it can access. There is one stored in the query state, but query items cannot borrow from the state, so it has to `clone()` the access for each row. Cloning the access involves memory allocations and can be expensive. ## Solution Let query items borrow from their query state. Add an `'s` lifetime to `WorldQuery::Item` and `WorldQuery::Fetch`, similar to the one in `SystemParam`, and provide `&'s Self::State` to the fetch so that it can borrow from the state. Unfortunately, there are a few cases where we currently return query items from temporary query states: the sorted iteration methods create a temporary state to query the sort keys, and the `EntityRef::components<Q>()` methods create a temporary state for their query. To allow these to continue to work with most `QueryData` implementations, introduce a new subtrait `ReleaseStateQueryData` that converts a `QueryItem<'w, 's>` to `QueryItem<'w, 'static>`, and is implemented for everything except `FilteredEntity(Ref|Mut)` and `Entity(Ref|Mut)Except`. `#[derive(QueryData)]` will generate `ReleaseStateQueryData` implementations that apply when all of the subqueries implement `ReleaseStateQueryData`. This PR does not actually change the implementation of `FilteredEntity(Ref|Mut)` or `Entity(Ref|Mut)Except`! That will be done as a follow-up PR so that the changes are easier to review. I have pushed the changes as chescock/bevy#5. ## Testing I ran performance traces of many_foxes, both against main and against chescock/bevy#5, both including #15282. These changes do appear to make generalized animation a bit faster: (Red is main, yellow is chescock/bevy#5)  ## Migration Guide The `WorldQuery::Item` and `WorldQuery::Fetch` associated types and the `QueryItem` and `ROQueryItem` type aliases now have an additional lifetime parameter corresponding to the `'s` lifetime in `Query`. Manual implementations of `WorldQuery` will need to update the method signatures to include the new lifetimes. Other uses of the types will need to be updated to include a lifetime parameter, although it can usually be passed as `'_`. In particular, `ROQueryItem` is used when implementing `RenderCommand`. Before: ```rust fn render<'w>( item: &P, view: ROQueryItem<'w, Self::ViewQuery>, entity: Option<ROQueryItem<'w, Self::ItemQuery>>, param: SystemParamItem<'w, '_, Self::Param>, pass: &mut TrackedRenderPass<'w>, ) -> RenderCommandResult; ``` After: ```rust fn render<'w>( item: &P, view: ROQueryItem<'w, '_, Self::ViewQuery>, entity: Option<ROQueryItem<'w, '_, Self::ItemQuery>>, param: SystemParamItem<'w, '_, Self::Param>, pass: &mut TrackedRenderPass<'w>, ) -> RenderCommandResult; ``` --- Methods on `QueryState` that take `&mut self` may now result in conflicting borrows if the query items capture the lifetime of the mutable reference. This affects `get()`, `iter()`, and others. To fix the errors, first call `QueryState::update_archetypes()`, and then replace a call `state.foo(world, param)` with `state.query_manual(world).foo_inner(param)`. Alternately, you may be able to restructure the code to call `state.query(world)` once and then make multiple calls using the `Query`. Before: ```rust let mut state: QueryState<_, _> = ...; let d1 = state.get(world, e1); let d2 = state.get(world, e2); // Error: cannot borrow `state` as mutable more than once at a time println!("{d1:?}"); println!("{d2:?}"); ``` After: ```rust let mut state: QueryState<_, _> = ...; state.update_archetypes(world); let d1 = state.get_manual(world, e1); let d2 = state.get_manual(world, e2); // OR state.update_archetypes(world); let d1 = state.query(world).get_inner(e1); let d2 = state.query(world).get_inner(e2); // OR let query = state.query(world); let d1 = query.get_inner(e1); let d1 = query.get_inner(e2); println!("{d1:?}"); println!("{d2:?}"); ``` |
||
|
Alice Cecile
|
b7d2cb8547
|
Provide access to the original target of entity-events in observers (#19663)
# Objective Getting access to the original target of an entity-event is really helpful when working with bubbled / propagated events. `bevy_picking` special-cases this, but users have requested this for all sorts of bubbled events. The existing naming convention was also very confusing. Fixes https://github.com/bevyengine/bevy/issues/17112, but also see #18982. ## Solution 1. Rename `ObserverTrigger::target` -> `current_target`. 1. Store `original_target: Option<Entity>` in `ObserverTrigger`. 1. Wire it up so this field gets set correctly. 1. Remove the `target` field on the `Pointer` events from `bevy_picking`. Closes https://github.com/bevyengine/bevy/pull/18710, which attempted the same thing. Thanks @emfax! ## Testing I've modified an existing test to check that the entities returned during event bubbling / propagation are correct. ## Notes to reviewers It's a little weird / sad that you can no longer access this infromation via the buffered events for `Pointer`. That said, you already couldn't access any bubbled target. We should probably remove the `BufferedEvent` form of `Pointer` to reduce confusion and overhead, but I didn't want to do so here. Observer events can be trivially converted into buffered events (write an observer with an EventWriter), and I suspect that that is the better migration if you want the controllable timing or performance characteristics of buffered events for your specific use case. ## Future work It would be nice to not store this data at all (and not expose any methods) if propagation was disabled. That involves more trait shuffling, and I don't think we should do it here for reviewability. --------- Co-authored-by: Joona Aalto <jondolf.dev@gmail.com> |
||
|
Joona Aalto
|
38c3423693
|
Event Split: Event, EntityEvent, and BufferedEvent (#19647)
# Objective Closes #19564. The current `Event` trait looks like this: ```rust pub trait Event: Send + Sync + 'static { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } ``` The `Event` trait is used by both buffered events (`EventReader`/`EventWriter`) and observer events. If they are observer events, they can optionally be targeted at specific `Entity`s or `ComponentId`s, and can even be propagated to other entities. However, there has long been a desire to split the trait semantically for a variety of reasons, see #14843, #14272, and #16031 for discussion. Some reasons include: - It's very uncommon to use a single event type as both a buffered event and targeted observer event. They are used differently and tend to have distinct semantics. - A common footgun is using buffered events with observers or event readers with observer events, as there is no type-level error that prevents this kind of misuse. - #19440 made `Trigger::target` return an `Option<Entity>`. This *seriously* hurts ergonomics for the general case of entity observers, as you need to `.unwrap()` each time. If we could statically determine whether the event is expected to have an entity target, this would be unnecessary. There's really two main ways that we can categorize events: push vs. pull (i.e. "observer event" vs. "buffered event") and global vs. targeted: | | Push | Pull | | ------------ | --------------- | --------------------------- | | **Global** | Global observer | `EventReader`/`EventWriter` | | **Targeted** | Entity observer | - | There are many ways to approach this, each with their tradeoffs. Ultimately, we kind of want to split events both ways: - A type-level distinction between observer events and buffered events, to prevent people from using the wrong kind of event in APIs - A statically designated entity target for observer events to avoid accidentally using untargeted events for targeted APIs This PR achieves these goals by splitting event traits into `Event`, `EntityEvent`, and `BufferedEvent`, with `Event` being the shared trait implemented by all events. ## `Event`, `EntityEvent`, and `BufferedEvent` `Event` is now a very simple trait shared by all events. ```rust pub trait Event: Send + Sync + 'static { // Required for observer APIs fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } ``` You can call `trigger` for *any* event, and use a global observer for listening to the event. ```rust #[derive(Event)] struct Speak { message: String, } // ... app.add_observer(|trigger: On<Speak>| { println!("{}", trigger.message); }); // ... commands.trigger(Speak { message: "Y'all like these reworked events?".to_string(), }); ``` To allow an event to be targeted at entities and even propagated further, you can additionally implement the `EntityEvent` trait: ```rust pub trait EntityEvent: Event { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; } ``` This lets you call `trigger_targets`, and to use targeted observer APIs like `EntityCommands::observe`: ```rust #[derive(Event, EntityEvent)] #[entity_event(traversal = &'static ChildOf, auto_propagate)] struct Damage { amount: f32, } // ... let enemy = commands.spawn((Enemy, Health(100.0))).id(); // Spawn some armor as a child of the enemy entity. // When the armor takes damage, it will bubble the event up to the enemy. let armor_piece = commands .spawn((ArmorPiece, Health(25.0), ChildOf(enemy))) .observe(|trigger: On<Damage>, mut query: Query<&mut Health>| { // Note: `On::target` only exists because this is an `EntityEvent`. let mut health = query.get(trigger.target()).unwrap(); health.0 -= trigger.amount(); }); commands.trigger_targets(Damage { amount: 10.0 }, armor_piece); ``` > [!NOTE] > You *can* still also trigger an `EntityEvent` without targets using `trigger`. We probably *could* make this an either-or thing, but I'm not sure that's actually desirable. To allow an event to be used with the buffered API, you can implement `BufferedEvent`: ```rust pub trait BufferedEvent: Event {} ``` The event can then be used with `EventReader`/`EventWriter`: ```rust #[derive(Event, BufferedEvent)] struct Message(String); fn write_hello(mut writer: EventWriter<Message>) { writer.write(Message("I hope these examples are alright".to_string())); } fn read_messages(mut reader: EventReader<Message>) { // Process all buffered events of type `Message`. for Message(message) in reader.read() { println!("{message}"); } } ``` In summary: - Need a basic event you can trigger and observe? Derive `Event`! - Need the event to be targeted at an entity? Derive `EntityEvent`! - Need the event to be buffered and support the `EventReader`/`EventWriter` API? Derive `BufferedEvent`! ## Alternatives I'll now cover some of the alternative approaches I have considered and briefly explored. I made this section collapsible since it ended up being quite long :P <details> <summary>Expand this to see alternatives</summary> ### 1. Unified `Event` Trait One option is not to have *three* separate traits (`Event`, `EntityEvent`, `BufferedEvent`), and to instead just use associated constants on `Event` to determine whether an event supports targeting and buffering or not: ```rust pub trait Event: Send + Sync + 'static { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; const TARGETED: bool = false; const BUFFERED: bool = false; fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } ``` Methods can then use bounds like `where E: Event<TARGETED = true>` or `where E: Event<BUFFERED = true>` to limit APIs to specific kinds of events. This would keep everything under one `Event` trait, but I don't think it's necessarily a good idea. It makes APIs harder to read, and docs can't easily refer to specific types of events. You can also create weird invariants: what if you specify `TARGETED = false`, but have `Traversal` and/or `AUTO_PROPAGATE` enabled? ### 2. `Event` and `Trigger` Another option is to only split the traits between buffered events and observer events, since that is the main thing people have been asking for, and they have the largest API difference. If we did this, I think we would need to make the terms *clearly* separate. We can't really use `Event` and `BufferedEvent` as the names, since it would be strange that `BufferedEvent` doesn't implement `Event`. Something like `ObserverEvent` and `BufferedEvent` could work, but it'd be more verbose. For this approach, I would instead keep `Event` for the current `EventReader`/`EventWriter` API, and call the observer event a `Trigger`, since the "trigger" terminology is already used in the observer context within Bevy (both as a noun and a verb). This is also what a long [bikeshed on Discord](https://discord.com/channels/691052431525675048/749335865876021248/1298057661878898791) seemed to land on at the end of last year. ```rust // For `EventReader`/`EventWriter` pub trait Event: Send + Sync + 'static {} // For observers pub trait Trigger: Send + Sync + 'static { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; const TARGETED: bool = false; fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } ``` The problem is that "event" is just a really good term for something that "happens". Observers are rapidly becoming the more prominent API, so it'd be weird to give them the `Trigger` name and leave the good `Event` name for the less common API. So, even though a split like this seems neat on the surface, I think it ultimately wouldn't really work. We want to keep the `Event` name for observer events, and there is no good alternative for the buffered variant. (`Message` was suggested, but saying stuff like "sends a collision message" is weird.) ### 3. `GlobalEvent` + `TargetedEvent` What if instead of focusing on the buffered vs. observed split, we *only* make a distinction between global and targeted events? ```rust // A shared event trait to allow global observers to work pub trait Event: Send + Sync + 'static { fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } // For buffered events and non-targeted observer events pub trait GlobalEvent: Event {} // For targeted observer events pub trait TargetedEvent: Event { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; } ``` This is actually the first approach I implemented, and it has the neat characteristic that you can only use non-targeted APIs like `trigger` with a `GlobalEvent` and targeted APIs like `trigger_targets` with a `TargetedEvent`. You have full control over whether the entity should or should not have a target, as they are fully distinct at the type-level. However, there's a few problems: - There is no type-level indication of whether a `GlobalEvent` supports buffered events or just non-targeted observer events - An `Event` on its own does literally nothing, it's just a shared trait required to make global observers accept both non-targeted and targeted events - If an event is both a `GlobalEvent` and `TargetedEvent`, global observers again have ambiguity on whether an event has a target or not, undermining some of the benefits - The names are not ideal ### 4. `Event` and `EntityEvent` We can fix some of the problems of Alternative 3 by accepting that targeted events can also be used in non-targeted contexts, and simply having the `Event` and `EntityEvent` traits: ```rust // For buffered events and non-targeted observer events pub trait Event: Send + Sync + 'static { fn register_component_id(world: &mut World) -> ComponentId { ... } fn component_id(world: &World) -> Option<ComponentId> { ... } } // For targeted observer events pub trait EntityEvent: Event { type Traversal: Traversal<Self>; const AUTO_PROPAGATE: bool = false; } ``` This is essentially identical to this PR, just without a dedicated `BufferedEvent`. The remaining major "problem" is that there is still zero type-level indication of whether an `Event` event *actually* supports the buffered API. This leads us to the solution proposed in this PR, using `Event`, `EntityEvent`, and `BufferedEvent`. </details> ## Conclusion The `Event` + `EntityEvent` + `BufferedEvent` split proposed in this PR aims to solve all the common problems with Bevy's current event model while keeping the "weirdness" factor minimal. It splits in terms of both the push vs. pull *and* global vs. targeted aspects, while maintaining a shared concept for an "event". ### Why I Like This - The term "event" remains as a single concept for all the different kinds of events in Bevy. - Despite all event types being "events", they use fundamentally different APIs. Instead of assuming that you can use an event type with any pattern (when only one is typically supported), you explicitly opt in to each one with dedicated traits. - Using separate traits for each type of event helps with documentation and clearer function signatures. - I can safely make assumptions on expected usage. - If I see that an event is an `EntityEvent`, I can assume that I can use `observe` on it and get targeted events. - If I see that an event is a `BufferedEvent`, I can assume that I can use `EventReader` to read events. - If I see both `EntityEvent` and `BufferedEvent`, I can assume that both APIs are supported. In summary: This allows for a unified concept for events, while limiting the different ways to use them with opt-in traits. No more guess-work involved when using APIs. ### Problems? - Because `BufferedEvent` implements `Event` (for more consistent semantics etc.), you can still use all buffered events for non-targeted observers. I think this is fine/good. The important part is that if you see that an event implements `BufferedEvent`, you know that the `EventReader`/`EventWriter` API should be supported. Whether it *also* supports other APIs is secondary. - I currently only support `trigger_targets` for an `EntityEvent`. However, you can technically target components too, without targeting any entities. I consider that such a niche and advanced use case that it's not a huge problem to only support it for `EntityEvent`s, but we could also split `trigger_targets` into `trigger_entities` and `trigger_components` if we wanted to (or implement components as entities :P). - You can still trigger an `EntityEvent` *without* targets. I consider this correct, since `Event` implements the non-targeted behavior, and it'd be weird if implementing another trait *removed* behavior. However, it does mean that global observers for entity events can technically return `Entity::PLACEHOLDER` again (since I got rid of the `Option<Entity>` added in #19440 for ergonomics). I think that's enough of an edge case that it's not a huge problem, but it is worth keeping in mind. - ~~Deriving both `EntityEvent` and `BufferedEvent` for the same type currently duplicates the `Event` implementation, so you instead need to manually implement one of them.~~ Changed to always requiring `Event` to be derived. ## Related Work There are plans to implement multi-event support for observers, especially for UI contexts. [Cart's example](https://github.com/bevyengine/bevy/issues/14649#issuecomment-2960402508) API looked like this: ```rust // Truncated for brevity trigger: Trigger<( OnAdd<Pressed>, OnRemove<Pressed>, OnAdd<InteractionDisabled>, OnRemove<InteractionDisabled>, OnInsert<Hovered>, )>, ``` I believe this shouldn't be in conflict with this PR. If anything, this PR might *help* achieve the multi-event pattern for entity observers with fewer footguns: by statically enforcing that all of these events are `EntityEvent`s in the context of `EntityCommands::observe`, we can avoid misuse or weird cases where *some* events inside the trigger are targeted while others are not. |
||
|
Alice Cecile
|
58c276ab44
|
Make the ObservedBy component useful to public consumers (#19591)
# Objective As raised by @Jondolf, this type is `pub`, and useful for various consumers to ensure cleanup or debugging. However, it doesn't offer any way to actually view the data. ## Solution - Add a read-only view of the data. - Don't add any (easy) way to mutate the data, as this presents a huge footgun. - Implement Reflect and register the component so you can see it in inspectors nicely. |
||
|
Joona Aalto
|
e5dc177b4b
|
Rename Trigger to On (#19596)
# Objective
Currently, the observer API looks like this:
```rust
app.add_observer(|trigger: Trigger<Explode>| {
info!("Entity {} exploded!", trigger.target());
});
```
Future plans for observers also include "multi-event observers" with a
trigger that looks like this (see [Cart's
example](https://github.com/bevyengine/bevy/issues/14649#issuecomment-2960402508)):
```rust
trigger: Trigger<(
OnAdd<Pressed>,
OnRemove<Pressed>,
OnAdd<InteractionDisabled>,
OnRemove<InteractionDisabled>,
OnInsert<Hovered>,
)>,
```
In scenarios like this, there is a lot of repetition of `On`. These are
expected to be very high-traffic APIs especially in UI contexts, so
ergonomics and readability are critical.
By renaming `Trigger` to `On`, we can make these APIs read more cleanly
and get rid of the repetition:
```rust
app.add_observer(|trigger: On<Explode>| {
info!("Entity {} exploded!", trigger.target());
});
```
```rust
trigger: On<(
Add<Pressed>,
Remove<Pressed>,
Add<InteractionDisabled>,
Remove<InteractionDisabled>,
Insert<Hovered>,
)>,
```
Names like `On<Add<Pressed>>` emphasize the actual event listener nature
more than `Trigger<OnAdd<Pressed>>`, and look cleaner. This *also* frees
up the `Trigger` name if we want to use it for the observer event type,
splitting them out from buffered events (bikeshedding this is out of
scope for this PR though).
For prior art:
[`bevy_eventlistener`](https://github.com/aevyrie/bevy_eventlistener)
used
[`On`](https://docs.rs/bevy_eventlistener/latest/bevy_eventlistener/event_listener/struct.On.html)
for its event listener type. Though in our case, the observer is the
event listener, and `On` is just a type containing information about the
triggered event.
## Solution
Steal from `bevy_event_listener` by @aevyrie and use `On`.
- Rename `Trigger` to `On`
- Rename `OnAdd` to `Add`
- Rename `OnInsert` to `Insert`
- Rename `OnReplace` to `Replace`
- Rename `OnRemove` to `Remove`
- Rename `OnDespawn` to `Despawn`
## Discussion
### Naming Conflicts??
Using a name like `Add` might initially feel like a very bad idea, since
it risks conflict with `core::ops::Add`. However, I don't expect this to
be a big problem in practice.
- You rarely need to actually implement the `Add` trait, especially in
modules that would use the Bevy ECS.
- In the rare cases where you *do* get a conflict, it is very easy to
fix by just disambiguating, for example using `ops::Add`.
- The `Add` event is a struct while the `Add` trait is a trait (duh), so
the compiler error should be very obvious.
For the record, renaming `OnAdd` to `Add`, I got exactly *zero* errors
or conflicts within Bevy itself. But this is of course not entirely
representative of actual projects *using* Bevy.
You might then wonder, why not use `Added`? This would conflict with the
`Added` query filter, so it wouldn't work. Additionally, the current
naming convention for observer events does not use past tense.
### Documentation
This does make documentation slightly more awkward when referring to
`On` or its methods. Previous docs often referred to `Trigger::target`
or "sends a `Trigger`" (which is... a bit strange anyway), which would
now be `On::target` and "sends an observer `Event`".
You can see the diff in this PR to see some of the effects. I think it
should be fine though, we may just need to reword more documentation to
read better.
|
||
|
Alice Cecile
|
12f8f9c07c
|
Thoroughly document the current state of observers (#19590)
# Objective The documentation for observers is not very good. This poses a problem to users, but *also* causes serious problems for engine devs, as they attempt to improve assorted issues surrounding observers. This PR: - Fixes #14084. - Fixes #14726. - Fixes #16538. - Closes #18914, by attempting to solve the same issue. To keep this PR at all reviewable, I've opted to simply note the various limitations (some may call them bugs!) in place, rather than attempting to fix them. There is a huge amount of cleanup work to be done here: see https://github.com/orgs/bevyengine/projects/17. ## Solution - Write good module docs for observers, offering bread crumbs to the most common methods and techniques and comparing-and-contrasting as needed. - Fix any actively misleading documentation. - Try to explain how the various bits of the (public?!) internals are related. --------- Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com> Co-authored-by: Joona Aalto <jondolf.dev@gmail.com> |
||
|
Alice Cecile
|
6ddd0f16a8
|
Component lifecycle reorganization and documentation (#19543)
# Objective I set out with one simple goal: clearly document the differences between each of the component lifecycle events via module docs. Unfortunately, no such module existed: the various lifecycle code was scattered to the wind. Without a unified module, it's very hard to discover the related types, and there's nowhere good to put my shiny new documentation. ## Solution 1. Unify the assorted types into a single `bevy_ecs::component_lifecycle` module. 2. Write docs. 3. Write a migration guide. ## Testing Thanks CI! ## Follow-up 1. The lifecycle event names are pretty confusing, especially `OnReplace`. We should consider renaming those. No bikeshedding in my PR though! 2. Observers need real module docs too :( 3. Any additional functional changes should be done elsewhere; this is a simple docs and re-org PR. --------- Co-authored-by: theotherphil <phil.j.ellison@gmail.com> |
||
|
Eagster
|
064e5e48b4
|
Remove entity placeholder from observers (#19440)
# Objective `Entity::PLACEHOLDER` acts as a magic number that will *probably* never really exist, but it certainly could. And, `Entity` has a niche, so the only reason to use `PLACEHOLDER` is as an alternative to `MaybeUninit` that trades safety risks for logic risks. As a result, bevy has generally advised against using `PLACEHOLDER`, but we still use if for a lot internally. This pr starts removing internal uses of it, starting from observers. ## Solution Change all trigger target related types from `Entity` to `Option<Entity>` Small migration guide to come. ## Testing CI ## Future Work This turned a lot of code from ```rust trigger.target() ``` to ```rust trigger.target().unwrap() ``` The extra panic is no worse than before; it's just earlier than panicking after passing the placeholder to something else. But this is kinda annoying. I would like to add a `TriggerMode` or something to `Event` that would restrict what kinds of targets can be used for that event. Many events like `Removed` etc, are always triggered with a target. We can make those have a way to assume Some, etc. But I wanted to save that for a future pr. |
||
|
François Mockers
|
7a7bff8c17
|
Hot patching systems with subsecond (#19309)
# Objective - Enable hot patching systems with subsecond - Fixes #19296 ## Solution - First commit is the naive thin layer - Second commit only check the jump table when the code is hot patched instead of on every system execution - Depends on https://github.com/DioxusLabs/dioxus/pull/4153 for a nicer API, but could be done without - Everything in second commit is feature gated, it has no impact when the feature is not enabled ## Testing - Check dependencies without the feature enabled: nothing dioxus in tree - Run the new example: text and color can be changed --------- Co-authored-by: Jan Hohenheim <jan@hohenheim.ch> Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com> |
||
|
Chris Russell
|
571b3ba475
|
Remove ArchetypeComponentId and archetype_component_access (#19143)
# Objective Remove `ArchetypeComponentId` and `archetype_component_access`. Following #16885, they are no longer used by the engine, so we can stop spending time calculating them or space storing them. ## Solution Remove `ArchetypeComponentId` and everything that touches it. The `System::update_archetype_component_access` method no longer needs to update `archetype_component_access`. We do still need to update query caches, but we no longer need to do so *before* running the system. We'd have to touch every caller anyway if we gave the method a better name, so just remove `System::update_archetype_component_access` and `SystemParam::new_archetype` entirely, and update the query cache in `Query::get_param`. The `Single` and `Populated` params also need their query caches updated in `SystemParam::validate_param`, so change `validate_param` to take `&mut Self::State` instead of `&Self::State`. |
||
|
theotherphil
|
54c9f03021
|
Mention in .add_observer() docs that first parameter must be a Trigger (#19315)
# Objective Fix https://github.com/bevyengine/bevy/issues/13860 ## Solution Add note in docs that Trigger must be the first parameter of observer systems |
||
|
SpecificProtagonist
|
e7e9973c80
|
Per world error handler (#18810)
# Objective [see original comment](https://github.com/bevyengine/bevy/pull/18801#issuecomment-2796981745) > Alternately, could we store it on the World instead of a global? I think we have a World nearby whenever we call default_error_handler(). That would avoid the need for atomics or locks, since we could do ordinary reads and writes to the World. Global error handlers don't actually need to be global – per world is enough. This allows using different handlers for different worlds and also removes the restrictions on changing the handler only once. ## Solution Each `World` can now store its own error handler in a resource. For convenience, you can also set the default error handler for an `App`, which applies it to the worlds of all `SubApp`s. The old behavior of only being able to set the error handler once is kept for apps. We also don't need the `configurable_error_handler` feature anymore now. ## Testing New/adjusted tests for failing schedule systems & observers. --- ## Showcase ```rust App::new() .set_error_handler(info) … ``` |
||
|
Eagster
|
0b4858726c
|
Make entity::index non max (#18704)
# Objective There are two problems this aims to solve. First, `Entity::index` is currently a `u32`. That means there are `u32::MAX + 1` possible entities. Not only is that awkward, but it also make `Entity` allocation more difficult. I discovered this while working on remote entity reservation, but even on main, `Entities` doesn't handle the `u32::MAX + 1` entity very well. It can not be batch reserved because that iterator uses exclusive ranges, which has a maximum upper bound of `u32::MAX - 1`. In other words, having `u32::MAX` as a valid index can be thought of as a bug right now. We either need to make that invalid (this PR), which makes Entity allocation cleaner and makes remote reservation easier (because the length only needs to be u32 instead of u64, which, in atomics is a big deal), or we need to take another pass at `Entities` to make it handle the `u32::MAX` index properly. Second, `TableRow`, `ArchetypeRow` and `EntityIndex` (a type alias for u32) all have `u32` as the underlying type. That means using these as the index type in a `SparseSet` uses 64 bits for the sparse list because it stores `Option<IndexType>`. By using `NonMaxU32` here, we cut the memory of that list in half. To my knowledge, `EntityIndex` is the only thing that would really benefit from this niche. `TableRow` and `ArchetypeRow` I think are not stored in an `Option` in bulk. But if they ever are, this would help. Additionally this ensures `TableRow::INVALID` and `ArchetypeRow::INVALID` never conflict with an actual row, which in a nice bonus. As a related note, if we do components as entities where `ComponentId` becomes `Entity`, the the `SparseSet<ComponentId>` will see a similar memory improvement too. ## Solution Create a new type `EntityRow` that wraps `NonMaxU32`, similar to `TableRow` and `ArchetypeRow`. Change `Entity::index` to this type. ## Downsides `NonMax` is implemented as a `NonZero` with a binary inversion. That means accessing and storing the value takes one more instruction. I don't think that's a big deal, but it's worth mentioning. As a consequence, `to_bits` uses `transmute` to skip the inversion which keeps it a nop. But that also means that ordering has now flipped. In other words, higher indices are considered less than lower indices. I don't think that's a problem, but it's also worth mentioning. ## Alternatives We could keep the index as a u32 type and just document that `u32::MAX` is invalid, modifying `Entities` to ensure it never gets handed out. (But that's not enforced by the type system.) We could still take advantage of the niche here in `ComponentSparseSet`. We'd just need some unsafe manual conversions, which is probably fine, but opens up the possibility for correctness problems later. We could change `Entities` to fully support the `u32::MAX` index. (But that makes `Entities` more complex and potentially slightly slower.) ## Testing - CI - A few tests were changed because they depend on different ordering and `to_bits` values. ## Future Work - It might be worth removing the niche on `Entity::generation` since there is now a different niche. - We could move `Entity::generation` into it's own type too for clarity. - We should change `ComponentSparseSet` to take advantage of the new niche. (This PR doesn't change that yet.) - Consider removing or updating `Identifier`. This is only used for `Entity`, so it might be worth combining since `Entity` is now more unique. --------- Co-authored-by: atlv <email@atlasdostal.com> Co-authored-by: Zachary Harrold <zac@harrold.com.au> |
||
|
re0312
|
5ed8e0639a
|
Merge ObserverState and Observer into single component (#18728)
# Objective - bevy removed `Observe` type parameters in #15151 ,it enables merging `Observer` and `ObserverState ` into a single component. with this consolidation ,we can improve efficiency while reducing boilerplate. ## Solution - remove `ObserverState `and merge it into `Observer` ## Testing 40%~60% performance win due to removal of redundant look up.  This also improves ergonomics when using dynamic observer ```rust // previously world.spawn(ObserverState { // SAFETY: we registered `event_a` above and it matches the type of EventA descriptor: unsafe { ObserverDescriptor::default().with_events(vec![event_a]) }, runner: |mut world, _trigger, _ptr, _propagate| { world.resource_mut::<Order>().observed("event_a"); }, ..Default::default() }); // now let observe = unsafe { Observer::with_dynamic_runner(|mut world, _trigger, _ptr, _propagate| { world.resource_mut::<Order>().observed("event_a"); }) .with_event(event_a) }; world.spawn(observe); ``` |
||
|
Chris Russell
|
5f936aefc8
|
Prevent exclusive systems from being used as observers (#19033)
# Objective Prevent using exclusive systems as observers. Allowing them is unsound, because observers are only expected to have `DeferredWorld` access, and the observer infrastructure will keep pointers that are invalidated by the creation of `&mut World`. See https://github.com/bevyengine/bevy/actions/runs/14778342801/job/41491517847?pr=19011 for a MIRI failure in a recent PR caused by an exclusive system being used as an observer in a test. ## Solution Have `Observer::new` panic if `System::is_exclusive()` is true. Document that method, and methods that call it, as panicking. (It should be possible to express this in the type system so that the calls won't even compile, but I did not want to attempt that.) ## Testing Added a unit test that calls `World::add_observer` with an exclusive system. |
||
|
Greeble
|
b516e78317
|
Bump crate-ci/typos from 1.31.1 to 1.32.0 (#19072)
Adopted #19066. Bumps [crate-ci/typos](https://github.com/crate-ci/typos) from 1.31.1 to 1.32.0. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
Carter Anderson
|
e9a0ef49f9
|
Rename bevy_platform_support to bevy_platform (#18813)
# Objective The goal of `bevy_platform_support` is to provide a set of platform agnostic APIs, alongside platform-specific functionality. This is a high traffic crate (providing things like HashMap and Instant). Especially in light of https://github.com/bevyengine/bevy/discussions/18799, it deserves a friendlier / shorter name. Given that it hasn't had a full release yet, getting this change in before Bevy 0.16 makes sense. ## Solution - Rename `bevy_platform_support` to `bevy_platform`. |
||
|
Freyja-moth
|
714b4a43d6
|
Change with_related to work with a Bundle and added with_relationships method (#18699)
# Objective Fixes #18678 ## Solution Moved the current `with_related` method to `with_relationships` and added a new `with_related` that uses a bundle. I'm not entirely sold on the name just yet, if anyone has any ideas let me know. ## Testing I wasn't able to test these changes because it crashed my computer every time I tried (fun). But there don't seem to be any tests that use the old `with_related` method so it should be fine, hopefully ## Showcase ```rust commands.spawn_empty() .with_related::<Relationship>(Name::new("Related thingy")) .with_relationships(|rel| { rel.spawn(Name::new("Second related thingy")); }); ``` --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com> |
||
|
Vic
|
f57c7a43c4
|
reexport entity set collections in entity module (#18413)
# Objective Unlike for their helper typers, the import paths for `unique_array::UniqueEntityArray`, `unique_slice::UniqueEntitySlice`, `unique_vec::UniqueEntityVec`, `hash_set::EntityHashSet`, `hash_map::EntityHashMap`, `index_set::EntityIndexSet`, `index_map::EntityIndexMap` are quite redundant. When looking at the structure of `hashbrown`, we can also see that while both `HashSet` and `HashMap` have their own modules, the main types themselves are re-exported to the crate level. ## Solution Re-export the types in their shared `entity` parent module, and simplify the imports where they're used. |
||
|
Chris Russell
|
5d1fe16bfd
|
Fix run_system for adapter systems wrapping exclusive systems (#18406)
# Objective Fix panic in `run_system` when running an exclusive system wrapped in a `PipeSystem` or `AdapterSystem`. #18076 introduced a `System::run_without_applying_deferred` method. It normally calls `System::run_unsafe`, but `ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for that type. Unfortunately, `PipeSystem::run_without_applying_deferred` still calls `PipeSystem::run_unsafe`, which can then call `ExclusiveFunctionSystem::run_unsafe` and panic. ## Solution Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking. Clarify the safety requirements that make this sound. The alternative is to override `run_without_applying_deferred` in `PipeSystem`, `CombinatorSystem`, `AdapterSystem`, `InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems like a lot of extra code just to preserve a confusing special case! Remove some implementations of `System::run` that are no longer necessary with this change. This slightly changes the behavior of `PipeSystem` and `CombinatorSystem`: Currently `run` will call `apply_deferred` on the first system before running the second, but after this change it will only call it after *both* systems have run. The new behavior is consistent with `run_unsafe` and `run_without_applying_deferred`, and restores the behavior prior to #11823. The panic was originally necessary because [`run_unsafe` took `&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90). Now that it takes `UnsafeWorldCell`, it is possible to make it work. See also Cart's concerns at https://github.com/bevyengine/bevy/pull/4166#discussion_r979140356, although those also predate `UnsafeWorldCell`. And see #6698 for a previous bug caused by this panic. |
||
|
Chris Russell
|
837991a5b5
|
Replace ValidationOutcome with Result (#18541)
# Objective Make it easier to short-circuit system parameter validation. Simplify the API surface by combining `ValidationOutcome` with `SystemParamValidationError`. ## Solution Replace `ValidationOutcome` with `Result<(), SystemParamValidationError>`. Move the docs from `ValidationOutcome` to `SystemParamValidationError`. Add a `skipped` field to `SystemParamValidationError` to distinguish the `Skipped` and `Invalid` variants. Use the `?` operator to short-circuit validation in tuples of system params. |
||
|
Eagster
|
834260845a
|
Ensure spawning related entities in an OnAdd observer downstream of a World::spawn in a Command does not cause a crash (#18545)
# Objective fixes #18452. ## Solution Spawning used to flush commands only, but those commands can reserve entities. Now, spawning flushes everything, including reserved entities. I checked, and this was the only place where `flush_commands` is used instead of `flush` by mistake. ## Testing I simplified the MRE from #18452 into its own test, which fails on main, but passes on this branch. |
||
|
Alice Cecile
|
6a981aaa6f
|
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
|
ce7d4e41d6
|
Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER (#18454)
# Objective 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). ## Solution 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. ## Testing 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 🥲 ## Questions for reviewers 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. ## Migration Guide `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> |
||
|
Carter Anderson
|
a033f1b206
|
Replace VisitEntities with MapEntities (#18432)
# Objective There are currently too many disparate ways to handle entity mapping, especially after #17687. We now have MapEntities, VisitEntities, VisitEntitiesMut, Component::visit_entities, Component::visit_entities_mut. Our only known use case at the moment for these is entity mapping. This means we have significant consolidation potential. Additionally, VisitEntitiesMut cannot be implemented for map-style collections like HashSets, as you cant "just" mutate a `&mut Entity`. Our current approach to Component mapping requires VisitEntitiesMut, meaning this category of entity collection isn't mappable. `MapEntities` is more generally applicable. Additionally, the _existence_ of the blanket From impl on VisitEntitiesMut blocks us from implementing MapEntities for HashSets (or any types we don't own), because the owner could always add a conflicting impl in the future. ## Solution Use `MapEntities` everywhere and remove all "visit entities" usages. * Add `Component::map_entities` * Remove `Component::visit_entities`, `Component::visit_entities_mut`, `VisitEntities`, and `VisitEntitiesMut` * Support deriving `Component::map_entities` in `#[derive(Coomponent)]` * Add `#[derive(MapEntities)]`, and share logic with the `Component::map_entities` derive. * Add `ComponentCloneCtx::queue_deferred`, which is command-like logic that runs immediately after normal clones. Reframe `FromWorld` fallback logic in the "reflect clone" impl to use it. This cuts out a lot of unnecessary work and I think justifies the existence of a pseudo-command interface (given how niche, yet performance sensitive this is). Note that we no longer auto-impl entity mapping for ` IntoIterator<Item = &'a Entity>` types, as this would block our ability to implement cases like `HashMap`. This means the onus is on us (or type authors) to add explicit support for types that should be mappable. Also note that the Component-related changes do not require a migration guide as there hasn't been a release with them yet. ## Migration Guide If you were previously implementing `VisitEntities` or `VisitEntitiesMut` (likely via a derive), instead use `MapEntities`. Those were almost certainly used in the context of Bevy Scenes or reflection via `ReflectMapEntities`. If you have a case that uses `VisitEntities` or `VisitEntitiesMut` directly, where `MapEntities` is not a viable replacement, please let us know! ```rust // before #[derive(VisitEntities, VisitEntitiesMut)] struct Inventory { items: Vec<Entity>, #[visit_entities(ignore)] label: String, } // after #[derive(MapEntities)] struct Inventory { #[entities] items: Vec<Entity>, label: String, } ``` |
||
|
Alice Cecile
|
5d0505a85e
|
Unify and simplify command and system error handling (#18351)
# Objective - ECS error handling is a lovely flagship feature for Bevy 0.16, all in the name of reducing panics and encouraging better error handling (#14275). - Currently though, command and system error handling are completely disjoint and use different mechanisms. - Additionally, there's a number of distinct ways to set the default/fallback/global error handler that have limited value. As far as I can tell, this will be cfg flagged to toggle between dev and production builds in 99.9% of cases, with no real value in more granular settings or helpers. - Fixes #17272 ## Solution - Standardize error handling on the OnceLock global error mechanisms ironed out in https://github.com/bevyengine/bevy/pull/17215 - As discussed there, there are serious performance concerns there, especially for commands - I also think this is a better fit for the use cases, as it's truly global - Move from `SystemErrorContext` to a more general purpose `ErrorContext`, which can handle observers and commands more clearly - Cut the superfluous setter methods on `App` and `SubApp` - Rename the limited (and unhelpful) `fallible_systems` example to `error_handling`, and add an example of command error handling ## Testing Ran the `error_handling` example. ## Notes for reviewers - Do you see a clear way to allow commands to retain &mut World access in the per-command custom error handlers? IMO that's a key feature here (allowing the ad-hoc creation of custom commands), but I'm not sure how to get there without exploding complexity. - I've removed the feature gate on the default_error_handler: contrary to @cart's opinion in #17215 I think that virtually all apps will want to use this. Can you think of a category of app that a) is extremely performance sensitive b) is fine with shipping to production with the panic error handler? If so, I can try to gather performance numbers and/or reintroduce the feature flag. UPDATE: see benches at the end of this message. - ~~`OnceLock` is in `std`: @bushrat011899 what should we do here?~~ - Do you have ideas for more automated tests for this collection of features? ## Benchmarks I checked the impact of the feature flag introduced: benchmarks might show regressions. This bears more investigation. I'm still skeptical that there are users who are well-served by a fast always panicking approach, but I'm going to re-add the feature flag here to avoid stalling this out.  --------- Co-authored-by: Zachary Harrold <zac@harrold.com.au> |
||
|
Eagster
|
246ce590e5
|
Queued component registration (#18173)
# Objective This is an alternative to #17871 and #17701 for tracking issue #18155. This thanks to @maniwani for help with this design. The goal is to enable component ids to be reserved from multiple threads concurrently and with only `&World`. This contributes to assets as entities, read-only query and system parameter initialization, etc. ## What's wrong with #17871 ? In #17871, I used my proposed staging utilities to allow *fully* registering components from any thread concurrently with only `&Components`. However, if we want to pursue components as entities (which is desirable for a great many reasons. See [here](https://discord.com/channels/691052431525675048/692572690833473578/1346499196655505534) on discord), this staging isn't going to work. After all, if registering a component requires spawning an entity, and spawning an entity requires `&mut World`, it is impossible to register a component fully with only `&World`. ## Solution But what if we don't have to register it all the way? What if it's enough to just know the `ComponentId` it will have once it is registered and to queue it to be registered at a later time? Spoiler alert: That is all we need for these features. Here's the basic design: Queue a registration: 1. Check if it has already been registered. 2. Check if it has already been queued. 3. Reserve a `ComponentId`. 4. Queue the registration at that id. Direct (normal) registration: 1. Check if this registration has been queued. 2. If it has, use the queued registration instead. 3. Otherwise, proceed like normal. Appllying the queue: 1. Pop queued items off one by one. 2. Register them directly. One other change: The whole point of this design over #17871 is to facilitate coupling component registration with the World. To ensure that this would fully work with that, I went ahead and moved the `ComponentId` generator onto the world itself. That stemmed a couple of minor organizational changes (see migration guide). As we do components as entities, we will replace this generator with `Entities`, which lives on `World` too. Doing this move early let me verify the design and will reduce migration headaches in the future. If components as entities is as close as I think it is, I don't think splitting this up into different PRs is worth it. If it is not as close as it is, it might make sense to still do #17871 in the meantime (see the risks section). I'll leave it up to y'all what we end up doing though. ## Risks and Testing The biggest downside of this compared to #17871 is that now we have to deal with correct but invalid `ComponentId`s. They are invalid because the component still isn't registered, but they are correct because, once registered, the component will have exactly that id. However, the only time this becomes a problem is if some code violates safety rules by queuing a registration and using the returned id as if it was valid. As this is a new feature though, nothing in Bevy does this, so no new tests were added for it. When we do use it, I left detailed docs to help mitigate issues here, and we can test those usages. Ex: we will want some tests on using queries initialized from queued registrations. ## Migration Guide Component registration can now be queued with only `&World`. To facilitate this, a few APIs needed to be moved around. The following functions have moved from `Components` to `ComponentsRegistrator`: - `register_component` - `register_component_with_descriptor` - `register_resource_with_descriptor` - `register_non_send` - `register_resource` - `register_required_components_manual` Accordingly, functions in `Bundle` and `Component` now take `ComponentsRegistrator` instead of `Components`. You can obtain `ComponentsRegistrator` from the new `World::components_registrator`. You can obtain `ComponentsQueuedRegistrator` from the new `World::components_queue`, and use it to stage component registration if desired. # Open Question Can we verify that it is enough to queue registration with `&World`? I don't think it would be too difficult to package this up into a `Arc<MyComponentsManager>` type thing if we need to, but keeping this on `&World` certainly simplifies things. If we do need the `Arc`, we'll need to look into partitioning `Entities` for components as entities, so we can keep most of the allocation fast on `World` and only keep a smaller partition in the `Arc`. I'd love an SME on assets as entities to shed some light on this. --------- Co-authored-by: andriyDev <andriydzikh@gmail.com> |
||
|
Tim Overbeek
|
664000f848
|
Improve derive(Event) and simplify macro code (#18083)
# Objective simplify some code and improve Event macro Closes https://github.com/bevyengine/bevy/issues/14336, # Showcase you can now write derive Events like so ```rust #[derive(event)] #[event(auto_propagate, traversal = MyType)] struct MyEvent; ``` |
||
|
Carter Anderson
|
cca5813472
|
BevyError: Bevy's new catch-all error type (#18144)
## Objective Fixes #18092 Bevy's current error type is a simple type alias for `Box<dyn Error + Send + Sync + 'static>`. This largely works as a catch-all error, but it is missing a critical feature: the ability to capture a backtrace at the point that the error occurs. The best way to do this is `anyhow`-style error handling: a new error type that takes advantage of the fact that the `?` `From` conversion happens "inline" to capture the backtrace at the point of the error. ## Solution This PR adds a new `BevyError` type (replacing our old `std::error::Error` type alias), which uses the "from conversion backtrace capture" approach: ```rust fn oh_no() -> Result<(), BevyError> { // this fails with Rust's built in ParseIntError, which // is converted into the catch-all BevyError type let number: usize = "hi".parse()?; println!("parsed {number}"); Ok(()) } ``` This also updates our exported `Result` type alias to default to `BevyError`, meaning you can write this instead: ```rust fn oh_no() -> Result { let number: usize = "hi".parse()?; println!("parsed {number}"); Ok(()) } ``` When a BevyError is encountered in a system, it will use Bevy's default system error handler (which panics by default). BevyError does custom "backtrace filtering" by default, meaning we can cut out the _massive_ amount of "rust internals", "async executor internals", and "bevy system scheduler internals" that show up in backtraces. It also trims out the first generally-unnecssary `From` conversion backtrace lines that make it harder to locate the real error location. The result is a blissfully simple backtrace by default:  The full backtrace can be shown by setting the `BEVY_BACKTRACE=full` environment variable. Non-BevyError panics still use the default Rust backtrace behavior. One issue that prevented the truly noise-free backtrace during panics that you see above is that Rust's default panic handler will print the unfiltered (and largely unhelpful real-panic-point) backtrace by default, in _addition_ to our filtered BevyError backtrace (with the helpful backtrace origin) that we capture and print. To resolve this, I have extended Bevy's existing PanicHandlerPlugin to wrap the default panic handler. If we panic from the result of a BevyError, we will skip the default "print full backtrace" panic handler. This behavior can be enabled and disabled using the new `error_panic_hook` cargo feature in `bevy_app` (which is enabled by default). One downside to _not_ using `Box<dyn Error>` directly is that we can no longer take advantage of the built-in `Into` impl for strings to errors. To resolve this, I have added the following: ```rust // Before Err("some error")? // After Err(BevyError::message("some error"))? ``` We can discuss adding shorthand methods or macros for this (similar to anyhow's `anyhow!("some error")` macro), but I'd prefer to discuss that later. I have also added the following extension method: ```rust // Before some_option.ok_or("some error")?; // After some_option.ok_or_message("some error")?; ``` I've also moved all of our existing error infrastructure from `bevy_ecs::result` to `bevy_ecs::error`, as I think that is the better home for it ## Why not anyhow (or eyre)? The biggest reason is that `anyhow` needs to be a "generically useful error type", whereas Bevy is a much narrower scope. By using our own error, we can be significantly more opinionated. For example, anyhow doesn't do the extensive (and invasive) backtrace filtering that BevyError does because it can't operate on Bevy-specific context, and needs to be generically useful. Bevy also has a lot of operational context (ex: system info) that could be useful to attach to errors. If we have control over the error type, we can add whatever context we want to in a structured way. This could be increasingly useful as we add more visual / interactive error handling tools and editor integrations. Additionally, the core approach used is simple and requires almost no code. anyhow clocks in at ~2500 lines of code, but the impl here uses 160. We are able to boil this down to exactly what we need, and by doing so we improve our compile times and the understandability of our code. |
||
|
Carter Anderson
|
a530c07bc5
|
Preserve spawned RelationshipTarget order and other improvements (#17858)
Fixes #17720 ## Objective Spawning RelationshipTargets from scenes currently fails to preserve RelationshipTarget ordering (ex: `Children` has an arbitrary order). This is because it uses the normal hook flow to set up the collection, which means we are pushing onto the collection in _spawn order_ (which is currently in archetype order, which will often produce mismatched orderings). We need to preserve the ordering in the original RelationshipTarget collection. Ideally without expensive checking / fixups. ## Solution One solution would be to spawn in hierarchy-order. However this gets complicated as there can be multiple hierarchies, and it also means we can't spawn in more cache-friendly orders (ex: the current per-archetype spawning, or future even-smarter per-table spawning). Additionally, same-world cloning has _slightly_ more nuanced needs (ex: recursively clone linked relationships, while maintaining _original_ relationships outside of the tree via normal hooks). The preferred approach is to directly spawn the remapped RelationshipTarget collection, as this trivially preserves the ordering. Unfortunately we can't _just_ do that, as when we spawn the children with their Relationships (ex: `ChildOf`), that will insert a duplicate. We could "fixup" the collection retroactively by just removing the back half of duplicates, but this requires another pass / more lookups / allocating twice as much space. Additionally, it becomes complicated because observers could insert additional children, making it harder (aka more expensive) to determine which children are dupes and which are not. The path I chose is to support "opting out" of the relationship target hook in the contexts that need that, as this allows us to just cheaply clone the mapped collection. The relationship hook can look for this configuration when it runs and skip its logic when that happens. A "simple" / small-amount-of-code way to do this would be to add a "skip relationship spawn" flag to World. Sadly, any hook / observer that runs _as the result of an insert_ would also read this flag. We really need a way to scope this setting to a _specific_ insert. Therefore I opted to add a new `RelationshipInsertHookMode` enum and an `entity.insert_with_relationship_insert_hook_mode` variant. Obviously this is verbose and ugly. And nobody wants _more_ insert variants. But sadly this was the best I could come up with from a performance and capability perspective. If you have alternatives let me know! There are three variants: 1. `RelationshipInsertHookMode::Run`: always run relationship insert hooks (this is the default) 2. `RelationshipInsertHookMode::Skip`: do not run any relationship insert hooks for this insert (this is used by spawner code) 3. `RelationshipInsertHookMode::RunIfNotLinked`: only run hooks for _unlinked_ relationships (this is used in same-world recursive entity cloning to preserve relationships outside of the deep-cloned tree) Note that I have intentionally only added "insert with relationship hook mode" variants to the cases we absolutely need (everything else uses the default `Run` mode), just to keep the code size in check. I do not think we should add more without real _very necessary_ use cases. I also made some other minor tweaks: 1. I split out `SourceComponent` from `ComponentCloneCtx`. Reading the source component no longer needlessly blocks mutable access to `ComponentCloneCtx`. 2. Thanks to (1), I've removed the `RefCell` wrapper over the cloned component queue. 3. (1) also allowed me to write to the EntityMapper while queuing up clones, meaning we can reserve entities during the component clone and write them to the mapper _before_ inserting the component, meaning cloned collections can be mapped on insert. 4. I've removed the closure from `write_target_component_ptr` to simplify the API / make it compatible with the split `SourceComponent` approach. 5. I've renamed `EntityCloner::recursive` to `EntityCloner::linked_cloning` to connect that feature more directly with `RelationshipTarget::LINKED_SPAWN` 6. I've removed `EntityCloneBehavior::RelationshipTarget`. This was always intended to be temporary, and this new behavior removes the need for it. --------- Co-authored-by: Viktor Gustavsson <villor94@gmail.com> |
||
|
Zachary Harrold
|
20813aed64
|
Handle TriggerTargets that are combinations for components/entities (#18024)
# Objective * Fixes https://github.com/bevyengine/bevy/issues/14074 * Applies CI fixes for #16326 It is currently not possible to issues a trigger that targets a specific list of components AND a specific list of entities ## Solution We can now use `((A, B), (entity_1, entity_2))` as a trigger target, as well as the reverse ## Testing Added a unit test. The triggering rules for observers are quite confusing: Triggers once per entity target For each entity target, an observer system triggers if any of its components matches the trigger target components (but it triggers at most once, since we use an internal counter to make sure that an observer can run at most once per entity target) (copied from #14563) (copied from #16326) ## Notes All credit to @BenjaminBrienen and @cBournhonesque! Just applying a small fix to this PR so it can be merged. --------- Co-authored-by: Benjamin Brienen <Benjamin.Brienen@outlook.com> Co-authored-by: Christian Hughes <xdotdash@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Zachary Harrold
|
5241e09671
|
Upgrade to Rust Edition 2024 (#17967)
# Objective - Fixes #17960 ## Solution - Followed the [edition upgrade guide](https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-project-to-a-new-edition.html) ## Testing - CI --- ## Summary of Changes ### Documentation Indentation When using lists in documentation, proper indentation is now linted for. This means subsequent lines within the same list item must start at the same indentation level as the item. ```rust /* Valid */ /// - Item 1 /// Run-on sentence. /// - Item 2 struct Foo; /* Invalid */ /// - Item 1 /// Run-on sentence. /// - Item 2 struct Foo; ``` ### Implicit `!` to `()` Conversion `!` (the never return type, returned by `panic!`, etc.) no longer implicitly converts to `()`. This is particularly painful for systems with `todo!` or `panic!` statements, as they will no longer be functions returning `()` (or `Result<()>`), making them invalid systems for functions like `add_systems`. The ideal fix would be to accept functions returning `!` (or rather, _not_ returning), but this is blocked on the [stabilisation of the `!` type itself](https://doc.rust-lang.org/std/primitive.never.html), which is not done. The "simple" fix would be to add an explicit `-> ()` to system signatures (e.g., `|| { todo!() }` becomes `|| -> () { todo!() }`). However, this is _also_ banned, as there is an existing lint which (IMO, incorrectly) marks this as an unnecessary annotation. So, the "fix" (read: workaround) is to put these kinds of `|| -> ! { ... }` closuers into variables and give the variable an explicit type (e.g., `fn()`). ```rust // Valid let system: fn() = || todo!("Not implemented yet!"); app.add_systems(..., system); // Invalid app.add_systems(..., || todo!("Not implemented yet!")); ``` ### Temporary Variable Lifetimes The order in which temporary variables are dropped has changed. The simple fix here is _usually_ to just assign temporaries to a named variable before use. ### `gen` is a keyword We can no longer use the name `gen` as it is reserved for a future generator syntax. This involved replacing uses of the name `gen` with `r#gen` (the raw-identifier syntax). ### Formatting has changed Use statements have had the order of imports changed, causing a substantial +/-3,000 diff when applied. For now, I have opted-out of this change by amending `rustfmt.toml` ```toml style_edition = "2021" ``` This preserves the original formatting for now, reducing the size of this PR. It would be a simple followup to update this to 2024 and run `cargo fmt`. ### New `use<>` Opt-Out Syntax Lifetimes are now implicitly included in RPIT types. There was a handful of instances where it needed to be added to satisfy the borrow checker, but there may be more cases where it _should_ be added to avoid breakages in user code. ### `MyUnitStruct { .. }` is an invalid pattern Previously, you could match against unit structs (and unit enum variants) with a `{ .. }` destructuring. This is no longer valid. ### Pretty much every use of `ref` and `mut` are gone Pattern binding has changed to the point where these terms are largely unused now. They still serve a purpose, but it is far more niche now. ### `iter::repeat(...).take(...)` is bad New lint recommends using the more explicit `iter::repeat_n(..., ...)` instead. ## Migration Guide The lifetimes of functions using return-position impl-trait (RPIT) are likely _more_ conservative than they had been previously. If you encounter lifetime issues with such a function, please create an issue to investigate the addition of `+ use<...>`. ## Notes - Check the individual commits for a clearer breakdown for what _actually_ changed. --------- Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
Jean Mertz
|
7d8504f30e
|
feat(ecs): implement fallible observer systems (#17731)
This commit builds on top of the work done in #16589 and #17051, by adding support for fallible observer systems. As with the previous work, the actual results of the observer system are suppressed for now, but the intention is to provide a way to handle errors in a global way. Until then, you can use a `PipeSystem` to manually handle results. --------- Signed-off-by: Jean Mertz <git@jeanmertz.com> |
||
|
Chris Russell
|
eee7fd5b3e
|
Encapsulate cfg(feature = "track_location") in a type. (#17602)
# Objective Eliminate the need to write `cfg(feature = "track_location")` every time one uses an API that may use location tracking. It's verbose, and a little intimidating. And it requires code outside of `bevy_ecs` that wants to use location tracking needs to either unconditionally enable the feature, or include conditional compilation of its own. It would be good for users to be able to log locations when they are available without needing to add feature flags to their own crates. Reduce the number of cases where code compiles with the `track_location` feature enabled, but not with it disabled, or vice versa. It can be hard to remember to test it both ways! Remove the need to store a `None` in `HookContext` when the `track_location` feature is disabled. ## Solution Create an `MaybeLocation<T>` type that contains a `T` if the `track_location` feature is enabled, and is a ZST if it is not. The overall API is similar to `Option`, but whether the value is `Some` or `None` is set at compile time and is the same for all values. Default `T` to `&'static Location<'static>`, since that is the most common case. Remove all `cfg(feature = "track_location")` blocks outside of the implementation of that type, and instead call methods on it. When `track_location` is disabled, `MaybeLocation` is a ZST and all methods are `#[inline]` and empty, so they should be entirely removed by the compiler. But the code will still be visible to the compiler and checked, so if it compiles with the feature disabled then it should also compile with it enabled, and vice versa. ## Open Questions Where should these types live? I put them in `change_detection` because that's where the existing `MaybeLocation` types were, but we now use these outside of change detection. While I believe that the compiler should be able to remove all of these calls, I have not actually tested anything. If we want to take this approach, what testing is required to ensure it doesn't impact performance? ## Migration Guide Methods like `Ref::changed_by()` that return a `&'static Location<'static>` will now be available even when the `track_location` feature is disabled, but they will return a new `MaybeLocation` type. `MaybeLocation` wraps a `&'static Location<'static>` when the feature is enabled, and is a ZST when the feature is disabled. Existing code that needs a `&Location` can call `into_option().unwrap()` to recover it. Many trait impls are forwarded, so if you only need `Display` then no changes will be necessary. If that code was conditionally compiled, you may instead want to use the methods on `MaybeLocation` to remove the need for conditional compilation. Code that constructs a `Ref`, `Mut`, `Res`, or `ResMut` will now need to provide location information unconditionally. If you are creating them from existing Bevy types, you can obtain a `MaybeLocation` from methods like `Table::get_changed_by_slice_for()` or `ComponentSparseSet::get_with_ticks`. Otherwise, you will need to store a `MaybeLocation` next to your data and use methods like `as_ref()` or `as_mut()` to obtain wrapped references. |
||
|
raldone01
|
1b7db895b7
|
Harden proc macro path resolution and add integration tests. (#17330)
This pr uses the `extern crate self as` trick to make proc macros behave the same way inside and outside bevy. # Objective - Removes noise introduced by `crate as` in the whole bevy repo. - Fixes #17004. - Hardens proc macro path resolution. ## TODO - [x] `BevyManifest` needs cleanup. - [x] Cleanup remaining `crate as`. - [x] Add proper integration tests to the ci. ## Notes - `cargo-manifest-proc-macros` is written by me and based/inspired by the old `BevyManifest` implementation and [`bkchr/proc-macro-crate`](https://github.com/bkchr/proc-macro-crate). - What do you think about the new integration test machinery I added to the `ci`? More and better integration tests can be added at a later stage. The goal of these integration tests is to simulate an actual separate crate that uses bevy. Ideally they would lightly touch all bevy crates. ## Testing - Needs RA test - Needs testing from other users - Others need to run at least `cargo run -p ci integration-test` and verify that they work. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Carter Anderson
|
3c8fae2390
|
Improved Entity Mapping and Cloning (#17687)
Fixes #17535 Bevy's approach to handling "entity mapping" during spawning and cloning needs some work. The addition of [Relations](https://github.com/bevyengine/bevy/pull/17398) both [introduced a new "duplicate entities" bug when spawning scenes in the scene system](#17535) and made the weaknesses of the current mapping system exceedingly clear: 1. Entity mapping requires _a ton_ of boilerplate (implement or derive VisitEntities and VisitEntitesMut, then register / reflect MapEntities). Knowing the incantation is challenging and if you forget to do it in part or in whole, spawning subtly breaks. 2. Entity mapping a spawned component in scenes incurs unnecessary overhead: look up ReflectMapEntities, create a _brand new temporary instance_ of the component using FromReflect, map the entities in that instance, and then apply that on top of the actual component using reflection. We can do much better. Additionally, while our new [Entity cloning system](https://github.com/bevyengine/bevy/pull/16132) is already pretty great, it has some areas we can make better: * It doesn't expose semantic info about the clone (ex: ignore or "clone empty"), meaning we can't key off of that in places where it would be useful, such as scene spawning. Rather than duplicating this info across contexts, I think it makes more sense to add that info to the clone system, especially given that we'd like to use cloning code in some of our spawning scenarios. * EntityCloner is currently built in a way that prioritizes a single entity clone * EntityCloner's recursive cloning is built to be done "inside out" in a parallel context (queue commands that each have a clone of EntityCloner). By making EntityCloner the orchestrator of the clone we can remove internal arcs, improve the clarity of the code, make EntityCloner mutable again, and simplify the builder code. * EntityCloner does not currently take into account entity mapping. This is necessary to do true "bullet proof" cloning, would allow us to unify the per-component scene spawning and cloning UX, and ultimately would allow us to use EntityCloner in place of raw reflection for scenes like `Scene(World)` (which would give us a nice performance boost: fewer archetype moves, less reflection overhead). ## Solution ### Improved Entity Mapping First, components now have first-class "entity visiting and mapping" behavior: ```rust #[derive(Component, Reflect)] #[reflect(Component)] struct Inventory { size: usize, #[entities] items: Vec<Entity>, } ``` Any field with the `#[entities]` annotation will be viewable and mappable when cloning and spawning scenes. Compare that to what was required before! ```rust #[derive(Component, Reflect, VisitEntities, VisitEntitiesMut)] #[reflect(Component, MapEntities)] struct Inventory { #[visit_entities(ignore)] size: usize, items: Vec<Entity>, } ``` Additionally, for relationships `#[entities]` is implied, meaning this "just works" in scenes and cloning: ```rust #[derive(Component, Reflect)] #[relationship(relationship_target = Children)] #[reflect(Component)] struct ChildOf(pub Entity); ``` Note that Component _does not_ implement `VisitEntities` directly. Instead, it has `Component::visit_entities` and `Component::visit_entities_mut` methods. This is for a few reasons: 1. We cannot implement `VisitEntities for C: Component` because that would conflict with our impl of VisitEntities for anything that implements `IntoIterator<Item=Entity>`. Preserving that impl is more important from a UX perspective. 2. We should not implement `Component: VisitEntities` VisitEntities in the Component derive, as that would increase the burden of manual Component trait implementors. 3. Making VisitEntitiesMut directly callable for components would make it easy to invalidate invariants defined by a component author. By putting it in the `Component` impl, we can make it harder to call naturally / unavailable to autocomplete using `fn visit_entities_mut(this: &mut Self, ...)`. `ReflectComponent::apply_or_insert` is now `ReflectComponent::apply_or_insert_mapped`. By moving mapping inside this impl, we remove the need to go through the reflection system to do entity mapping, meaning we no longer need to create a clone of the target component, map the entities in that component, and patch those values on top. This will make spawning mapped entities _much_ faster (The default `Component::visit_entities_mut` impl is an inlined empty function, so it will incur no overhead for unmapped entities). ### The Bug Fix To solve #17535, spawning code now skips entities with the new `ComponentCloneBehavior::Ignore` and `ComponentCloneBehavior::RelationshipTarget` variants (note RelationshipTarget is a temporary "workaround" variant that allows scenes to skip these components. This is a temporary workaround that can be removed as these cases should _really_ be using EntityCloner logic, which should be done in a followup PR. When that is done, `ComponentCloneBehavior::RelationshipTarget` can be merged into the normal `ComponentCloneBehavior::Custom`). ### Improved Cloning * `Option<ComponentCloneHandler>` has been replaced by `ComponentCloneBehavior`, which encodes additional intent and context (ex: `Default`, `Ignore`, `Custom`, `RelationshipTarget` (this last one is temporary)). * Global per-world entity cloning configuration has been removed. This felt overly complicated, increased our API surface, and felt too generic. Each clone context can have different requirements (ex: what a user wants in a specific system, what a scene spawner wants, etc). I'd prefer to see how far context-specific EntityCloners get us first. * EntityCloner's internals have been reworked to remove Arcs and make it mutable. * EntityCloner is now directly stored on EntityClonerBuilder, simplifying the code somewhat * EntityCloner's "bundle scratch" pattern has been moved into the new BundleScratch type, improving its usability and making it usable in other contexts (such as future cross-world cloning code). Currently this is still private, but with some higher level safe APIs it could be used externally for making dynamic bundles * EntityCloner's recursive cloning behavior has been "externalized". It is now responsible for orchestrating recursive clones, meaning it no longer needs to be sharable/clone-able across threads / read-only. * EntityCloner now does entity mapping during clones, like scenes do. This gives behavior parity and also makes it more generically useful. * `RelatonshipTarget::RECURSIVE_SPAWN` is now `RelationshipTarget::LINKED_SPAWN`, and this field is used when cloning relationship targets to determine if cloning should happen recursively. The new `LINKED_SPAWN` term was picked to make it more generically applicable across spawning and cloning scenarios. ## Next Steps * I think we should adapt EntityCloner to support cross world cloning. I think this PR helps set the stage for that by making the internals slightly more generalized. We could have a CrossWorldEntityCloner that reuses a lot of this infrastructure. * Once we support cross world cloning, we should use EntityCloner to spawn `Scene(World)` scenes. This would yield significant performance benefits (no archetype moves, less reflection overhead). --------- Co-authored-by: eugineerd <70062110+eugineerd@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
ElliottjPierce
|
1b2cf7d6cd
|
Isolate component registration (#17671)
# Objective Progresses #17569. The end goal here is to synchronize component registration. See the other PR for details for the motivation behind that. For this PR specifically, the objective is to decouple `Components` from `Storages`. What components are registered etc should have nothing to do with what Storages looks like. Storages should only care about what entity archetypes have been spawned. ## Solution Previously, this was used to create sparse sets for relevant components when those components were registered. Now, we do that when the component is inserted/spawned. This PR proposes doing that in `BundleInfo::new`, but there may be a better place. ## Testing In theory, this shouldn't have changed any functionality, so no new tests were created. I'm not aware of any examples that make heavy use of sparse set components either. ## Migration Guide - Remove storages from functions where it is no longer needed. - Note that SparseSets are no longer present for all registered sparse set components, only those that have been spawned. --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> |
||
|
Zachary Harrold
|
d0c0bad7b4
|
Split Component::register_component_hooks into individual methods (#17685)
# Objective - Fixes #17411 ## Solution - Deprecated `Component::register_component_hooks` - Added individual methods for each hook which return `None` if the hook is unused. ## Testing - CI --- ## Migration Guide `Component::register_component_hooks` is now deprecated and will be removed in a future release. When implementing `Component` manually, also implement the respective hook methods on `Component`. ```rust // Before impl Component for Foo { // snip fn register_component_hooks(hooks: &mut ComponentHooks) { hooks.on_add(foo_on_add); } } // After impl Component for Foo { // snip fn on_add() -> Option<ComponentHook> { Some(foo_on_add) } } ``` ## Notes I've chosen to deprecate `Component::register_component_hooks` rather than outright remove it to ease the migration guide. While it is in a state of deprecation, it must be used by `Components::register_component_internal` to ensure users who haven't migrated to the new hook definition scheme aren't left behind. For users of the new scheme, a default implementation of `Component::register_component_hooks` is provided which forwards the new individual hook implementations. Personally, I think this is a cleaner API to work with, and would allow the documentation for hooks to exist on the respective `Component` methods (e.g., documentation for `OnAdd` can exist on `Component::on_add`). Ideally, `Component::on_add` would be the hook itself rather than a getter for the hook, but it is the only way to early-out for a no-op hook, which is important for performance. ## Migration Guide `Component::register_component_hooks` has been deprecated. If you are manually implementing the `Component` trait and registering hooks there, use the individual methods such as `on_add` instead for increased clarity. |
||
|
Periwink
|
75e8e8c0f6
|
Expose ObserverDescriptor fields (#17623)
# Objective Expose accessor functions to the `ObserverDescriptor`, so that users can use the `Observer` component to inspect what the observer is watching. This would be useful for me, I don't think there's any reason to hide these. |
||
|
Zachary Harrold
|
9bc0ae33c3
|
Move hashbrown and foldhash out of bevy_utils (#17460)
# Objective - Contributes to #16877 ## Solution - Moved `hashbrown`, `foldhash`, and related types out of `bevy_utils` and into `bevy_platform_support` - Refactored the above to match the layout of these types in `std`. - Updated crates as required. ## Testing - CI --- ## Migration Guide - The following items were moved out of `bevy_utils` and into `bevy_platform_support::hash`: - `FixedState` - `DefaultHasher` - `RandomState` - `FixedHasher` - `Hashed` - `PassHash` - `PassHasher` - `NoOpHash` - The following items were moved out of `bevy_utils` and into `bevy_platform_support::collections`: - `HashMap` - `HashSet` - `bevy_utils::hashbrown` has been removed. Instead, import from `bevy_platform_support::collections` _or_ take a dependency on `hashbrown` directly. - `bevy_utils::Entry` has been removed. Instead, import from `bevy_platform_support::collections::hash_map` or `bevy_platform_support::collections::hash_set` as appropriate. - All of the above equally apply to `bevy::utils` and `bevy::platform_support`. ## Notes - I left `PreHashMap`, `PreHashMapExt`, and `TypeIdMap` in `bevy_utils` as they might be candidates for micro-crating. They can always be moved into `bevy_platform_support` at a later date if desired. |
||
|
Zachary Harrold
|
41e79ae826
|
Refactored ComponentHook Parameters into HookContext (#17503)
# Objective
- Make the function signature for `ComponentHook` less verbose
## Solution
- Refactored `Entity`, `ComponentId`, and `Option<&Location>` into a new
`HookContext` struct.
## Testing
- CI
---
## Migration Guide
Update the function signatures for your component hooks to only take 2
arguments, `world` and `context`. Note that because `HookContext` is
plain data with all members public, you can use de-structuring to
simplify migration.
```rust
// Before
fn my_hook(
mut world: DeferredWorld,
entity: Entity,
component_id: ComponentId,
) { ... }
// After
fn my_hook(
mut world: DeferredWorld,
HookContext { entity, component_id, caller }: HookContext,
) { ... }
```
Likewise, if you were discarding certain parameters, you can use `..` in
the de-structuring:
```rust
// Before
fn my_hook(
mut world: DeferredWorld,
entity: Entity,
_: ComponentId,
) { ... }
// After
fn my_hook(
mut world: DeferredWorld,
HookContext { entity, .. }: HookContext,
) { ... }
```
|
||
|
SpecificProtagonist
|
f32a6fb205
|
Track callsite for observers & hooks (#15607)
# Objective Fixes #14708 Also fixes some commands not updating tracked location. ## Solution `ObserverTrigger` has a new `caller` field with the `track_change_detection` feature; hooks take an additional caller parameter (which is `Some(…)` or `None` depending on the feature). ## Testing See the new tests in `src/observer/mod.rs` --- ## Showcase Observers now know from where they were triggered (if `track_change_detection` is enabled): ```rust world.observe(move |trigger: Trigger<OnAdd, Foo>| { println!("Added Foo from {}", trigger.caller()); }); ``` ## Migration - hooks now take an additional `Option<&'static Location>` argument --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |