dependabot/cargo/taffy-0.8
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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
|
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.
|
||
|
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> |
||
|
Chris Russell
|
b4614dadcd
|
Use Display instead of Debug in the default error handler (#18629)
# Objective Improve error messages for missing resources. The default error handler currently prints the `Debug` representation of the error type instead of `Display`. Most error types use `#[derive(Debug)]`, resulting in a dump of the structure, but will have a user-friendly message for `Display`. Follow-up to #18593 ## Solution Change the default error handler to use `Display` instead of `Debug`. Change `BevyError` to include the backtrace in the `Display` format in addition to `Debug` so that it is still included. ## Showcase Before: ``` Encountered an error in system `system_name`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res<app_name::ResourceType>" } Encountered an error in system `other_system_name`: "String message with\nmultiple lines." ``` After ``` Encountered an error in system `system_name`: Parameter `Res<ResourceType>` failed validation: Resource does not exist Encountered an error in system `other_system_name`: String message with multiple lines. ``` |
||
|
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> |