# Objective
Allow combinator and pipe systems to delay validation of the second
system, while still allowing the second system to be skipped.
Fixes#18796
Allow fallible systems to be used as one-shot systems, reporting errors
to the error handler when used through commands.
Fixes#19722
Allow fallible systems to be used as run conditions, including when used
with combinators. Alternative to #19580.
Always validate parameters when calling the safe
`run_without_applying_deferred`, `run`, and `run_readonly` methods on a
`System`.
## Solution
Have `System::run_unsafe` return a `Result`.
We want pipe systems to run the first system before validating the
second, since the first system may affect whether the second system has
valid parameters. But if the second system skips then we have no output
value to return! So, pipe systems must return a `Result` that indicates
whether the second system ran.
But if we just make pipe systems have `Out = Result<B::Out>`, then
chaining `a.pipe(b).pipe(c)` becomes difficult. `c` would need to accept
the `Result` from `a.pipe(b)`, which means it would likely need to
return `Result` itself, giving `Result<Result<Out>>`!
Instead, we make *all* systems return a `Result`! We move the handling
of fallible systems from `IntoScheduleConfigs` and `IntoObserverSystem`
to `SystemParamFunction` and `ExclusiveSystemParamFunction`, so that an
infallible system can be wrapped before being passed to a combinator.
As a side effect, this enables fallible systems to be used as run
conditions and one-shot systems.
Now that the safe `run_without_applying_deferred`, `run`, and
`run_readonly` methods return a `Result`, we can have them perform
parameter validation themselves instead of requiring each caller to
remember to call them. `run_unsafe` will continue to not validate
parameters, since it is used in the multi-threaded executor when we want
to validate and run in separate tasks.
Note that this makes type inference a little more brittle. A function
that returns `Result<T>` can be considered either a fallible system
returning `T` or an infallible system returning `Result<T>` (and this is
important to continue supporting `pipe`-based error handling)! So there
are some cases where the output type of a system can no longer be
inferred. It will work fine when directly adding to a schedule, since
then the output type is fixed to `()` (or `bool` for run conditions).
And it will work fine when `pipe`ing to a system with a typed input
parameter.
I used a dedicated `RunSystemError` for the error type instead of plain
`BevyError` so that skipping a system does not box an error or capture a
backtrace.
# Objective
- First step towards #279
## Solution
Makes the necessary internal data structure changes in order to allow
system removal to be added in a future PR: `Vec`s storing systems and
system sets in `ScheduleGraph` have been replaced with `SlotMap`s.
See the included migration guide for the required changes.
## Testing
Internal changes only and no new features *should* mean no new tests are
requried.
# Objective
- This unblocks some work I am doing for #19887.
## Solution
- Rename `RenderGraphApp` to `RenderGraphExt`.
- Implement `RenderGraphExt` for `World`.
- Change `SubApp` and `App` to call the `World` impl.
# Objective
- Progress towards #19024.
## Solution
- Remove `Handle::Weak`!
If users were relying on `Handle::Weak` for some purpose, they can
almost certainly replace it with raw `AssetId` instead. If they cannot,
they can make their own enum that holds either a Handle or an AssetId.
In either case, we don't need weak handles!
Sadly we still need Uuid handles since we rely on them for "default"
assets and "invalid" assets, as well as anywhere where a component wants
to impl default with a non-defaulted asset handle. One step at a time
though!
# Objective
During the migration to required components a lot of things were changed
around and somehow the draw order for some UI elements ended up
depending on the system ordering in `RenderSystems::Queue`, which can
sometimes result in the elements being drawn in the wrong order.
Fixes#19674
## Solution
* Added some more `stack_z_offsets` constants and used them to enforce
an explicit ordering.
* Removed the `stack_index: u32` field from `ExtractedUiNodes` and
replaced it with a `z_order: f32` field.
These changes should fix all the ordering problems.
## Testing
I added a nine-patched bordered node with a navy background color to the
slice section of the `testbed_ui` example.
The border should always be drawn above the background color.
# Objective
Change `ScrollPosition` to newtype `Vec2`. It's easier to work with a
`Vec2` wrapper than individual fields.
I'm not sure why this wasn't newtyped to start with. Maybe the intent
was to support responsive coordinates eventually but that probably isn't
very useful or straightforward to implement. And even if we do want to
support responsive coords in the future, it can newtype `Val2`.
## Solution
Change `ScrollPosition` to newtype `Vec2`.
Also added some extra details to the doc comments.
## Testing
Try the `scroll` example.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
Because we want to be able to support more notification options in the
future (in addition to just using registered one-shot systems), the
`Option<SystemId>` notifications have been changed to a new enum,
`Callback`.
@alice-i-cecile
Currently, our specialization API works through a series of wrapper
structs and traits, which make things confusing to follow and difficult
to generalize.
This pr takes a different approach, where "specializers" (types that
implement `Specialize`) are composable, but "flat" rather than composed
of a series of wrappers. The key is that specializers don't *produce*
pipeline descriptors, but instead *modify* existing ones:
```rs
pub trait Specialize<T: Specializable> {
type Key: SpecializeKey;
fn specialize(
&self,
key: Self::Key,
descriptor: &mut T::Descriptor
) -> Result<Canonical<Self::Key>, BevyError>;
}
```
This lets us use some derive magic to stick multiple specializers
together:
```rs
pub struct A;
pub struct B;
impl Specialize<RenderPipeline> for A { ... }
impl Specialize<RenderPipeline> for A { ... }
#[derive(Specialize)]
#[specialize(RenderPipeline)]
struct C {
// specialization is applied in struct field order
applied_first: A,
applied_second: B,
}
type C::Key = (A::Key, B::Key);
```
This approach is much easier to understand, IMO, and also lets us
separate concerns better. Specializers can be placed in fully separate
crates/modules, and key computation can be shared as well.
The only real breaking change here is that since specializers only
modify descriptors, we need a "base" descriptor to work off of. This can
either be manually supplied when constructing a `Specializer` (the new
collection replacing `Specialized[Render/Compute]Pipelines`), or
supplied by implementing `HasBaseDescriptor` on a specializer. See
`examples/shader/custom_phase_item.rs` for an example implementation.
## Testing
- Did some simple manual testing of the derive macro, it seems robust.
---
## Showcase
```rs
#[derive(Specialize, HasBaseDescriptor)]
#[specialize(RenderPipeline)]
pub struct SpecializeMeshMaterial<M: Material> {
// set mesh bind group layout and shader defs
mesh: SpecializeMesh,
// set view bind group layout and shader defs
view: SpecializeView,
// since type SpecializeMaterial::Key = (),
// we can hide it from the wrapper's external API
#[key(default)]
// defer to the GetBaseDescriptor impl of SpecializeMaterial,
// since it carries the vertex and fragment handles
#[base_descriptor]
// set material bind group layout, etc
material: SpecializeMaterial<M>,
}
// implementation generated by the derive macro
impl <M: Material> Specialize<RenderPipeline> for SpecializeMeshMaterial<M> {
type Key = (MeshKey, ViewKey);
fn specialize(
&self,
key: Self::Key,
descriptor: &mut RenderPipelineDescriptor
) -> Result<Canonical<Self::Key>, BevyError> {
let mesh_key = self.mesh.specialize(key.0, descriptor)?;
let view_key = self.view.specialize(key.1, descriptor)?;
let _ = self.material.specialize((), descriptor)?;
Ok((mesh_key, view_key));
}
}
impl <M: Material> HasBaseDescriptor<RenderPipeline> for SpecializeMeshMaterial<M> {
fn base_descriptor(&self) -> RenderPipelineDescriptor {
self.material.base_descriptor()
}
}
```
---------
Co-authored-by: Tim Overbeek <158390905+Bleachfuel@users.noreply.github.com>
# Objective
- A step towards #19024.
- `AnimationGraph` can serialize raw `AssetId`s. However for normal
handles, this is a runtime ID. This means it is unlikely that the
`AssetId` will correspond to the same asset after deserializing -
effectively breaking the graph.
## Solution
- Stop allowing `AssetId` to be serialized by `AnimationGraph`.
Serializing a handle with no path is now an error.
- Add `MigrationSerializedAnimationClip`. This is an untagged enum for
serde, meaning that it will take the first variant that deserializes. So
it will first try the "modern" version, then it will fallback to the
legacy version.
- Add some logging/error messages to explain what users should do.
Note: one limitation here is that this removes the ability to serialize
and deserialize UUIDs. In theory, someone could be using this to have a
"default" animation. If someone inserts an empty `AnimationClip` into
the `Handle::default()`, this **might** produce a T-pose. It might also
do nothing though. Unclear! I think this is worth the risk for
simplicity as it seems unlikely that people are sticking UUIDs in here
(or that you want a default animation in **any** AnimationGraph).
## Testing
- Ran `cargo r --example animation_graph -- --save` on main, then ran
`cargo r --example animation_graph` on this PR. The PR was able to load
the old data (after #19631).
# Objective
It's odd that `TextShadow` is accessible by importing `bevy::ui::*` but
`Text` isn't.
Move the `TextShadow` component to `text` widget module and move its
type registration to the `build_text_interop` function.
# Objective
This PR introduces Bevy Feathers, an opinionated widget toolkit and
theming system intended for use by the Bevy Editor, World Inspector, and
other tools.
The `bevy_feathers` crate is incomplete and hidden behind an
experimental feature flag. The API is going to change significantly
before release.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
*Step towards https://github.com/bevyengine/bevy/issues/19686*
We now have all the infrastructure in place to migrate Bevy's default
behavior when loading glTF files to respect their coordinate system.
Let's start migrating! For motivation, see the issue linked above
## Solution
- Introduce a feature flag called `gltf_convert_coordinates_default`
- Currently,`GltfPlugin::convert_coordinates` defaults to `false`
- If `gltf_convert_coordinates_default` is enabled,
`GltfPlugin::convert_coordinates` will default to `true`
- If `gltf_convert_coordinates_default` is not enabled *and*
`GltfPlugin::convert_coordinates` is false, we assume the user is
implicitly using the old behavior. Print a warning *once* in that case,
but only when a glTF was actually loaded
- A user can opt into the new behavior either
- Globally, by enabling `gltf_convert_coordinates_default` in their
`Cargo.toml`
- Globally, by enabling `GltfPlugin::convert_coordinates`
- Per asset, by enabling `GltfLoaderSettings::convert_coordinates`
- A user can explicitly opt out of the new behavior and silence the
warning by
- Enabling `gltf_convert_coordinates_default` in their `Cargo.toml` and
disabling `GltfPlugin::convert_coordinates`
- This PR also moves the existing release note into a migration guide
Note that I'm very open to change any features, mechanisms, warning
texts, etc. as needed :)
## Future Work
- This PR leaves all examples fully functional by not enabling this flag
internally yet. A followup PR will enable it as a `dev-dependency` and
migrate all of our examples involving glTFs to the new behavior.
- After 0.17 (and the RC before) lands, we'll gather feedback to see if
anything breaks or the suggested migration is inconvenient in some way
- If all goes well, we'll kill this flag and change the default of
`GltfPlugin::convert_coordinates` to `true` in 0.18
## Testing
- Ran examples with and without the flag
---------
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Co-authored-by: AlephCubed <76791009+AlephCubed@users.noreply.github.com>
# Objective
- add support for alternate zstd backend through `zstd` for faster
decompression
## Solution
- make existing `zstd` feature only specify that support is required,
disambiguate which backend to use via two other features `zstd_native`
and `zstd_rust`.
- Similar to the approach taken by #18411, but we keep current behavior
by defaulting to the rust implementation because its safer, and isolate
this change.
NOTE: the default feature-set may seem to not currently require `zstd`,
but it does, it is enabled transitively by the `tonemapping_luts`
feature, which is a default feature. Thus this does not add default
features.
## Testing
- Cargo clippy on both feature combinations
# Objective
Upgrade to `wgpu` version `25.0`.
Depends on https://github.com/bevyengine/naga_oil/pull/121
## Solution
### Problem
The biggest issue we face upgrading is the following requirement:
> To facilitate this change, there was an additional validation rule put
in place: if there is a binding array in a bind group, you may not use
dynamic offset buffers or uniform buffers in that bind group. This
requirement comes from vulkan rules on UpdateAfterBind descriptors.
This is a major difficulty for us, as there are a number of binding
arrays that are used in the view bind group. Note, this requirement does
not affect merely uniform buffors that use dynamic offset but the use of
*any* uniform in a bind group that also has a binding array.
### Attempted fixes
The easiest fix would be to change uniforms to be storage buffers
whenever binding arrays are in use:
```wgsl
#ifdef BINDING_ARRAYS_ARE_USED
@group(0) @binding(0) var<uniform> view: View;
@group(0) @binding(1) var<uniform> lights: types::Lights;
#else
@group(0) @binding(0) var<storage> view: array<View>;
@group(0) @binding(1) var<storage> lights: array<types::Lights>;
#endif
```
This requires passing the view index to the shader so that we know where
to index into the buffer:
```wgsl
struct PushConstants {
view_index: u32,
}
var<push_constant> push_constants: PushConstants;
```
Using push constants is no problem because binding arrays are only
usable on native anyway.
However, this greatly complicates the ability to access `view` in
shaders. For example:
```wgsl
#ifdef BINDING_ARRAYS_ARE_USED
mesh_view_bindings::view.view_from_world[0].z
#else
mesh_view_bindings::view[mesh_view_bindings::view_index].view_from_world[0].z
#endif
```
Using this approach would work but would have the effect of polluting
our shaders with ifdef spam basically *everywhere*.
Why not use a function? Unfortunately, the following is not valid wgsl
as it returns a binding directly from a function in the uniform path.
```wgsl
fn get_view() -> View {
#if BINDING_ARRAYS_ARE_USED
let view_index = push_constants.view_index;
let view = views[view_index];
#endif
return view;
}
```
This also poses problems for things like lights where we want to return
a ptr to the light data. Returning ptrs from wgsl functions isn't
allowed even if both bindings were buffers.
The next attempt was to simply use indexed buffers everywhere, in both
the binding array and non binding array path. This would be viable if
push constants were available everywhere to pass the view index, but
unfortunately they are not available on webgpu. This means either
passing the view index in a storage buffer (not ideal for such a small
amount of state) or using push constants sometimes and uniform buffers
only on webgpu. However, this kind of conditional layout infects
absolutely everything.
Even if we were to accept just using storage buffer for the view index,
there's also the additional problem that some dynamic offsets aren't
actually per-view but per-use of a setting on a camera, which would
require passing that uniform data on *every* camera regardless of
whether that rendering feature is being used, which is also gross.
As such, although it's gross, the simplest solution just to bump binding
arrays into `@group(1)` and all other bindings up one bind group. This
should still bring us under the device limit of 4 for most users.
### Next steps / looking towards the future
I'd like to avoid needing split our view bind group into multiple parts.
In the future, if `wgpu` were to add `@builtin(draw_index)`, we could
build a list of draw state in gpu processing and avoid the need for any
kind of state change at all (see
https://github.com/gfx-rs/wgpu/issues/6823). This would also provide
significantly more flexibility to handle things like offsets into other
arrays that may not be per-view.
### Testing
Tested a number of examples, there are probably more that are still
broken.
---------
Co-authored-by: François Mockers <mockersf@gmail.com>
Co-authored-by: Elabajaba <Elabajaba@users.noreply.github.com>
# Objective
- Fixes https://github.com/bevyengine/bevy/issues/14328
- `DynamicMap::drain` was broken (indices weren't cleared, causing a
panic when reading later)
- `PartialReflect::apply` was broken for maps and sets, because they
don't remove entries from the `self` map that aren't in the applied map.
- I discovered this bug when implementing MapEntities on a Component
containing a `HashMap<Entity, _>`. Because `apply` is used to reapply
the changes to the reflected map, the map ended up littered with a ton
of outdated entries.
## Solution
- Remove the separate `Vec` in `DynamicMap` and use the `HashTable`
directly, like it is in `DynamicSet`.
- Replace `MapIter` by `Box<dyn Iterator>` (like for `DynamicSet`), and
`Map::get_at` and `Map::get_at_mut` which are now unused.
- Now assume `DynamicMap` types are unordered and adjust documentation
accordingly.
- Fix documentation of `DynamicSet` (ordered -> unordered)
- Added `Map::retain` and `Set::retain`, and use them to remove excess
entries in `PartialReflect::apply` implementations.
## Testing
- Added `map::tests::apply` and `set::tests::apply` to validate
`<DynamicMap as PartialReflect>::apply` and `<DynamicSet as
PartialReflect>::apply`
# Objective
- Followup to https://github.com/bevyengine/bevy/pull/19633
- As discussed, it's a bit cumbersome to specify that you want the
correct orientation every single time
- Also, glTFs loaded from third parties will still be loaded incorrectly
## Solution
- Allow opting into the new behavior globally or per-asset
- Also improved some docs while on it :)
## Testing
- Ran the animation examples
- Ran the test scene from the last PR with all configuration
combinations
# 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>
# Objective
- Related to #19024.
## Solution
- Remove the `FULLSCREEN_SHADER_HANDLE` `weak_handle` with a resource
holding the shader handle.
- This also changes us from using `load_internal_asset` to
`embedded_asset`/`load_embedded_asset`.
- All uses have been migrated to clone the `FullscreenShader` resource
and use its `to_vertex_state` method.
## Testing
- `anti_aliasing` example still works.
- `bloom_3d` example still works.
---------
Co-authored-by: charlotte 🌸 <charlotte.c.mcelwain@gmail.com>
# Objective
An attempt to start building a base for first-party tilemaps (#13782).
The objective is to create a very simple tilemap chunk rendering plugin
that can be used as a building block for 3rd-party tilemap crates, and
eventually a first-party tilemap implementation.
## Solution
- Introduces two user-facing components, `TilemapChunk` and
`TilemapChunkIndices`, and a new material `TilemapChunkMaterial`.
- `TilemapChunk` holds the chunk and tile sizes, and the tileset image
- The tileset image is expected to be a layered image for use with
`texture_2d_array`, with the assumption that atlases or multiple images
would go through an asset loader/processor. Not sure if that should be
part of this PR or not..
- `TilemapChunkIndices` holds a 1d representation of all of the tile's
Option<u32> index into the tileset image.
- Indices are fixed to the size of tiles in a chunk (though maybe this
should just be an assertion instead?)
- Indices are cloned and sent to the shader through a u32 texture.
## Testing
- Initial testing done with the `tilemap_chunk` example, though I need
to include some way to update indices as part of it.
- Tested wasm with webgl2 and webgpu
- I'm thinking it would probably be good to do some basic perf testing.
---
## Showcase
```rust
let chunk_size = UVec2::splat(64);
let tile_size = UVec2::splat(16);
let indices: Vec<Option<u32>> = (0..chunk_size.x * chunk_size.y)
.map(|_| rng.gen_range(0..5))
.map(|i| if i == 0 { None } else { Some(i - 1) })
.collect();
commands.spawn((
TilemapChunk {
chunk_size,
tile_size,
tileset,
},
TilemapChunkIndices(indices),
));
```
# Objective
- Notice a word duplication typo
- Small quest to fix similar or nearby typos with my faithful companion
`\b(\w+)\s+\1\b`
## Solution
Fix em
# Objective
Some methods and commands carelessly overwrite `Relationship`
components. This may overwrite additional data stored at them which is
undesired.
Part of #19589
## Solution
A new private method will be used instead of insert:
`modify_or_insert_relation_with_relationship_hook_mode`.
This method behaves different to `insert` if `Relationship` is a larger
type than `Entity` and already contains this component. It will then use
the `modify_component` API and a new `Relationship::set_risky` method to
set the related entity, keeping all other data untouched.
For the `replace_related`(`_with_difference`) methods this also required
a `InsertHookMode` parameter for efficient modifications of multiple
children. The changes here are limited to the non-public methods.
I would appreciate feedback if this is all good.
# Testing
Added tests of all methods that previously could reset `Relationship`
data.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
Add support for interpolation in OKLab and OKLCH color spaces for UI
gradients.
## Solution
* New `InterpolationColorSpace` enum with `OkLab`, `OkLch`, `OkLchLong`,
`Srgb` and `LinearRgb` variants.
* Added a color space specialization to the gradients pipeline.
* Added support for interpolation in OkLCH and OkLAB color spaces to the
gradients shader. OKLCH interpolation supports both short and long hue
paths. This is mostly based on the conversion functions from
`bevy_color` except that interpolation in polar space uses radians.
* Added `color_space` fields to each gradient type.
## Testing
The `gradients` example has been updated to demonstrate the different
color interpolation methods.
Press space to cycle through the different options.
---
## Showcase
# Objective
This is part of the "core widgets" effort:
https://github.com/bevyengine/bevy/issues/19236.
## Solution
This adds the "core checkbox" widget type.
## Testing
Tested using examples core_widgets and core_widgets_observers.
Note to reviewers: I reorganized the code in the examples, so the diffs
are large because of code moves.
# Objective
Unblock #18162.
#15396 added the `'s` lifetime to `QueryData::Item` to make it possible
for query items to borrow from the state. The state isn't passed
directly to `QueryData::fetch()`, so it also added the `'s` lifetime to
`WorldQuery::Fetch` so that we can pass the borrows through there.
Unfortunately, having `WorldQuery::Fetch` borrow from the state makes it
impossible to have owned state, because we store the state and the
`Fetch` in the same `struct` during iteration.
## Solution
Undo the change to add the `'s` lifetime to `WorldQuery::Fetch`.
Instead, add a `&'s Self::State` parameter to `QueryData::fetch()` and
`QueryFilter::filter_fetch()` so that borrows from the state can be
passed directly to query items.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Emerson Coskey <emerson@coskey.dev>
# Objective
While `KeyCode` is very often the correct way to interact with keyboard
input there are a bunch of cases where it isn't, notably most of the
symbols (e.g. plus, minus, different parentheses). Currently the only
way to get these is to read from `EventReader<KeyboardInput>`, but then
you'd have to redo the `ButtonInput` logic for pressed/released to e.g.
make zoom functionality that depends on plus/minus keys.
This has led to confusion previously, like
https://github.com/bevyengine/bevy/issues/3278
## Solution
Add a `ButtonInput<Key>` resource.
## Testing
Modified the `keyboard_input` example to test it.
## Open questions
I'm not 100% sure this is the right way forward, since it duplicates the
key processing logic and might make people use the shorter
`ButtonInput<Key>` even when it's not appropriate.
Another option is to add a new struct with both `Key` and `KeyCode`, and
use `ButtonInput` with that instead. That would make it more
explanatory, but that is a lot of churn.
The third alternative is to not do this because it's too niche.
I'll add more documentation and take it out of draft if we want to move
forward with it.
# 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>
# Objective
- Fixes#19627
- Tackles part of #19644
- Supersedes #19629
- `Window` has become a very very very big component
- As such, our change detection does not *really* work on it, as e.g.
moving the mouse will cause a change for the entire window
- We circumvented this with a cache
- But, some things *shouldn't* be cached as they can be changed from
outside the user's control, notably the cursor grab mode on web
- So, we need to disable the cache for that
- But because change detection is broken, that would result in the
cursor grab mode being set every frame the mouse is moved
- That is usually *not* what a dev wants, as it forces the cursor to be
locked even when the end-user is trying to free the cursor on the
browser
- the cache in this situation is invalid due to #8949
## Solution
- Split `Window` into multiple components, each with working change
detection
- Disable caching of the cursor grab mode
- This will only attempt to force the grab mode when the `CursorOptions`
were touched by the user, which is *much* rarer than simply moving the
mouse.
- If this PR is merged, I'll do the exact same for the other
constituents of `Window` as a follow-up
## Testing
- Ran all the changed examples
# Objective
- compute_matrix doesn't compute anything, it just puts an Affine3A into
a Mat4. the name is inaccurate
## Solution
- rename it to conform with to_isometry (which, ironically, does compute
a decomposition which is rather expensive)
## Testing
- Its a rename. If it compiles, its good to go
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
*Fixes #5670 as an opt-in for now*
glTF uses the following coordinate system:
- forward: Z
- up: Y
- right: -X
and Bevy uses:
- forward: -Z
- up: Y
- right: X
For the longest time, Bevy has simply ignored this distinction. That
caused issues when working across programs, as most software respects
the
glTF coordinate system when importing and exporting glTFs. Your scene
might have looked correct in Blender, Maya, TrenchBroom, etc. but
everything would be flipped when importing it into Bevy!
## Solution
Add an option to the glTF loader to perform coordinate conversion. Note
that this makes a distinction in the camera nodes, as glTF uses a
different coordinate system for them.
## Follow Ups
- Add global glTF loader settings, similar to the image loader, so that
users can make third-party crates also load their glTFs with corrected
coordinates
- Decide on a migration strategy to make this the future default
- Create an issue
- Get feedback from Patrick Walton and Cart (not pinging them here to
not spam them)
- Include this pic for reference of how Blender assumes -Y as forward:
## Testing
I ran all glTF animation examples with the new setting enabled to
validate that they look the same, just flipped.
Also got a nice test scene from Chris that includes a camera inside the
glTF. Thanks @ChristopherBiscardi!
Blender (-Y forward):
Bevy (-Z forward, but the model looks the wrong way):
Bevy with `convert_coordinates` enabled (-Z forward):
Validation that the axes are correct with F3D's glTF viewer (+Z
forward):
# 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:?}");
```
# 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>
# 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.
# Objective
- A step towards #19024.
- Allow `ReflectAsset` to work with any `AssetId` not just `Handle`.
- `ReflectAsset::ids()` returns an iterator of `AssetId`s, but then
there's no way to use these ids, since all the other APIs in
`ReflectAsset` require a handle (and we don't have a reflect way to get
the handle).
## Solution
- Replace the `UntypedHandle` argument in `ReflectAsset` methods with
`impl Into<UntypedAssetId>`.
- This matches the regular asset API.
- This allows `ReflectAsset::ids()` to be more useful.
## Testing
- None.
# Objective
This is part of the "core widgets" effort: #19236.
## Solution
This PR adds the "core slider" widget to the collection.
## Testing
Tested using examples `core_widgets` and `core_widgets_observers`.
---------
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Follow-up of #19274.
Make the `check_change_tick` methods, of which some are now public, take
`CheckChangeTicks` to make it obvious where this tick comes from, see
other PR.
This also affects the `System` trait, hence the many changed files.
---------
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective
Reduce memory usage by storing fewer copies of
`FilteredAccessSet<ComponentId>`.
Currently, the `System` trait exposes the `component_access_set` for the
system, which is used by the multi-threaded executor to determine which
systems can run concurrently. But because it is available on the trait,
it needs to be stored for *every* system, even ones that are not run by
the executor! In particular, it is never needed for observers, or for
the inner systems in a `PipeSystem` or `CombinatorSystem`.
## Solution
Instead of exposing the access from a method on `System`, return it from
`System::initialize`. Since it is still needed during scheduling, store
the access alongside the boxed system in the schedule.
That's not quite enough for systems built using `SystemParamBuilder`s,
though. Those calculate the access in `SystemParamBuilder::build`, which
happens earlier than `System::initialize`. To handle those, we separate
`SystemParam::init_state` into `init_state`, which creates the state
value, and `init_access`, which calculates the access. This lets
`System::initialize` call `init_access` on a state that was provided by
the builder.
An additional benefit of that separation is that it removes the need to
duplicate access checks between `SystemParamBuilder::build` and
`SystemParam::init_state`.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Bevy Solari
<img
src="https://github.com/user-attachments/assets/94061fc8-01cf-4208-b72a-8eecad610d76"
width="100" />
## Preface
- See release notes.
- Please talk to me in #rendering-dev on discord or open a github
discussion if you have questions about the long term plan, and keep
discussion in this PR limited to the contents of the PR :)
## Connections
- Works towards #639, #16408.
- Spawned https://github.com/bevyengine/bevy/issues/18993.
- Need to fix RT stuff in naga_oil first
https://github.com/bevyengine/naga_oil/pull/116.
## This PR
After nearly two years, I've revived the raytraced lighting effort I
first started in https://github.com/bevyengine/bevy/pull/10000.
Unlike that PR, which has realtime techniques, I've limited this PR to:
* `RaytracingScenePlugin` - BLAS and TLAS building, geometry and texture
binding, sampling functions.
* `PathtracingPlugin` - A non-realtime path tracer intended to serve as
a testbed and reference.
## What's implemented?
* BLAS building on mesh load
* Emissive lights
* Directional lights with soft shadows
* Diffuse (lambert, not Bevy's diffuse BRDF) and emissive materials
* A reference path tracer with:
* Antialiasing
* Direct light sampling (next event estimation) with 0/1 MIS weights
* Importance-sampled BRDF bounces
* Russian roulette
## What's _not_ implemented?
* Anything realtime, including a real-time denoiser
* Integration with Bevy's rasterized gbuffer
* Specular materials
* Non-opaque geometry
* Any sort of CPU or GPU optimizations
* BLAS compaction, proper bindless, and further RT APIs are things that
we need wgpu to add
* PointLights, SpotLights, or skyboxes / environment lighting
* Support for materials other than StandardMaterial (and only a subset
of properties are supported)
* Skinned/morphed or otherwise animating/deformed meshes
* Mipmaps
* Adaptive self-intersection ray bias
* A good way for developers to detect whether the user's GPU supports RT
or not, and fallback to baked lighting.
* Documentation and actual finalized APIs (literally everything is
subject to change)
## End-user Usage
* Have a GPU that supports RT with inline ray queries
* Add `SolariPlugin` to your app
* Ensure any `Mesh` asset you want to use for raytracing has
`enable_raytracing: true` (defaults to true), and that it uses the
standard uncompressed position/normal/uv_0/tangent vertex attribute set,
triangle list topology, and 32-bit indices.
* If you don't want to build a BLAS and use the mesh for RT, set
enable_raytracing to false.
* Add the `RaytracingMesh3d` component to your entity (separate from
`Mesh3d` or `MeshletMesh3d`).
## Testing
- Did you test these changes? If so, how?
- Ran the solari example.
- Are there any parts that need more testing?
- Other test scenes probably. Normal mapping would be good to test.
- How can other people (reviewers) test your changes? Is there anything
specific they need to know?
- See the solari.rs example for how to setup raytracing.
- If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?
- Windows 11, NVIDIA RTX 3080.
---------
Co-authored-by: atlv <email@atlasdostal.com>
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# 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.
# Objective
#19366 implemented core button widgets, which included the `Depressed`
state component.
`Depressed` was chosen instead of `Pressed` to avoid conflict with the
`Pointer<Pressed>` event, but it is problematic and awkward in many
ways:
- Using the word "depressed" for such a high-traffic type is not great
due to the obvious connection to "depressed" as in depression.
- "Depressed" is not what I would search for if I was looking for a
component like this, and I'm not aware of any other engine or UI
framework using the term.
- `Depressed` is not a very natural pair to the `Pointer<Pressed>`
event.
- It might be because I'm not a native English speaker, but I have very
rarely heard someone say "a button is depressed". Seeing it, my mind
initially goes from "depression??" to "oh, de-pressed, meaning released"
and definitely not "is pressed", even though that *is* also a valid
meaning for it.
A related problem is that the current `Pointer<Pressed>` and
`Pointer<Released>` event names use a different verb tense than all of
our other observer events such as `Pointer<Click>` or
`Pointer<DragStart>`. By fixing this and renaming `Pressed` (and
`Released`), we can then use `Pressed` instead of `Depressed` for the
state component.
Additionally, the `IsHovered` and `IsDirectlyHovered` components added
in #19366 use an inconsistent naming; the other similar components don't
use an `Is` prefix. It also makes query filters like `Has<IsHovered>`
and `With<IsHovered>` a bit more awkward.
This is partially related to Cart's [picking concept
proposal](https://gist.github.com/cart/756e48a149db2838028be600defbd24a?permalink_comment_id=5598154).
## Solution
- Rename `Pointer<Pressed>` to `Pointer<Press>`
- Rename `Pointer<Released>` to `Pointer<Release>`
- Rename `Depressed` to `Pressed`
- Rename `IsHovered` to `Hovered`
- Rename `IsDirectlyHovered` to `DirectlyHovered`
# Objective
Part of #19236
## Solution
Adds a new `bevy_core_widgets` crate containing headless widget
implementations. This PR adds a single `CoreButton` widget, more widgets
to be added later once this is approved.
## Testing
There's an example, ui/core_widgets.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
# Objective
As discussed in #19285, some of our names conflict. `Entry` in bevy_ecs
is one of those overly general names.
## Solution
Rename this type (and the related types) to `ComponentEntry`.
---------
Co-authored-by: urben1680 <55257931+urben1680@users.noreply.github.com>
# 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>
