mirror of
https://github.com/bevyengine/bevy.git
synced 2026-07-01 08:12:51 -04:00
create-pull-request/patch
33 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Kevin Chen
|
0e49f63973 |
Add a rectangular light to the 3d testbed Light scene (#24025)
# Objective - Regression test for broken Rect Lights ## Solution - Add a rectangular light to testbed/3d ## Testing - `cargo run --example testbed_3d` - I can see the new light (well, after #24024 is merged hehe) Before (w/o rect light) <img width="1271" height="747" alt="Screenshot 2026-04-28 at 11 28 16 PM" src="https://github.com/user-attachments/assets/aa6b6258-e80d-4a98-861c-bcad6e8db541" /> After (w/ rect light - it’s behind the cube) <img width="1271" height="750" alt="Screenshot 2026-04-28 at 11 25 33 PM" src="https://github.com/user-attachments/assets/877013f0-843a-43f2-9dbd-96f2fa37d8e2" /> |
||
|
andriyDev
|
61127f6d01 |
Replace all different load variants in AssetServer with a builder. (#23663)
# Objective
- In 0.18, we had 10 different functions that load assets (I'm not even
counting `load_folder`).
- In 0.19, we've even added `load_erased` - but it unfortunately doesn't
support all the features that the other variants support.
- We apparently needed `load_acquire_with_settings_override` which 1)
loads the asset, 2) uses the settings provided, 3) allows reading
unapproved asset paths, and 4) drops a guard once the load completes.
- That's fine if that's necessary. But we needed to create an explicit
variant for that.
- We need fewer load paths!
## Solution
- Create a builder.
- Store all these options dynamically instead of statically handling
each case.
- Have the caller choose a particular "kind" of load when they are
ready: `load`, `load_erased`, `load_untyped`, or `load_untyped_async`.
- I intentionally didn't provide a `load_async` or `load_erased_async`,
since those can be replicated using `load`/`load_erased` +
`AssetServer::wait_for_asset_id` to get the exact same effect.
I am also intentionally leaving `NestedLoader` untouched in this PR, but
a followup will duplicate this API for `NestedLoader`, which should make
it easier to understand.
Unlike the `NestedLoader` API, we aren't doing any type-state craziness,
so the docs are much more clear: users don't need to understand how
type-state stuff works, they just call the handful of methods on the
type. The "cost" here is we now need to be careful about including the
cross product of loads between static asset type, runtime asset type, or
dynamic asset type, crossed with deferred or async. In theory, if we
added more kinds on either side, we would need to expand this cross
product a lot. In practice though, it seems unlikely there will be any
more variants there. (maybe there could be a blocking variant? I don't
think this is a popular opinion though).
A big con here is some somewhat common calls are now more verbose.
Specifically, `asset_server.load_with_settings()` has become
`asset_server.load_builder().with_settings().load()`. I am not really
concerned about this though, since it really isn't that painful.
## Testing
- Tests all pass!
---
## Showcase
Now instead of:
```rust
asset_server.load_acquire_with_settings_override("some_path", |settings: &mut GltfLoaderSettings| { ... }, my_lock_guard);
```
You can instead do:
```rust
asset_server.load_builder()
.with_guard(my_lock_guard)
.with_settings(|settings: &mut GltfLoaderSettings| { ... })
.override_unapproved()
.load("some_path");
```
We also now cover more variants! For example, you can now load an asset
untyped with a guard, or with override_unapproved, etc.
---------
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
|
||
|
Kevin Reid
|
ec254ded7d |
Make Skybox’s image optional so that Skybox::default() is valid. (#23691)
# Objective - Partially addresses #23688. - Prevents use of `Skybox::default()` from causing errors. ## Solution `Skybox::default()` is problematic because it contains an `Image` that is not a valid skybox. ~~This change removes the `Default` implementation and instead provides a `new()` function which takes the image as a parameter (and also the brightness, which is practically required).~~ This change makes the `image` field optional so that the default `None` renders nothing. Things we could do instead of this: * Make `Skybox` not implement `Default`. I am informed this is a bad idea. * Create a default cubemap image for `default()` to use. ## Testing Ran the `skybox` and `irradiance_volumes` examples. |
||
|
Carter Anderson
|
535cf401cc |
Reframe old "scene" terminology as "world serialization" (#23630)
Part 2 of #23619 In **Bevy 0.19** we are landing a subset of Bevy's Next Generation Scene system (often known as BSN), which now lives in the `bevy_scene` / `bevy::scene` crate. However the old `bevy_scene` system still needs to stick around for a bit longer, as it provides some features that Bevy's Next Generation Scene system doesn't (yet!): 1. It is not _yet_ possible to write a World _to_ BSN, so the old system is still necessary for "round trip World serialization". 2. The GLTF scene loader has not yet been ported to BSN, so the old system is still necessary to spawn GLTF scenes in Bevy. For this reason, we have renamed the old `bevy_scene` crate to `bevy_world_serialization`. If you were referencing `bevy_scene::*` or `bevy::scene::*` types, rename those paths to `bevy_world_serialization::*` and `bevy::world_serialization::*` respectively. Additionally, to avoid confusion / conflicts with the new scene system, all "scene" terminology / types have been reframed as "world serialization": - `Scene` -> `WorldAsset` (as this was always just a World wrapper) - `SceneRoot` -> `WorldAssetRoot` - `DynamicScene` -> `DynamicWorld` - `DynamicScene::from_scene` -> `DynamicWorld::from_world_asset` - `DynamicSceneBuilder` -> `DynamicWorldBuilder` - `DynamicSceneRoot` -> `DynamicWorldRoot` - `SceneInstanceReady` -> `WorldInstanceReady` - `SceneLoader` -> `WorldAssetLoader` - `ScenePlugin` -> `WorldSerializationPlugin` - `SceneRootTemplate` -> `WorldAssetRootTemplate` - `SceneSpawner` -> `WorldInstanceSpawner` - `SceneFilter` -> `WorldFilter` - `SceneLoaderError` -> `WorldAssetLoaderError` - `SceneSpawnError` -> `WorldInstanceSpawnError` Note that I went with `bevy_world_serialization` over `bevy_ecs_serialization`, as that is what all of the internal features described themselves as. I think it is both more specific and does a better job of making itself decoupled from `bevy_ecs` proper. |
||
|
IceSentry
|
2589ed242f |
Add render_layers scene to testbed_3d (#23559)
# Objective - RenderLayers are easy to break and so is multiple viewport support ## Solution - Add a new scene to testbed_3d that shows multiple cameras at the same time all with different combinations of render layers ## Testing - I ran the example locally and toggled between all the scenes --- ## Showcase <img width="1282" height="752" alt="testbed_3d_KmHPJ8pweU" src="https://github.com/user-attachments/assets/dad376f3-fd72-424d-b68d-7048843da179" /> As you can see, this already highlights a bug with shadows not respecting render layers |
||
|
François Mockers
|
5e6493b82d |
move white furnace to the 3d testbed (#23207)
# Objective - https://github.com/bevyengine/bevy/pull/22584 added a white furnace test, but it was not setup as a testbed - Fixes #23202 ## Solution - Add it to the 3d testbed |
||
|
Chintan Bhatt
|
7c153070c0 |
Adds text gizmos to testbeds (#23064)
# Objective - Fixed both #23058 and #23059 ## Solution - Adds text gizmos to the 2D and 3D testbed examples ## Testing Tested on Windows 11/Nvidia/Vulkan --- ## Showcase <details> <summary>Click to view showcase</summary> ### testbed_2d <img width="1282" height="752" alt="image" src="https://github.com/user-attachments/assets/ce20cf8f-c6f0-49be-8290-9f912c144f65" /> ### testbed_3d <img width="1282" height="752" alt="image" src="https://github.com/user-attachments/assets/2de280bd-bf2f-4335-a7ba-c25fc9a53d25" /> </details> |
||
|
Aevyrie
|
a88af65738 |
Contact Shadows (#22382)
# Objective - Implement contact shadows to add fine shadow detail where shadow cascades cannot. ## Solution - Extend our existing pbr implementation using our existing raymarching functions. --- ## Showcase <img width="1824" height="1180" alt="image" src="https://github.com/user-attachments/assets/e93b79c5-c596-4a9e-b94d-20bdde1d863b" /> <img width="1824" height="1180" alt="image" src="https://github.com/user-attachments/assets/0fd7dffa-60b8-4b92-8fad-7f993d4d89dd" /> https://github.com/user-attachments/assets/e74b190d-9ae3-4aaf-97f0-b520930a0667 https://github.com/user-attachments/assets/e80ccb26-bbaa-4d25-a823-8ea12354c5b9 https://github.com/user-attachments/assets/b04f4b00-92bd-4a2f-b7dd-5157d8fbe0ab <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/b7629908-dd32-48db-8ee7-a4d2dd8f66c2" /> <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/3de0258e-9191-4180-ac57-41b32e1205bd" /> <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/951477f9-e9a9-426f-ae8d-18ae50cc7b85" /> <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/2291453c-da57-4fcc-a6b0-f60f6eac6cbb" /> <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/5820cdff-ea54-4294-b520-2a8d8dc24996" /> <img width="1073" height="685" alt="image" src="https://github.com/user-attachments/assets/3ea16481-7689-4e99-87e2-1589f1532e4c" /> --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: charlotte 🌸 <charlotte.c.mcelwain@gmail.com> |
||
|
François Mockers
|
730b002260 |
fix testbeds with argh in wasm (#22339)
# Objective - #22223 used argh in the testbed examples - this compile in wasm but crashes when running them ## Solution - Fix it like the other examples using argh |
||
|
panpanpro888
|
9f02248af4 |
Add command line option to choose a starting scene in the `testbed_*' examples (#22223)
# Objective - Fixes #22218 ## Solution - Use `argh` to check if you passed a scene name, and if you did, start on that one instead of the default ## Testing - Tested on my local machine, I don't see a reason why it would change between different machines and/or platforms --------- Co-authored-by: Bayley Foster <43776524+apekros@users.noreply.github.com> Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
Greeble
|
a3ad40d412 |
Alternative glTF coordinate conversion (#20394)
## Objective Change glTF coordinate conversion to satisfy some common use cases while dodging the more controversial aspects. This fixes #20621, but at the cost of removing one feature. ## Summary The Bevy glTF loader can optionally convert nodes and meshes from glTF's "+Z forward" semantics to Bevy's "-Z forward". But the current implementation [has issues](https://github.com/bevyengine/bevy/issues/20621), particularly with cameras and lights. It might also cause problems for users who want to re-orient the scene as a whole while preserving the original node semantics. This PR replaces node conversion with a simpler correction to the scene root and mesh entities. The new approach satisfies many use cases and fixes the issues with cameras and lights. But it could be a regression for some users. ## Background There's been confusion over how glTF behaves and what users might want from coordinate conversion. This section recaps the basic concepts, glTF's semantics, the current loader behaviour, and some potential user stories. Or you can skip to the next section if you want to get straight to the changes. <details> <summary>Click to expand</summary> ### Coordinate Systems and Semantics 3D coordinate systems can have semantics assigned to their axes. These semantics are often defined as a forward axis, an up axis, and a [handedness](https://en.wikipedia.org/wiki/Right-hand_rule) - the side axis is implicit in the other choices. Bevy's standard semantics are "-Z = forward, +Y = up, right handed". This standard is codified by the `forward` and `up` methods of `Transform` and `GlobalTransform`, and by the renderer's interpretation of camera and light transforms. There are debates about the standard and whether users should be able to choose different semantics. This PR does not account for those debates, and assumes that users want to follow the current standard. Other engines, DCCs, and file formats can have [different semantics](https://mastodon.social/@acegikmo/113313928426095165). Unlike Bevy, some vary their semantics by object type - a camera's forward axis may not be the same as a light's. Some only specify an up axis, leaving the forward and side axes unspecified. Assets might not follow the standard semantics of their file format. Static mesh hierarchies and skeletal animation rigs may even have per-node or per-joint semantics - a character rig could be +Y forward in the scene, while the head joint is +Z forward. One character rig might have both feet +X forward, while another rig might have the left foot +X forward and the right foot -X forward. This creates complexity, but also creates jobs, so no-one can say if it's good or bad. ### Asset Loaders And Coordinate Conversion Bevy currently has a glTF loader, and I'm assuming it will get in-repo FBX and USD loaders at some point. These loaders are likely to follow a common pattern: - The files contain meshes, which correspond to Bevy `Mesh` assets and skinned meshes. - Bevy meshes can only have a single material, so what the file format considers a single mesh might be multiple Bevy meshes. - The files have a node hierarchy, where nodes roughly correspond to Bevy entities with a `Transform`. - Nodes can optionally be mesh instances, cameras, lights or skinned mesh joints. - The loader outputs the assets and a `Scene` with an entity hierarchy that tries to match the file's node hierarchy. - Some aspects of nodes (e.g. pivot transforms) can't be represented in Bevy within a single entity. - So a 1:1 mapping might not be possible - instead nodes become multiple entities, or some data is lost (e.g. baking down pivot transforms). - Users can choose to spawn the scene, or they can ignore it and use the assets directly. Users may want asset loaders that convert assets to Bevy's standard semantics, so `Transform::forward` matches the asset. But the details of conversion can be contentious - users may want some parts of the scene to be converted differently from other parts, and assets may have ambiguities than can only be resolved by the user. There will never be a simple "it just works" option, although there could be a least worst default that satisfies the biggest group of users. Converting in the loader is not the only option. The user could edit the assets themselves or run a conversion script in DCC. But that's a pain - particularly for users who rely on asset packs and don't have DCC experience. Another option is to implement an asset transform that does coordinate conversion. But having the options right there in the loader is convenient. ### User Stories For coordinate conversion in the loader, some user stories might be: - "I want to spawn a scene on an entity with Bevy semantics and have it look right." - This is probably the most common case - the user wants to do `SceneRoot(load("my.gltf"))` and have it visually match the entity's `Transform::forward()`, and cameras and lights should do the right thing. - The user might not care about the semantics of mesh assets and nodes in the scene - they just want the scene as a whole to look right. - "I want to spawn a scene, and convert some or all of the nodes to Bevy semantics." - The user might have nodes in their scene that they want to animate manually or hook up to other systems that assume Bevy semantics. - That becomes easier if the loader can convert the node's forward to match `Transform::forward()`. - Conversely, some users might want nodes to stay as they are (particularly skeletal animation rigs). - "I want a mesh asset that's converted to Bevy semantics. I'm not using a scene." - Maybe the user is doing `Mesh3d(load("mesh.gltf#Mesh0"))` and wants it to match the entity's forward. - Or this is the first stage of an asset pipeline and the remaining stages expect Bevy semantics. - "I don't want the loader to touch anything." - Maybe they've already converted the file, or want to convert it post-load, or don't want to use Bevy semantics at all. - "I want one of the other conversion stories, but the loader should convert to my chosen semantics rather than Bevy's". - Z-up is not a crime. ### glTF Semantics glTF [scene semantics](https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#coordinate-system-and-units) are "+Z = forward, +Y = up, right handed". This is almost the same as Bevy, except that scene forward is +Z instead of Bevy's -Z. Some glTF assets do not follow the spec's scene semantics. The Kenney asset packs use a mix of +Z and -Z forward. At least [one of the Khronos sample assets](https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/Duck) uses +X forward. That said, the majority of Kenney assets and almost all the Khronos sample assets I tested do follow the spec. glTF [camera node](https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#view-matrix) and [light node](https://github.com/KhronosGroup/glTF/blob/main/extensions/2.0/Khronos/KHR_lights_punctual/README.md#adding-light-instances-to-nodes) semantics are different from glTF scene semantics - they're -Z forward, same as Bevy. The glTF spec doesn't explicitly say if non-camera/light nodes and mesh buffers have semantics. I'm guessing that some users will have nodes and meshes that follow the spec's scene semantics, and might want them converted to Bevy semantics. But as noted in the user stories, it's likely that other users will have different needs. glTF and Bevy allow a single node/entity to be both a mesh and a camera or a light. This only makes sense if the user intends the mesh to have the same semantics as cameras and lights. I think it's very unlikely that significant numbers of users will want support for this combination - many other DCCs, file formats and engines don't support it at all. ### How The Bevy glTF Loader Works The loader maps glTF nodes to Bevy entities. It also adds entities for two cases: 1. A single "scene root" entity is added as a parent of the glTF root nodes. - Note that this is not the user's entity with the `SceneRoot` component - the scene root entity is a child of that entity. 2. Mesh primitive entities are added as a child of each glTF mesh node. - In glTF, a single mesh node can contain multiple primitives. - But in Bevy a mesh component can only contain a single primitive, so one entity can't contain multiple primitives. - So, for each primitive, Bevy adds a child entity with a mesh component. A single branch of the resulting scene hierarchy might look like this: - User entity with `SceneRoot` component. - Scene root entity. - glTF root node entity. - glTF intermediate node entities. - glTF mesh node entity (does not contain `Mesh3d` component) - Mesh primitive entities (does contain `Mesh3d` component). ### glTF Loader Changes In 0.17 In Bevy 0.16, the only user story supported by the glTF loader was "no conversion". During the 0.17 cycle, #19633 and some follow up PRs implemented an option that converts nodes, meshes and animation tracks. The changes do satisfy some user stories, including the common "convert scene semantics" (mostly) and "convert mesh semantics". But there's some problems (#20621): - The conversion depends on converting both nodes and meshes. - Some users might want to convert the scene without converting nodes and/or meshes. - Light and camera nodes get complicated. - glTF camera/light nodes already match Bevy semantics, so they need a counter-conversion (since their parent might have been converted). - Animation tracks for lights and cameras are not correctly converted. - (Counterpoint: This is fixable at the cost of some complexity) - Child nodes of lights and cameras are not correctly converted. - (Counterpoint: Also fixable, and probably a niche case?) - The conversion can't support a node that's a mesh instance and also a light and/or a camera. - (Counterpoint: As mentioned earlier, this is probably a very niche or non-existent use case.) </details> ## Solution The big change in this PR is the removal of node conversion. Instead, corrective transforms are applied to the scene root entity and mesh primitive entities. Before this PR: - Scene root entity. - glTF root node entity. <-- CONVERTED - glTF intermediate node entities. <-- CONVERTED - glTF mesh node entity. <-- CONVERTED - Mesh primitive entities. After this PR: - Scene root entity. <-- CORRECTIVE (if scene conversion enabled) - glTF root node entity. - glTF intermediate node entities. - glTF mesh node entity. - Mesh primitive entities. <-- CORRECTIVE (if mesh conversion enabled) The result is visually the same even though the scene internals are different. Cameras and lights now work correctly, including when animated. The new conversion is also simpler. There's no need to convert animations, and the scene part of the conversion only changes a single entity: ```diff +let world_root_transform = convert_coordinates.scene_conversion_transform(); let world_root_id = world - .spawn((Transform::default(), Visibility::default())) + .spawn((world_root_transform, Visibility::default())) .with_children(|parent| { for node in scene.nodes() { ``` Removing node conversion might be a regression for some users. My guess is that most users just want to spawn a scene with the correct orientation and don't worry about individual node transforms, so on balance this PR will be win. But I don't have much evidence to back that up. There might also be a path to adding node conversion back in as an option - see the "Future" section below. The previous conversion option - `GltfPlugin::use_model_forward_direction` - has been split into two separate options for scene and mesh conversion. ```diff struct GltfPlugin { ... - use_model_forward_direction: bool, + convert_coordinates: GltfConvertCoordinates, } ``` ```rust struct GltfConvertCoordinates { scenes: bool, meshes: bool, } ``` This might be turn out to be unnecessary flexibility, but I think it's the safer option for now in case users have unexpected needs. Both options are disabled by default. ### Testing I've tested various examples and glTFs with each combination of options, including glTFs with animated cameras and lights. ```sh # Visually the same as current Bevy *without* conversion. cargo run --example scene_viewer "assets/models/faces/faces.glb" cargo run --example scene_viewer "assets/models/faces/faces.glb" --convert-mesh-coordinates # Visually the same as current Bevy *with* conversion. cargo run --example scene_viewer "assets/models/faces/faces.glb" --convert-scene-coordinates cargo run --example scene_viewer "assets/models/faces/faces.glb" --convert-scene-coordinates --convert-mesh-coordinates cargo run --example animated_mesh ``` ## Future <details> <summary>Click to expand</summary> This PR removes node conversion, which is a desirable feature for some users. There are a couple of ways it could be added back as an option. The difficult part of node conversion is how to support camera and light nodes. glTF's camera/light semantics already match Bevy's -Z forward, so simply converting every node from +Z to -Z forward will leave camera and light nodes facing the wrong direction. The obvious solution is to special case camera/light node transforms - this is what the 0.17 conversion tries to do. But it's surprisingly complex to get right due to animation, child nodes, and nodes that can be meshes and cameras and lights. E.g. children of cameras and lights need a counter-conversion applied to their transform and animation tracks. For cameras, an alternative would be to split them multiple entities. The existing entity would correspond to the glTF node and be converted like every other node. But the Bevy `Camera` component would be on a new child entity and have a corrective transform. Before: - Parent glTF node entity. - Camera glTF node entity with `Camera` component and animated transform. - glTF node parented to camera node. After: - Parent glTF node entity. - Camera glTF node entity with animated transform. - New child entity with `Camera` component and corrective transform. - glTF node parented to camera node. Lights are already set up this way, so they only need the corrective transform. This approach is simpler since nodes are treated uniformly. And it's arguably a better reflection of the glTF format - glTF cameras are kind of a separate thing from nodes, and can be given a name that's different to their node's name. So it could be better for some users. The downside is that the glTF node entity might have the wrong semantics from the perspective of some users (although not all). And it will be annoying for users who currently assume the `Camera` component is on the node entity. </details> ## Alternatives <details> <summary>Click to expand</summary> ### What About The Forward Flag Proposal? There's a [proposal](https://github.com/bevyengine/bevy/pull/20135) to allow per-transform semantics, aka the "forward flag". This means the axis of `Transform::forward()` and others would depend on a variable in the `Transform`. In theory the forward flag might avoid the need for coordinate conversion in the loader. But whether that works in practice is unclear, and the proposal appears to be stalled. ### What Do Other Engines Do? [Godot's semantics](https://docs.godotengine.org/en/4.4/tutorials/assets_pipeline/importing_3d_scenes/model_export_considerations.html#d-asset-direction-conventions) are the same as the glTF standard. Godot doesn't offer any conversion options. Unreal's default semantics are "+X forward, +Z up, left handed", except meshes are typically "+Y forward, +Z up". Their glTF importer converts nodes and meshes to Unreal's mesh semantics - this is done by swapping the Y and Z axes, which implicitly flips the X for handedness. So Unreal's approach is actually closer to the current main approach of node + mesh conversion, versus this PR's scene + mesh conversion. The Unreal importer also supports a custom scene/mesh rotation and translation that's applied after normal conversion. There's no option to disable conversion. </details> --------- Co-authored-by: Carter Anderson <mcanders1@gmail.com> Co-authored-by: Alice I Cecile <alice.i.cecile@gmail.com> |
||
|
atlv
|
6b34012ffa |
Rename cuboid to cube (#21393)
# Objective - Gizmos::cuboid is for drawing cubes, even the docs say so. A cuboid drawing function should take Cuboid as input. ## Solution - Rename ::cuboid to ::cube, making way for an actual cuboid drawing function. ## Testing - ci |
||
|
Carter Anderson
|
eda118d033 |
Event Rearchitecture (#20731)
There is general consensus that our terminology for Events, "entity events", Observers, and BufferedEvents needs clarity. Additionally, many of us also agree that the current Observer system would benefit from additional static-ness: currently it is assumed that you can use events in pretty much any context, and they all go through the exact same code path. Alice put forth a proposal to [Overhaul Observers](https://hackmd.io/@bevy/rk4S92hmlg), and we have already partially implemented it for 0.17. I think it does a great job of outlining many of the issues at play, and it solves them reasonably well. But I _also_ think the proposed solution isn't yet ideal. Given that it is already partially implemented for 0.17, it is a breaking change, _and_ given that we have already broken the Observer API a number of times, I think we need to sort this out before the next release. This is a big changeset, but it is _largely_ just a reframing of what is already there. I haven't fundamentally changed the behaviors. I've just refined and constrained in a way that allows us to do what we are currently doing in a clearer, simpler, and more performant way. First, I'll give some quick notes on Alice's proposal (which you all should read if you haven't yet!): ### Notes on Alice's Proposal - I like the move toward a more static API - I think we've gone too far down the "separate terminology" path. The proposal introduces a zoo of apis, terms, and "subterms". I think we need to simplify our concepts and names to make this all easier to talk about and use in practice. - BroadcastEvent feels like the wrong name. EntityEvent is also "broadcast" in the exact same way - BufferedEvent is a completely different system than EntityEvent and BroadcastEvent. This muddles concepts too much. It needs its own standalone, single-word concept name. - "Universal observers": I think this should be fully context driven, rather than needing encoding in the API. - I agree we can't get rid of buffered events, and that merging them with "broadcast events" isn't helpful - I'm not quite sure how we'd make the proposed PropagateEvent subtrait work transparently. This can't be "layered on top" as a trait. It needs to be baked in at more fundamental level. * I don't like `app.add_broadcast_observers()`, `app.add_universal_observers()`, `Observer::entity_observer`, `Observer::broadcast`, etc. The `On` event should statically determine whether an observer is an "entity observer" or a "broadcast" Observer. This would already be encoded in the type system and is therefore something we can do on the developer's behalf. Likewise, any observer being registered at a top level is inherently _not_ a specific entity observer. All of these variants serve to make users guess and poke around in a way that is unnecessary. I want simple one word concept names, single constructors, etc. ### Proposed Principals - Static-ness: - Events should only be usable in the context they were defined to be used. - When triggered, Observers should *only* have access to fields and behaviors that are relevant: - Dont return Option or PLACEHOLDER: the field or function shouldn't exist - Entity events that don't support propagation shouldn't expose that functionality - Don't do unnecessary work at runtime - Event triggers shouldn't branch through every potential event code path - Don't clone potentially large lists of event context unnecessarily (Ex: we currently clone the component list for every observer invocation) - Minimize codegen - Don't recompile things redundantly. - Don't compile unnecessary code paths. - Clear and Simple - Minimize the number of concept names floating around, and lock each concept down heavily to a specific context - I'm convinced at this point that "buffered events" and "observer events" sharing concept names is wrong. We need two clean and clear terms, and I'm willing to give "buffered events" a slightly worse name if it means "observer events" can be nicer. - Don't throw the concept name "Event" out ... it is a very good name. Instead, constrain it to one specific thing. - Minimize our API surface - Events contain all context, including what used to be the "target". This lets people define the "target" name that makes the most sense for the context, and lets the documentation fully describe the context of that "target". ### Concepts - **Event** (the thing you "observe") - Rationale: "Event" is the clear choice for this concept. An "event" feels like something that happens in real time. "Event observers" are things that observe events when they occur (are triggered). Additionally, this is the concept that "propagates", and "event propagation" is a term people understand. - **Trigger**: (the verb that "causes" events to happen for targets). Events are Triggered. This can include additional context/ data that is passed to observers / informs the trigger behavior. Events have _exactly_ one Trigger. If you want a different trigger behavior, define a new event. This makes the system more static, more predictable, and easier to understand and document. `world.trigger_ref_with` makes it possible to pass in mutable reference to your own Trigger data, making it possible to customize the input trigger data and read out the final trigger data. - **Observer** (the thing that "observes" events): An event's `Trigger` determines which observers will run. - **Event Types**: You can build any "type" of event. The concept of a "target" has been removed. Instead, define a `Trigger` that expects a specific kind of event (ex: `E: EntityEvent`). - **EntityEvent** We add a new `EntityEvent` trait, which defines an `event.entity()` accessor. This is used by the `Trigger` impls : `EntityTrigger`, `PropagateEntityTrigger`, and `EntityComponentsTrigger`. - **Message** (the buffered thing you "read" and "write") - `Message` is a solid metaphor for what this is ... it is data that is written and then at some later point read by someone / something else. I expect existing consumers of "buffered events" to lament this name change, as "event" feels nicer. But having a separate name is within everyone's best interest. - **MessageReader** (the thing that reads messages) - **MessageWriter** (the thing that writes messages) ### The Changes - `Event` trait changes - Event is now used exclusively by Observers - Added `Event::Trigger`, which defines what trigger implementation this event will use - Added the `Trigger` trait - All of the shared / hard-coded observer trigger logic has been broken out into individual context-specific Trigger traits. - "Trigger Targets" have been removed. - Instead, Events, in combination with their Trigger impl, decide how they will be triggered. In general, this means that Events now include their "targets" as fields on the event. - APIs like `trigger_targets` have been replaced by `trigger`, which can now be used for any `Event` - `EntityEvent` trait changes - Propagation config has been removed from the `EntityEvent` trait. It now lives on the `Trigger` trait (specifically the `PropagateEntityTrigger` trait). - `EntityEvent` now provides `entity / entity_mut` accessors for the Event it is implemented for - `EntityEvent` defaults to having no propagation (uses the simpler `EntityTrigger`) - `#[entity_event(propagate)]` enables the "default" propagation logic (uses ChildOf). The existing `#[entity_event(traversal = X)]` has been renamed to `#[entity_event(propagate = X)` - Deriving `EntityEvent` requires either a single `MyEvent(Entity)`, the `entity` field name (`MyEvent { entity: Entity}`), or `MyEvent { #[event_entity] custom: Entity }` - Animation event changes - Animation events now have their own `AnimationEvent` trait, which sets the `AnimationEventTrigger`. This allows developers to pass in events that _dont_ include the Entity field (as this is set by the system). The custom trigger also opens the doors to cheaply passing in additional animation system context, accessible through `On` - `EntityComponentsTrigger` - The built in Add/Remove/etc lifecycle events now use the `EntityComponentsTrigger`, which passes in the components as additional state. This _significantly_ cuts down on clones, as it does a borrow rather than cloning the list into _each_ observer execution. - Each event now has an `entity` field. - Style changes - Prefer the event name for variables: `explode: On<Explode>` not `event: On<Explode>` - Prefer using the direct field name for the entity on entity events, rather than `event.entity()`. This allows us to use more specific names where appropriate, provides better / more contextual docs, and coaches developers to think of `On<MyEvent>` _as_ the event itself. Take a look at the changes to the examples and the built-in events to see what this looks like in practice. ### Downsides - Moving the "target" into the event adds some new constraints: - Triggering the same event for multiple entities requires multiple trigger calls. For "expensive" events (ex: lots of data attached to the event), this will be more awkward. Your options become: - Create multiple instances of the event, cloning the expensive data - Use `trigger_ref`, and mutate the event on each call to change the target. - Move the "expensive" shared data into the Trigger, and use `trigger_ref_with`` - We could build a new EntityEvent method that abstracts over the "event mutation" behavior and provides something like the old `trigger_target` behavior. - Use a different `EntityTargetTrigger` (not currently provided by bevy, but we could), which brings back the old behavior. This would be used with `trigger_with` to replicate the old pattern: `world.trigger_with(MyEvent, [e1, e2].into())` (or we could make the `into()` implicit) - Bubbling the event involves mutating the event to set the entity. This means that `trigger_ref` will result in the event's `EntityEvent::entity()` being the final bubbled entity instead of the initial entity. - Some APIs (trivially) benefit from the "target entity" being separate from the event. Specifically, this new API requires changes to the "Animation Event" system in AnimationPlayer. I think this is actually a good change set, as it allows us to: - Cheaply expose more animation state as part of a new AnimationEventTrigger impl - Move that "implict" entity target provided by the AnimationPlayer into the AnimationEventTrigger - Encode the "animation event trigger-ness" of the event into the type itself (by requiring `#[event(trigger = AnimationEventTrigger)]`) - By not implementing Default for AnimationEventTrigger, we can block animation events from being fired manually by the user. ### Draft TODO - [x] Fill in documentation and update existing docs - [ ] Benchmark: I expect this impl to be significantly faster. There might also be tangible binary size improvements, as I've removed a lot of redundant codegen. - [x] Update release notes and migration guides ### Next Steps - The `BufferedEvent -> Message` rename was not included to keep the size down. Fixes #19648 --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Jan Hohenheim <jan@hohenheim.ch> |
||
|
atlv
|
f6b77e4e49 |
bevy_post_process (#20778)
# Objective Split out post process effects from bevy_core_pipeline because they are not core pipelines. ## Solution @IceSentry proposed something like this, not sure if the split is exactly as he envisioned but this seems reasonable to me. The goal is to move absolutely everything possible out of bevy_core_pipelines to unblock bevy_pbr/bevy_sprite_render compilation. Future PRs may attempt to move more little bits out. ## Testing |
||
|
Jan Hohenheim
|
b43a73df3f |
Rename DespawnOnExitState to DespawnOnExit (#20872)
# Objective - `StateScoped` was renamed to `DespawnOnExitState`, and we introduced `DespawnOnEnterState` - This is redundant: the type it wraps is already a state - Often, state enums have `State` in their names, leading to stutter: `DespawnOnExitState(GameState::Gameplay)` - This component is *very* common in games. The longer name makes it clunkier to use, so we should be careful when expanding it. I think the added clarity is good, but we can do better. ## Solution - Compromise to `DespawnOnExit` - Do the same for `DespawnOnEnter` ## Testing - CI |
||
|
Carter Anderson
|
5058f8a9e6 |
Improve On Terminology (#20648)
# Objective Fixes #19263 (and expands on it) Within `Observers`, we have started to distance ourselves from the "trigger" terminology. Specifically, we have renamed `Trigger` to `On`. I think this was a very good move, but we're currently in an awkward middle ground state. Users interact with `On` as if it were an event: `On<E>` exposes the event and derefs directly to it. I think we should embrace this mindset fully, in the interest of clarity. ## Solution - Rename all `trigger: On<SomeEvent>` cases to `event: On<SomeEvent>`. - Rename `On::target` to `On::entity`. This reads _much_ better when writing `query.get(event.entity())` and pairs more effectively with the `EntityEvent` terminology. - Rename `On::original_target` to `On::original_entity`, for the same reasons. - Take advantage of the `Deref` behavior where appropriate ```rust // Before entity.observe(|trigger: On<Explode>| { println!("{} exploded!", trigger.target()); }) // After entity.observe(|event: On<Explode>| { println!("{} exploded!", event.entity()); }) ``` |
||
|
Greeble
|
590da83c24 |
Fix glTF coordinate conversion not converting mesh bounds (#20608)
glTF coordinate conversion is applied to mesh assets, but not their bounds: <img width="1602" height="939" alt="image" src="https://github.com/user-attachments/assets/b2bd0c4b-c9aa-4d53-b33d-c3463af7480e" /> After the fix: <img width="1602" height="939" alt="image" src="https://github.com/user-attachments/assets/ffb40e11-14e4-427f-adfb-5007e6b2cb0a" /> The PR also adds the above scene to `testbed_3d`. ## Testing ```sh cargo run --example testbed_3d ``` --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
theotherphil
|
2e259713c2 |
Bug fix in Gizmo grid (#19697)
# Objective Fix https://github.com/bevyengine/bevy/issues/19480 ## Solution Fix calculation of line counts in each direction ## Testing Added grids to the gizmos screens of testbed/example_3d and testbed/example_2d. <img width="1246" alt="image" src="https://github.com/user-attachments/assets/56924e44-1b54-43dc-9cf0-dba390570f3b" /> <img width="1246" alt="image" src="https://github.com/user-attachments/assets/566c3d1b-64aa-43bb-a1db-c2e41de2a7a6" /> |
||
|
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. |
||
|
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.
|
||
|
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
|
8a223be651 |
Enable state scoped entities by default (#19354)
# Objective - Enable state scoped entities by default - Provide a way to disable it when needed --------- Co-authored-by: Ben Frankel <ben.frankel7@gmail.com> |
||
|
Emerson Coskey
|
7ab00ca185 |
Split Camera.hdr out into a new component (#18873)
# Objective
- Simplify `Camera` initialization
- allow effects to require HDR
## Solution
- Split out `Camera.hdr` into a marker `Hdr` component
## Testing
- ran `bloom_3d` example
---
## Showcase
```rs
// before
commands.spawn((
Camera3d
Camera {
hdr: true
..Default::default()
}
))
// after
commands.spawn((Camera3d, Hdr));
// other rendering components can require that the camera enables hdr!
// currently implemented for Bloom, AutoExposure, and Atmosphere.
#[require(Hdr)]
pub struct Bloom;
```
|
||
|
mgi388
|
7a1fcb7fe7 |
Rename StateScoped to DespawnOnExitState and add DespawnOnEnterState (#18818)
# Objective - Alternative to and builds on top of #16284. - Fixes #15849. ## Solution - Rename component `StateScoped` to `DespawnOnExitState`. - Rename system `clear_state_scoped_entities` to `despawn_entities_on_exit_state`. - Add `DespawnOnEnterState` and `despawn_entities_on_enter_state` which is the `OnEnter` equivalent. > [!NOTE] > Compared to #16284, the main change is that I did the rename in such a way as to keep the terms `OnExit` and `OnEnter` together. In my own game, I was adding `VisibleOnEnterState` and `HiddenOnExitState` and when naming those, I kept the `OnExit` and `OnEnter` together. When I checked #16284 it stood out to me that the naming was a bit awkward. Putting the `State` in the middle and breaking up `OnEnter` and `OnExit` also breaks searching for those terms. ## Open questions 1. Should we split `enable_state_scoped_entities` into two functions, one for the `OnEnter` and one for the `OnExit`? I personally have zero need thus far for the `OnEnter` version, so I'd be interested in not having this enabled unless I ask for it. 2. If yes to 1., should we follow my lead in my `Visibility` state components (see below) and name these `app.enable_despawn_entities_on_enter_state()` and `app.enable_despawn_entities_on_exit_state()`, which IMO says what it does on the tin? ## Testing Ran all changed examples. ## Side note: `VisibleOnEnterState` and `HiddenOnExitState` For reference to anyone else and to help with the open questions, I'm including the code I wrote for controlling entity visibility when a state is entered/exited. <details> <summary>visibility.rs</summary> ```rust use bevy_app::prelude::*; use bevy_ecs::prelude::*; use bevy_reflect::prelude::*; use bevy_render::prelude::*; use bevy_state::{prelude::*, state::StateTransitionSteps}; use tracing::*; pub trait AppExtStates { fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self; fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self; } impl AppExtStates for App { fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self { self.main_mut() .enable_visible_entities_on_enter_state::<S>(); self } fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self { self.main_mut().enable_hidden_entities_on_exit_state::<S>(); self } } impl AppExtStates for SubApp { fn enable_visible_entities_on_enter_state<S: States>(&mut self) -> &mut Self { if !self .world() .contains_resource::<Events<StateTransitionEvent<S>>>() { let name = core::any::type_name::<S>(); warn!("Visible entities on enter state are enabled for state `{}`, but the state isn't installed in the app!", name); } // We work with [`StateTransition`] in set // [`StateTransitionSteps::ExitSchedules`] as opposed to [`OnExit`], // because [`OnExit`] only runs for one specific variant of the state. self.add_systems( StateTransition, update_to_visible_on_enter_state::<S>.in_set(StateTransitionSteps::ExitSchedules), ) } fn enable_hidden_entities_on_exit_state<S: States>(&mut self) -> &mut Self { if !self .world() .contains_resource::<Events<StateTransitionEvent<S>>>() { let name = core::any::type_name::<S>(); warn!("Hidden entities on exit state are enabled for state `{}`, but the state isn't installed in the app!", name); } // We work with [`StateTransition`] in set // [`StateTransitionSteps::ExitSchedules`] as opposed to [`OnExit`], // because [`OnExit`] only runs for one specific variant of the state. self.add_systems( StateTransition, update_to_hidden_on_exit_state::<S>.in_set(StateTransitionSteps::ExitSchedules), ) } } #[derive(Clone, Component, Debug, Reflect)] #[reflect(Component, Debug)] pub struct VisibleOnEnterState<S: States>(pub S); #[derive(Clone, Component, Debug, Reflect)] #[reflect(Component, Debug)] pub struct HiddenOnExitState<S: States>(pub S); /// Makes entities marked with [`VisibleOnEnterState<S>`] visible when the state /// `S` is entered. pub fn update_to_visible_on_enter_state<S: States>( mut transitions: EventReader<StateTransitionEvent<S>>, mut query: Query<(&VisibleOnEnterState<S>, &mut Visibility)>, ) { // We use the latest event, because state machine internals generate at most // 1 transition event (per type) each frame. No event means no change // happened and we skip iterating all entities. let Some(transition) = transitions.read().last() else { return; }; if transition.entered == transition.exited { return; } let Some(entered) = &transition.entered else { return; }; for (binding, mut visibility) in query.iter_mut() { if binding.0 == *entered { visibility.set_if_neq(Visibility::Visible); } } } /// Makes entities marked with [`HiddenOnExitState<S>`] invisible when the state /// `S` is exited. pub fn update_to_hidden_on_exit_state<S: States>( mut transitions: EventReader<StateTransitionEvent<S>>, mut query: Query<(&HiddenOnExitState<S>, &mut Visibility)>, ) { // We use the latest event, because state machine internals generate at most // 1 transition event (per type) each frame. No event means no change // happened and we skip iterating all entities. let Some(transition) = transitions.read().last() else { return; }; if transition.entered == transition.exited { return; } let Some(exited) = &transition.exited else { return; }; for (binding, mut visibility) in query.iter_mut() { if binding.0 == *exited { visibility.set_if_neq(Visibility::Hidden); } } } ``` </details> --------- Co-authored-by: Benjamin Brienen <Benjamin.Brienen@outlook.com> Co-authored-by: Ben Frankel <ben.frankel7@gmail.com> |
||
|
François Mockers
|
4b457cc2ce |
Revert "don't use bevy_pbr for base bevy_gizmos plugin" (#18327)
# Objective - #17581 broke gizmos - Fixes #18325 ## Solution - Revert #17581 - Add gizmos to testbed ## Testing - Run any example with gizmos, it renders correctly |
||
|
Rob Parrett
|
0cb3eaef67 |
Fix validation errors in Fox.glb (#17801)
# Objective Fix gltf validation errors in `Fox.glb`. Inspired by #8099, but that issue doesn't appear to describe a real bug to fix, as far as I can tell. ## Solution Use the latest version of the Fox from [glTF-Sample-Assets](https://github.com/KhronosGroup/glTF-Sample-Assets/blob/main/Models/Fox/glTF-Binary/Fox.glb). ## Testing Dropped both versions in https://github.khronos.org/glTF-Validator/ `cargo run --example animated_mesh` seems to still look fine. Before: ``` The asset contains errors. "numErrors": 126, "numWarnings": 4184, ``` After: ``` The asset is valid. "numErrors": 0, "numWarnings": 0, ``` ## Discussion The 3d testbed was panicking with ``` thread 'main' panicked at examples/testbed/3d.rs:288:60: called `Result::unwrap()` on an `Err` value: QueryDoesNotMatch(35v1 with components Transform, GlobalTransform, Visibility, InheritedVisibility, ViewVisibility, ChildOf, Children, Name) ``` Which is bizarre. I think this might be related to #17720, or maybe the structure of the gltf changed. I fixed it by using updating the testbed to use a more robust method of finding the correct entity as is done in `animated_mesh`. |
||
|
François Mockers
|
7d141829be |
run example in CI on windows using static dxc (#17783)
# Objective - Run more things on windows ## Solution - With the update of wgpu and the statically linked dxc, examples now run on windows in CI |
||
|
François Mockers
|
e57f73207e |
Smarter testbeds (#17573)
# Objective - Improve CI when testing rendering by having smarter testbeds ## Solution - CI testing no longer need a config file and will run with a default config if not found - It is now possible to give a name to a screenshot instead of just a frame number - 2d and 3d testbeds are now driven from code - a new system in testbed will watch for state changed - on state changed, trigger a screenshot 100 frames after (so that the scene has time to render) with the name of the scene - when the screenshot is taken (`Captured` component has been removed), switch scene - this means less setup to run a testbed (no need for a config file), screenshots have better names, and it's faster as we don't wait 100 frames for the screenshot to be taken ## Testing - `cargo run --example testbed_2d --features bevy_ci_testing` |
||
|
Alice Cecile
|
6fd6ce1367 |
Feature flag testbed_3d code correctly (#16866)
# Objective Rust-Analyzer was reporting problems with dead code in the 3d testbed scene. ## Solution These scenes don't work in CI on the Windows runner (because they're too weak). Mirror the feature flags from above onto the offending modules. ## Testing RA no longer complains. |
||
|
Aevyrie
|
61b98ec80f |
Rename trigger.entity() to trigger.target() (#16716)
# Objective - A `Trigger` has multiple associated `Entity`s - the entity observing the event, and the entity that was targeted by the event. - The field `entity: Entity` encodes no semantic information about what the entity is used for, you can already tell that it's an `Entity` by the type signature! ## Solution - Rename `trigger.entity()` to `trigger.target()` --- ## Changelog - `Trigger`s are associated with multiple entities. `Trigger::entity()` has been renamed to `Trigger::target()` to reflect the semantics of the entity being returned. ## Migration Guide - Rename `Trigger::entity()` to `Trigger::target()`. - Rename `ObserverTrigger::entity` to `ObserverTrigger::target` |
||
|
François Mockers
|
fcfb685821 |
enable_state_scoped_entities() as a derive attribute (#16180)
# Objective - I got tired of calling `enable_state_scoped_entities`, and though it would make more sense to define that at the place where the state is defined ## Solution - add a derive attribute `#[states(scoped_entities)]` when derive `States` or `SubStates` that enables it automatically when adding the state ## Testing - Ran the examples using it, they still work |
||
|
François Mockers
|
ec268420f7 |
Check examples screenshots on windows (#16010)
# Objective - Checks screenshots on Windows - Progress towards #15918 ## Solution - Checks screenshots on Windows - Also disable the helmet gltf scene in windows ci as it doesn't work |
||
|
François Mockers
|
74dedb2841 |
Testbed for 3d (#15993)
# Objective - Progress towards #15918 - Add tests for 3d ## Solution - Add tests that cover lights, bloom, gltf and animation - Removed examples `contributors` and `load_gltf` as they don't contribute additional checks to CI ## Testing - `CI_TESTING_CONFIG=.github/example-run/testbed_3d.ron cargo run --example testbed_3d --features "bevy_ci_testing"` |