-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retain Rendering World (adopted) #15320
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit b8b8225.
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 30, 2024
# Objective Fixes #15367. Currently, required components can only be defined through the `require` macro attribute. While this should be used in most cases, there are also several instances where you may want to define requirements at runtime, commonly in plugins. Example use cases: - Require components only if the relevant optional plugins are enabled. For example, a `SleepTimer` component (for physics) is only relevant if the `SleepPlugin` is enabled. - Third party crates can define their own requirements for first party types. For example, "each `Handle<Mesh>` should require my custom rendering data components". This also gets around the orphan rule. - Generic plugins that add marker components based on the existence of other components, like a generic `ColliderPlugin<C: AnyCollider>` that wants to add a `ColliderMarker` component for all types of colliders. - This is currently relevant for the retained render world in #15320. The `ExtractComponentPlugin<C>` should add `SyncToRenderWorld` to all components that should be extracted. This is currently done with observers, which is more expensive than required components, and causes archetype moves. - Replace some built-in components with custom versions. For example, if `GlobalTransform` required `Transform` through `TransformPlugin`, but we wanted to use a `CustomTransform` type, we could replace `TransformPlugin` with our own plugin. (This specific example isn't good, but there are likely better use cases where this may be useful) See #15367 for more in-depth reasoning. ## Solution Add `register_required_components::<T, R>` and `register_required_components_with::<T, R>` methods for `Default` and custom constructors respectively. These methods exist on `App` and `World`. ```rust struct BirdPlugin; impl Plugin for BirdPlugin { fn plugin(app: &mut App) { // Make `Bird` require `Wings` with a `Default` constructor. app.register_required_components::<Bird, Wings>(); // Make `Wings` require `FlapSpeed` with a custom constructor. // Fun fact: Some hummingbirds can flutter their wings 80 times per second! app.register_required_components_with::<Wings, FlapSpeed>(|| FlapSpeed::from_duration(1.0 / 80.0)); } } ``` The custom constructor is a function pointer to match the `require` API, though it could take a raw value too. Requirement inheritance works similarly as with the `require` attribute. If `Bird` required `FlapSpeed` directly, it would take precedence over indirectly requiring it through `Wings`. The same logic applies to all levels of the inheritance tree. Note that registering the same component requirement more than once will panic, similarly to trying to add multiple component hooks of the same type to the same component. This avoids constructor conflicts and confusing ordering issues. ### Implementation Runtime requirements have two additional challenges in comparison to the `require` attribute. 1. The `require` attribute uses recursion and macros with clever ordering to populate hash maps of required components for each component type. The expected semantics are that "more specific" requirements override ones deeper in the inheritance tree. However, at runtime, there is no representation of how "specific" each requirement is. 2. If you first register the requirement `X -> Y`, and later register `Y -> Z`, then `X` should also indirectly require `Z`. However, `Y` itself doesn't know that it is required by `X`, so it's not aware that it should update the list of required components for `X`. My solutions to these problems are: 1. Store the depth in the inheritance tree for each entry of a given component's `RequiredComponents`. This is used to determine how "specific" each requirement is. For `require`-based registration, these depths are computed as part of the recursion. 2. Store and maintain a `required_by` list in each component's `ComponentInfo`, next to `required_components`. For `require`-based registration, these are also added after each registration, as part of the recursion. When calling `register_required_components`, it works as follows: 1. Get the required components of `Foo`, and check that `Bar` isn't already a *direct* requirement. 3. Register `Bar` as a required component for `Foo`, and add `Foo` to the `required_by` list for `Bar`. 4. Find and register all indirect requirements inherited from `Bar`, adding `Foo` to the `required_by` list for each component. 5. Iterate through components that require `Foo`, registering the new inherited requires for them as indirect requirements. The runtime registration is likely slightly more expensive than the `require` version, but it is a one-time cost, and quite negligible in practice, unless projects have hundreds or thousands of runtime requirements. I have not benchmarked this however. This does also add a small amount of extra cost to the `require` attribute for updating `required_by` lists, but I expect it to be very minor. ## Testing I added some tests that are copies of the `require` versions, as well as some tests that are more specific to the runtime implementation. I might add a few more tests though. ## Discussion - Is `register_required_components` a good name? Originally I went for `register_component_requirement` to be consistent with `register_component_hooks`, but the general feature is often referred to as "required components", which is why I changed it to `register_required_components`. - Should we *not* panic for duplicate requirements? If so, should they just be ignored, or should the latest registration overwrite earlier ones? - If we do want to panic for duplicate, conflicting registrations, should we at least not panic if the registrations are *exactly* the same, i.e. same component and same constructor? The current implementation panics for all duplicate direct registrations regardless of the constructor. ## Next Steps - Allow `register_required_components` to take a `Bundle` instead of a single required component. - I could also try to do it in this PR if that would be preferable. - Not directly related, but archetype invariants?
5 tasks
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
- Adopted from bevyengine#14449 - Still fixes bevyengine#12144. ## Migration Guide The retained render world is a complex change: migrating might take one of a few different forms depending on the patterns you're using. For every example, we specify in which world the code is run. Most of the changes affect render world code, so for the average Bevy user who's using Bevy's high-level rendering APIs, these changes are unlikely to affect your code. ### Spawning entities in the render world Previously, if you spawned an entity with `world.spawn(...)`, `commands.spawn(...)` or some other method in the rendering world, it would be despawned at the end of each frame. In 0.15, this is no longer the case and so your old code could leak entities. This can be mitigated by either re-architecting your code to no longer continuously spawn entities (like you're used to in the main world), or by adding the `bevy_render::world_sync::TemporaryRenderEntity` component to the entity you're spawning. Entities tagged with `TemporaryRenderEntity` will be removed at the end of each frame (like before). ### Extract components with `ExtractComponentPlugin` ``` // main world app.add_plugins(ExtractComponentPlugin::<ComponentToExtract>::default()); ``` `ExtractComponentPlugin` has been changed to only work with synced entities. Entities are automatically synced if `ComponentToExtract` is added to them. However, entities are not "unsynced" if any given `ComponentToExtract` is removed, because an entity may have multiple components to extract. This would cause the other components to no longer get extracted because the entity is not synced. So be careful when only removing extracted components from entities in the render world, because it might leave an entity behind in the render world. The solution here is to avoid only removing extracted components and instead despawn the entire entity. ### Manual extraction using `Extract<Query<(Entity, ...)>>` ```rust // in render world, inspired by bevy_pbr/src/cluster/mod.rs pub fn extract_clusters( mut commands: Commands, views: Extract<Query<(Entity, &Clusters, &Camera)>>, ) { for (entity, clusters, camera) in &views { // some code commands.get_or_spawn(entity).insert(...); } } ``` One of the primary consequences of the retained rendering world is that there's no longer a one-to-one mapping from entity IDs in the main world to entity IDs in the render world. Unlike in Bevy 0.14, Entity 42 in the main world doesn't necessarily map to entity 42 in the render world. Previous code which called `get_or_spawn(main_world_entity)` in the render world (`Extract<Query<(Entity, ...)>>` returns main world entities). Instead, you should use `&RenderEntity` and `render_entity.id()` to get the correct entity in the render world. Note that this entity does need to be synced first in order to have a `RenderEntity`. When performing manual abstraction, this won't happen automatically (like with `ExtractComponentPlugin`) so add a `SyncToRenderWorld` marker component to the entities you want to extract. This results in the following code: ```rust // in render world, inspired by bevy_pbr/src/cluster/mod.rs pub fn extract_clusters( mut commands: Commands, views: Extract<Query<(&RenderEntity, &Clusters, &Camera)>>, ) { for (render_entity, clusters, camera) in &views { // some code commands.get_or_spawn(render_entity.id()).insert(...); } } // in main world, when spawning world.spawn(Clusters::default(), Camera::default(), SyncToRenderWorld) ``` ### Looking up `Entity` ids in the render world As previously stated, there's now no correspondence between main world and render world `Entity` identifiers. Querying for `Entity` in the render world will return the `Entity` id in the render world: query for `MainEntity` (and use its `id()` method) to get the corresponding entity in the main world. This is also a good way to tell the difference between synced and unsynced entities in the render world, because unsynced entities won't have a `MainEntity` component. --------- Co-authored-by: re0312 <re0312@outlook.com> Co-authored-by: re0312 <45868716+re0312@users.noreply.github.com> Co-authored-by: Periwink <charlesbour@gmail.com> Co-authored-by: Anselmo Sampietro <ans.samp@gmail.com> Co-authored-by: Emerson Coskey <56370779+ecoskey@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Christian Hughes <9044780+ItsDoot@users.noreply.github.com>
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
# Objective Fixes bevyengine#15367. Currently, required components can only be defined through the `require` macro attribute. While this should be used in most cases, there are also several instances where you may want to define requirements at runtime, commonly in plugins. Example use cases: - Require components only if the relevant optional plugins are enabled. For example, a `SleepTimer` component (for physics) is only relevant if the `SleepPlugin` is enabled. - Third party crates can define their own requirements for first party types. For example, "each `Handle<Mesh>` should require my custom rendering data components". This also gets around the orphan rule. - Generic plugins that add marker components based on the existence of other components, like a generic `ColliderPlugin<C: AnyCollider>` that wants to add a `ColliderMarker` component for all types of colliders. - This is currently relevant for the retained render world in bevyengine#15320. The `ExtractComponentPlugin<C>` should add `SyncToRenderWorld` to all components that should be extracted. This is currently done with observers, which is more expensive than required components, and causes archetype moves. - Replace some built-in components with custom versions. For example, if `GlobalTransform` required `Transform` through `TransformPlugin`, but we wanted to use a `CustomTransform` type, we could replace `TransformPlugin` with our own plugin. (This specific example isn't good, but there are likely better use cases where this may be useful) See bevyengine#15367 for more in-depth reasoning. ## Solution Add `register_required_components::<T, R>` and `register_required_components_with::<T, R>` methods for `Default` and custom constructors respectively. These methods exist on `App` and `World`. ```rust struct BirdPlugin; impl Plugin for BirdPlugin { fn plugin(app: &mut App) { // Make `Bird` require `Wings` with a `Default` constructor. app.register_required_components::<Bird, Wings>(); // Make `Wings` require `FlapSpeed` with a custom constructor. // Fun fact: Some hummingbirds can flutter their wings 80 times per second! app.register_required_components_with::<Wings, FlapSpeed>(|| FlapSpeed::from_duration(1.0 / 80.0)); } } ``` The custom constructor is a function pointer to match the `require` API, though it could take a raw value too. Requirement inheritance works similarly as with the `require` attribute. If `Bird` required `FlapSpeed` directly, it would take precedence over indirectly requiring it through `Wings`. The same logic applies to all levels of the inheritance tree. Note that registering the same component requirement more than once will panic, similarly to trying to add multiple component hooks of the same type to the same component. This avoids constructor conflicts and confusing ordering issues. ### Implementation Runtime requirements have two additional challenges in comparison to the `require` attribute. 1. The `require` attribute uses recursion and macros with clever ordering to populate hash maps of required components for each component type. The expected semantics are that "more specific" requirements override ones deeper in the inheritance tree. However, at runtime, there is no representation of how "specific" each requirement is. 2. If you first register the requirement `X -> Y`, and later register `Y -> Z`, then `X` should also indirectly require `Z`. However, `Y` itself doesn't know that it is required by `X`, so it's not aware that it should update the list of required components for `X`. My solutions to these problems are: 1. Store the depth in the inheritance tree for each entry of a given component's `RequiredComponents`. This is used to determine how "specific" each requirement is. For `require`-based registration, these depths are computed as part of the recursion. 2. Store and maintain a `required_by` list in each component's `ComponentInfo`, next to `required_components`. For `require`-based registration, these are also added after each registration, as part of the recursion. When calling `register_required_components`, it works as follows: 1. Get the required components of `Foo`, and check that `Bar` isn't already a *direct* requirement. 3. Register `Bar` as a required component for `Foo`, and add `Foo` to the `required_by` list for `Bar`. 4. Find and register all indirect requirements inherited from `Bar`, adding `Foo` to the `required_by` list for each component. 5. Iterate through components that require `Foo`, registering the new inherited requires for them as indirect requirements. The runtime registration is likely slightly more expensive than the `require` version, but it is a one-time cost, and quite negligible in practice, unless projects have hundreds or thousands of runtime requirements. I have not benchmarked this however. This does also add a small amount of extra cost to the `require` attribute for updating `required_by` lists, but I expect it to be very minor. ## Testing I added some tests that are copies of the `require` versions, as well as some tests that are more specific to the runtime implementation. I might add a few more tests though. ## Discussion - Is `register_required_components` a good name? Originally I went for `register_component_requirement` to be consistent with `register_component_hooks`, but the general feature is often referred to as "required components", which is why I changed it to `register_required_components`. - Should we *not* panic for duplicate requirements? If so, should they just be ignored, or should the latest registration overwrite earlier ones? - If we do want to panic for duplicate, conflicting registrations, should we at least not panic if the registrations are *exactly* the same, i.e. same component and same constructor? The current implementation panics for all duplicate direct registrations regardless of the constructor. ## Next Steps - Allow `register_required_components` to take a `Bundle` instead of a single required component. - I could also try to do it in this PR if that would be preferable. - Not directly related, but archetype invariants?
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
… world changes (bevyengine#15578) # Objective Update the UI's texture slice extraction to work with the changes from the retained render world PR (bevyengine#15320).
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 7, 2024
# Objective After merging retained rendering world #15320, we now have a good way of creating a link between worlds (*HIYAA intensifies*). This means that `get_or_spawn` is no longer necessary for that function. Entity should be opaque as the warning above `get_or_spawn` says. This is also part of #15459. I'm deprecating `get_or_spawn_batch` in a different PR in order to keep the PR small in size. ## Solution Deprecate `get_or_spawn` and replace it with `get_entity` in most contexts. If it's possible to query `&RenderEntity`, then the entity is synced and `render_entity.id()` is initialized in the render world. ## Migration Guide If you are given an `Entity` and you want to do something with it, use `Commands.entity(...)` or `World.entity(...)`. If instead you want to spawn something use `Commands.spawn(...)` or `World.spawn(...)`. If you are not sure if an entity exists, you can always use `get_entity` and match on the `Option<...>` that is returned. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
alice-i-cecile
changed the title
The Cooler 'Retain Rendering World'
Retain Rendering World (adopted)
Oct 10, 2024
targrub
pushed a commit
to targrub/bevy
that referenced
this pull request
Oct 10, 2024
# Objective In the Render World, there are a number of collections that are derived from Main World entities and are used to drive rendering. The most notable are: - `VisibleEntities`, which is generated in the `check_visibility` system and contains visible entities for a view. - `ExtractedInstances`, which maps entity ids to asset ids. In the old model, these collections were trivially kept in sync -- any extracted phase item could look itself up because the render entity id was guaranteed to always match the corresponding main world id. After bevyengine#15320, this became much more complicated, and was leading to a number of subtle bugs in the Render World. The main rendering systems, i.e. `queue_material_meshes` and `queue_material2d_meshes`, follow a similar pattern: ```rust for visible_entity in visible_entities.iter::<With<Mesh2d>>() { let Some(mesh_instance) = render_mesh_instances.get_mut(visible_entity) else { continue; }; // Look some more stuff up and specialize the pipeline... let bin_key = Opaque2dBinKey { pipeline: pipeline_id, draw_function: draw_opaque_2d, asset_id: mesh_instance.mesh_asset_id.into(), material_bind_group_id: material_2d.get_bind_group_id().0, }; opaque_phase.add( bin_key, *visible_entity, BinnedRenderPhaseType::mesh(mesh_instance.automatic_batching), ); } ``` In this case, `visible_entities` and `render_mesh_instances` are both collections that are created and keyed by Main World entity ids, and so this lookup happens to work by coincidence. However, there is a major unintentional bug here: namely, because `visible_entities` is a collection of Main World ids, the phase item being queued is created with a Main World id rather than its correct Render World id. This happens to not break mesh rendering because the render commands used for drawing meshes do not access the `ItemQuery` parameter, but demonstrates the confusion that is now possible: our UI phase items are correctly being queued with Render World ids while our meshes aren't. Additionally, this makes it very easy and error prone to use the wrong entity id to look up things like assets. For example, if instead we ignored visibility checks and queued our meshes via a query, we'd have to be extra careful to use `&MainEntity` instead of the natural `Entity`. ## Solution Make all collections that are derived from Main World data use `MainEntity` as their key, to ensure type safety and avoid accidentally looking up data with the wrong entity id: ```rust pub type MainEntityHashMap<V> = hashbrown::HashMap<MainEntity, V, EntityHash>; ``` Additionally, we make all `PhaseItem` be able to provide both their Main and Render World ids, to allow render phase implementors maximum flexibility as to what id should be used to look up data. You can think of this like tracking at the type level whether something in the Render World should use it's "primary key", i.e. entity id, or needs to use a foreign key, i.e. `MainEntity`. ## Testing ##### TODO: This will require extensive testing to make sure things didn't break! Additionally, some extraction logic has become more complicated and needs to be checked for regressions. ## Migration Guide With the advent of the retained render world, collections that contain references to `Entity` that are extracted into the render world have been changed to contain `MainEntity` in order to prevent errors where a render world entity id is used to look up an item by accident. Custom rendering code may need to be changed to query for `&MainEntity` in order to look up the correct item from such a collection. Additionally, users who implement their own extraction logic for collections of main world entity should strongly consider extracting into a different collection that uses `MainEntity` as a key. Additionally, render phases now require specifying both the `Entity` and `MainEntity` for a given `PhaseItem`. Custom render phases should ensure `MainEntity` is available when queuing a phase item.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 13, 2024
…15745) # Objective #15320 is a particularly painful breaking change, and the new `RenderEntity` in particular is very noisy, with a lot of `let entity = entity.id()` spam. ## Solution Implement `WorldQuery`, `QueryData` and `ReadOnlyQueryData` for `RenderEntity` and `WorldEntity`. These work the same as the `Entity` impls from a user-facing perspective: they simply return an owned (copied) `Entity` identifier. This dramatically reduces noise and eases migration. Under the hood, these impls defer to the implementations for `&T` for everything other than the "call .id() for the user" bit, as they involve read-only access to component data. Doing it this way (as opposed to implementing a custom fetch, as tried in the first commit) dramatically reduces the maintenance risk of complex unsafe code outside of `bevy_ecs`. To make this easier (and encourage users to do this themselves!), I've made `ReadFetch` and `WriteFetch` slightly more public: they're no longer `doc(hidden)`. This is a good change, since trying to vendor the logic is much worse than just deferring to the existing tested impls. ## Testing I've run a handful of rendering examples (breakout, alien_cake_addict, auto_exposure, fog_volumes, box_shadow) and nothing broke. ## Follow-up We should lint for the uses of `&RenderEntity` and `&MainEntity` in queries: this is just less nice for no reason. --------- Co-authored-by: Trashtalk217 <trashtalk217@gmail.com>
4 tasks
I'll tackle the release notes for this. I think that getting something in place for this ASAP is important to help communicate these changes to the rendering devs and ease initial migration during the release candidate. |
This was referenced Oct 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-ECS
Entities, components, systems, and events
A-Rendering
Drawing game state to the screen
D-Complex
Quite challenging from either a design or technical perspective. Ask for help!
M-Needs-Migration-Guide
A breaking change to Bevy's public API that needs to be noted in a migration guide
M-Needs-Release-Note
Work that should be called out in the blog due to impact
S-Ready-For-Final-Review
This PR has been approved by the community. It's ready for a maintainer to consider merging it
X-Blessed
Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migration Guide
The retained render world is a complex change: migrating might take one of a few different forms depending on the patterns you're using.
For every example, we specify in which world the code is run. Most of the changes affect render world code, so for the average Bevy user who's using Bevy's high-level rendering APIs, these changes are unlikely to affect your code.
Spawning entities in the render world
Previously, if you spawned an entity with
world.spawn(...)
,commands.spawn(...)
or some other method in the rendering world, it would be despawned at the end of each frame. In 0.15, this is no longer the case and so your old code could leak entities. This can be mitigated by either re-architecting your code to no longer continuously spawn entities (like you're used to in the main world), or by adding thebevy_render::world_sync::TemporaryRenderEntity
component to the entity you're spawning. Entities tagged withTemporaryRenderEntity
will be removed at the end of each frame (like before).Extract components with
ExtractComponentPlugin
ExtractComponentPlugin
has been changed to only work with synced entities. Entities are automatically synced ifComponentToExtract
is added to them. However, entities are not "unsynced" if any givenComponentToExtract
is removed, because an entity may have multiple components to extract. This would cause the other components to no longer get extracted because the entity is not synced.So be careful when only removing extracted components from entities in the render world, because it might leave an entity behind in the render world. The solution here is to avoid only removing extracted components and instead despawn the entire entity.
Manual extraction using
Extract<Query<(Entity, ...)>>
One of the primary consequences of the retained rendering world is that there's no longer a one-to-one mapping from entity IDs in the main world to entity IDs in the render world. Unlike in Bevy 0.14, Entity 42 in the main world doesn't necessarily map to entity 42 in the render world.
Previous code which called
get_or_spawn(main_world_entity)
in the render world (Extract<Query<(Entity, ...)>>
returns main world entities). Instead, you should use&RenderEntity
andrender_entity.id()
to get the correct entity in the render world. Note that this entity does need to be synced first in order to have aRenderEntity
.When performing manual abstraction, this won't happen automatically (like with
ExtractComponentPlugin
) so add aSyncToRenderWorld
marker component to the entities you want to extract.This results in the following code:
Looking up
Entity
ids in the render worldAs previously stated, there's now no correspondence between main world and render world
Entity
identifiers.Querying for
Entity
in the render world will return theEntity
id in the render world: query forMainEntity
(and use itsid()
method) to get the corresponding entity in the main world.This is also a good way to tell the difference between synced and unsynced entities in the render world, because unsynced entities won't have a
MainEntity
component.