From a66cc70576adecb49fb52d59acd8a58e05c177a6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 19 Jun 2022 00:35:13 -0700 Subject: [PATCH 01/18] Split sort phase implementations based on what's required --- crates/bevy_core_pipeline/src/core_2d/mod.rs | 6 +++++- crates/bevy_render/src/render_phase/draw.rs | 17 +++++++++++++++++ crates/bevy_render/src/render_phase/mod.rs | 10 +++++++++- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/crates/bevy_core_pipeline/src/core_2d/mod.rs b/crates/bevy_core_pipeline/src/core_2d/mod.rs index 8acdb42da54ef..5ce696037fc26 100644 --- a/crates/bevy_core_pipeline/src/core_2d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_2d/mod.rs @@ -22,7 +22,7 @@ use bevy_render::{ render_graph::{RenderGraph, SlotInfo, SlotType}, render_phase::{ batch_phase_system, sort_phase_system, BatchedPhaseItem, CachedRenderPipelinePhaseItem, - DrawFunctionId, DrawFunctions, EntityPhaseItem, PhaseItem, RenderPhase, + RenderPhaseSortMode, DrawFunctionId, DrawFunctions, EntityPhaseItem, PhaseItem, RenderPhase, }, render_resource::CachedRenderPipelineId, RenderApp, RenderStage, @@ -90,6 +90,10 @@ impl PhaseItem for Transparent2d { fn draw_function(&self) -> DrawFunctionId { self.draw_function } + + fn sort_mode() -> RenderPhaseSortMode { + RenderPhaseSortMode::Stable + } } impl EntityPhaseItem for Transparent2d { diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 5ffca0ea25886..52c44dda988ec 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -30,6 +30,16 @@ pub trait Draw: Send + Sync + 'static { ); } +pub enum RenderPhaseSortMode { + /// Requires a stable sort. Generally required for proper batching based on external criteria. + Stable, + /// The default: allows unstable sorting. Usually faster than Stable. + Unstable, + /// Unsorted. Omits sorting entirely. + Unsorted, +} + + /// An item which will be drawn to the screen. A phase item should be queued up for rendering /// during the [`RenderStage::Queue`](crate::RenderStage::Queue) stage. /// Afterwards it will be sorted and rendered automatically in the @@ -42,6 +52,13 @@ pub trait PhaseItem: Send + Sync + 'static { fn sort_key(&self) -> Self::SortKey; /// Specifies the [`Draw`] function used to render the item. fn draw_function(&self) -> DrawFunctionId; + + /// Specifies whether the phase requires batching. This should return true if and only if + /// the type implements [`BatchPhaseItem`]. + #[inline] + fn sort_mode() -> RenderPhaseSortMode { + RenderPhaseSortMode::Unstable + } } // TODO: make this generic? diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 76de6bf66501f..7a4cc79585966 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -29,7 +29,11 @@ impl RenderPhase { /// Sorts all of its [`PhaseItems`](PhaseItem). pub fn sort(&mut self) { - self.items.sort_by_key(|d| d.sort_key()); + match I::sort_mode() { + RenderPhaseSortMode::Stable => self.items.sort_by_key(|d| d.sort_key()), + RenderPhaseSortMode::Unstable => self.items.sort_unstable_by_key(|d| d.sort_key()), + RenderPhaseSortMode::Unsorted => {}, + } } } @@ -98,6 +102,10 @@ mod tests { fn draw_function(&self) -> DrawFunctionId { unimplemented!(); } + + fn sort_mode() -> RenderPhaseSortMode { + RenderPhaseSortMode::Stable + } } impl EntityPhaseItem for TestPhaseItem { fn entity(&self) -> bevy_ecs::entity::Entity { From 2ef7a7c2c45276a92e5e54be72a5ef44a5e6c885 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 19 Jun 2022 01:18:46 -0700 Subject: [PATCH 02/18] Fix CI --- crates/bevy_core_pipeline/src/core_2d/mod.rs | 3 ++- crates/bevy_render/src/render_phase/draw.rs | 3 +-- crates/bevy_render/src/render_phase/mod.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_core_pipeline/src/core_2d/mod.rs b/crates/bevy_core_pipeline/src/core_2d/mod.rs index 5ce696037fc26..6e5c7d4f67b33 100644 --- a/crates/bevy_core_pipeline/src/core_2d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_2d/mod.rs @@ -22,7 +22,8 @@ use bevy_render::{ render_graph::{RenderGraph, SlotInfo, SlotType}, render_phase::{ batch_phase_system, sort_phase_system, BatchedPhaseItem, CachedRenderPipelinePhaseItem, - RenderPhaseSortMode, DrawFunctionId, DrawFunctions, EntityPhaseItem, PhaseItem, RenderPhase, + DrawFunctionId, DrawFunctions, EntityPhaseItem, PhaseItem, RenderPhase, + RenderPhaseSortMode, }, render_resource::CachedRenderPipelineId, RenderApp, RenderStage, diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 52c44dda988ec..cb1675af614a0 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -39,7 +39,6 @@ pub enum RenderPhaseSortMode { Unsorted, } - /// An item which will be drawn to the screen. A phase item should be queued up for rendering /// during the [`RenderStage::Queue`](crate::RenderStage::Queue) stage. /// Afterwards it will be sorted and rendered automatically in the @@ -54,7 +53,7 @@ pub trait PhaseItem: Send + Sync + 'static { fn draw_function(&self) -> DrawFunctionId; /// Specifies whether the phase requires batching. This should return true if and only if - /// the type implements [`BatchPhaseItem`]. + /// the type implements [`BatchedPhaseItem`]. #[inline] fn sort_mode() -> RenderPhaseSortMode { RenderPhaseSortMode::Unstable diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 7a4cc79585966..992a8b993b2f4 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -32,7 +32,7 @@ impl RenderPhase { match I::sort_mode() { RenderPhaseSortMode::Stable => self.items.sort_by_key(|d| d.sort_key()), RenderPhaseSortMode::Unstable => self.items.sort_unstable_by_key(|d| d.sort_key()), - RenderPhaseSortMode::Unsorted => {}, + RenderPhaseSortMode::Unsorted => {} } } } From a725852f106ca0084b6661e3f6b0d167cfeb5025 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 20 Jun 2022 23:01:22 -0700 Subject: [PATCH 03/18] Update docs and review comments --- crates/bevy_core_pipeline/src/core_2d/mod.rs | 1 + crates/bevy_render/src/render_phase/draw.rs | 28 +++++++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/bevy_core_pipeline/src/core_2d/mod.rs b/crates/bevy_core_pipeline/src/core_2d/mod.rs index 6e5c7d4f67b33..e7d4eb20451cb 100644 --- a/crates/bevy_core_pipeline/src/core_2d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_2d/mod.rs @@ -92,6 +92,7 @@ impl PhaseItem for Transparent2d { self.draw_function } + #[inline] fn sort_mode() -> RenderPhaseSortMode { RenderPhaseSortMode::Stable } diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index cb1675af614a0..b80889f00d149 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -30,6 +30,7 @@ pub trait Draw: Send + Sync + 'static { ); } +/// Configures how a render phase is sorted. For more information, see [`PhaseItem::sort_mode`]. pub enum RenderPhaseSortMode { /// Requires a stable sort. Generally required for proper batching based on external criteria. Stable, @@ -39,6 +40,12 @@ pub enum RenderPhaseSortMode { Unsorted, } +impl Default for RenderPhaseSortMode { + fn default() -> Self { + Self::Unstable + } +} + /// An item which will be drawn to the screen. A phase item should be queued up for rendering /// during the [`RenderStage::Queue`](crate::RenderStage::Queue) stage. /// Afterwards it will be sorted and rendered automatically in the @@ -52,11 +59,22 @@ pub trait PhaseItem: Send + Sync + 'static { /// Specifies the [`Draw`] function used to render the item. fn draw_function(&self) -> DrawFunctionId; - /// Specifies whether the phase requires batching. This should return true if and only if - /// the type implements [`BatchedPhaseItem`]. + /// Specifies what kind of sort to apply to the phase. Generally if the same type + /// implements [`BatchedPhaseItem`], this should return [`RenderPhaseSortMode::Stable`]. + /// In almost all other cases, this should not be altered from the default, + /// [`RenderPhaseSortMode::Unstable`], as this provides the best balance of CPU and GPU + /// performance. + /// + /// It's generally only advised to use [`RenderPhaseSortMode::Unsorted`] if and only if + /// the renderer supports a depth prepass, which is by default not supported by the rest + /// of Bevy's first party rendering crates. Even then, this may have a negative impact + /// on GPU-side perf due to overdraw. + /// + /// It's advised to always profile for performance changes when changing this to + /// looser values. #[inline] fn sort_mode() -> RenderPhaseSortMode { - RenderPhaseSortMode::Unstable + Default::default() } } @@ -186,6 +204,10 @@ pub trait CachedRenderPipelinePhaseItem: PhaseItem { /// /// Batching is an optimization that regroups multiple items in the same vertex buffer /// to render them in a single draw call. +/// +/// If this is implemented on a type, the implementation of [`PhaseItem::sort_mode`] should +/// be changed to return [`RenderPhaseSortMode::Stable`], or incorrect/suboptimal batching +/// may result. pub trait BatchedPhaseItem: EntityPhaseItem { /// Range in the vertex buffer of this item fn batch_range(&self) -> &Option>; From 4ab57180f8824b882e82aada4a0bf5153b4e1904 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 21 Jun 2022 00:11:54 -0700 Subject: [PATCH 04/18] Review comments --- crates/bevy_render/src/render_phase/draw.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index b80889f00d149..4705f9615f39f 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -68,10 +68,10 @@ pub trait PhaseItem: Send + Sync + 'static { /// It's generally only advised to use [`RenderPhaseSortMode::Unsorted`] if and only if /// the renderer supports a depth prepass, which is by default not supported by the rest /// of Bevy's first party rendering crates. Even then, this may have a negative impact - /// on GPU-side perf due to overdraw. + /// on GPU-side performance due to overdraw. /// /// It's advised to always profile for performance changes when changing this to - /// looser values. + /// a different value. #[inline] fn sort_mode() -> RenderPhaseSortMode { Default::default() From 00dd07360dba80a97b0acb754c168a60a8c390ad Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 21 Jun 2022 00:43:00 -0700 Subject: [PATCH 05/18] Review comments --- crates/bevy_render/src/render_phase/draw.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 4705f9615f39f..3aad4167ed81b 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -33,6 +33,8 @@ pub trait Draw: Send + Sync + 'static { /// Configures how a render phase is sorted. For more information, see [`PhaseItem::sort_mode`]. pub enum RenderPhaseSortMode { /// Requires a stable sort. Generally required for proper batching based on external criteria. + /// Specifically when the sorting result is draw order dependent, typically required for + /// transparency that do not use a depth buffer. Stable, /// The default: allows unstable sorting. Usually faster than Stable. Unstable, From 9bc29a0de7bc8659fa6c2ac451896141cf536458 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 21 Jun 2022 04:16:35 -0700 Subject: [PATCH 06/18] Update doc comment. Co-authored-by: Robert Swain --- crates/bevy_render/src/render_phase/draw.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 3aad4167ed81b..7f54a8e720b98 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -32,9 +32,10 @@ pub trait Draw: Send + Sync + 'static { /// Configures how a render phase is sorted. For more information, see [`PhaseItem::sort_mode`]. pub enum RenderPhaseSortMode { - /// Requires a stable sort. Generally required for proper batching based on external criteria. - /// Specifically when the sorting result is draw order dependent, typically required for - /// transparency that do not use a depth buffer. + /// Use a stable sort that preserves the order of items with the same sort key. This is + /// required for proper batching based on external criteria, and for stability in + /// order-dependent transparency, or other drawing techniques where draw order defines the + /// final appearance, including those that do not use a depth buffer. Stable, /// The default: allows unstable sorting. Usually faster than Stable. Unstable, From 54deac228e43e4742f5966431f948493652364f4 Mon Sep 17 00:00:00 2001 From: Federico Rinaldi Date: Tue, 21 Jun 2022 15:29:22 +0000 Subject: [PATCH 07/18] Improve entity and component API docs (#4767) # Objective The descriptions included in the API docs of `entity` module, `Entity` struct, and `Component` trait have some issues: 1. the concept of entity is not clearly defined, 2. descriptions are a little bit out of place, 3. in a case the description leak too many details about the implementation, 4. some descriptions are not exhaustive, 5. there are not enough examples, 6. the content can be formatted in a much better way. ## Solution 1. ~~Stress the fact that entity is an abstract and elementary concept. Abstract because the concept of entity is not hardcoded into the library but emerges from the interaction of `Entity` with every other part of `bevy_ecs`, like components and world methods. Elementary because it is a fundamental concept that cannot be defined with other terms (like point in euclidean geometry, or time in classical physics).~~ We decided to omit the definition of entity in the API docs ([see why]). It is only described in its relationship with components. 2. Information has been moved to relevant places and links are used instead in the other places. 3. Implementation details about `Entity` have been reduced. 4. Descriptions have been made more exhaustive by stating how to obtain and use items. Entity operations are enriched with `World` methods. 5. Examples have been added or enriched. 6. Sections have been added to organize content. Entity operations are now laid out in a table. ### Todo list - [x] Break lines at sentence-level. ## For reviewers - ~~I added a TODO over `Component` docs, make sure to check it out and discuss it if necessary.~~ ([Resolved]) - You can easily check the rendered documentation by doing `cargo doc -p bevy_ecs --no-deps --open`. [see why]: https://github.com/bevyengine/bevy/pull/4767#discussion_r875106329 [Resolved]: https://github.com/bevyengine/bevy/pull/4767#discussion_r874127825 --- crates/bevy_ecs/src/component.rs | 98 ++++++++++++++++++++++++---- crates/bevy_ecs/src/entity/mod.rs | 103 ++++++++++++++++++++++-------- 2 files changed, 165 insertions(+), 36 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index fdcb2a0d0d974..81246e186ce60 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -14,26 +14,102 @@ use std::{ mem::needs_drop, }; -/// A component is data associated with an [`Entity`](crate::entity::Entity). Each entity can have -/// multiple different types of components, but only one of them per type. +/// A data type that can be used to store data for an [entity]. /// -/// Any type that is `Send + Sync + 'static` can implement `Component` using `#[derive(Component)]`. /// -/// In order to use foreign types as components, wrap them using a newtype pattern. +/// `Component` is a [derivable trait]: this means that a data type can implement it by applying a `#[derive(Component)]` attribute to it. +/// However, components must always satisfy the `Send + Sync + 'static` trait bounds. +/// +/// [entity]: crate::entity +/// [derivable trait]: https://doc.rust-lang.org/book/appendix-03-derivable-traits.html +/// +/// # Examples +/// +/// Components can take many forms: they are usually structs, but can also be of every other kind of data type, like enums or zero sized types. +/// The following examples show how components are laid out in code. +/// +/// ``` +/// # use bevy_ecs::component::Component; +/// # struct Color; +/// # +/// // A component can contain data... +/// #[derive(Component)] +/// struct LicensePlate(String); +/// +/// // ... but it can also be a zero-sized marker. +/// #[derive(Component)] +/// struct Car; +/// +/// // Components can also be structs with named fields... +/// #[derive(Component)] +/// struct VehiclePerformance { +/// acceleration: f32, +/// top_speed: f32, +/// handling: f32, +/// } +/// +/// // ... or enums. +/// #[derive(Component)] +/// enum WheelCount { +/// Two, +/// Three, +/// Four, +/// } +/// ``` +/// +/// # Component and data access +/// +/// See the [`entity`] module level documentation to learn how to add or remove components from an entity. +/// +/// See the documentation for [`Query`] to learn how to access component data from a system. +/// +/// [`entity`]: crate::entity#usage +/// [`Query`]: crate::system::Query +/// +/// # Choosing a storage type +/// +/// Components can be stored in the world using different strategies with their own performance implications. +/// By default, components are added to the [`Table`] storage, which is optimized for query iteration. +/// +/// Alternatively, components can be added to the [`SparseSet`] storage, which is optimized for component insertion and removal. +/// This is achieved by adding an additional `#[component(storage = "SparseSet")]` attribute to the derive one: +/// /// ``` /// # use bevy_ecs::component::Component; +/// # +/// #[derive(Component)] +/// #[component(storage = "SparseSet")] +/// struct ComponentA; +/// ``` +/// +/// [`Table`]: crate::storage::Table +/// [`SparseSet`]: crate::storage::SparseSet +/// +/// # Implementing the trait for foreign types +/// +/// As a consequence of the [orphan rule], it is not possible to separate into two different crates the implementation of `Component` from the definition of a type. +/// This means that it is not possible to directly have a type defined in a third party library as a component. +/// This important limitation can be easily worked around using the [newtype pattern]: +/// this makes it possible to locally define and implement `Component` for a tuple struct that wraps the foreign type. +/// The following example gives a demonstration of this pattern. +/// +/// ``` +/// // `Component` is defined in the `bevy_ecs` crate. +/// use bevy_ecs::component::Component; +/// +/// // `Duration` is defined in the `std` crate. /// use std::time::Duration; +/// +/// // It is not possible to implement `Component` for `Duration` from this position, as they are +/// // both foreign items, defined in an external crate. However, nothing prevents to define a new +/// // `Cooldown` type that wraps `Duration`. As `Cooldown` is defined in a local crate, it is +/// // possible to implement `Component` for it. /// #[derive(Component)] /// struct Cooldown(Duration); /// ``` -/// Components are added with new entities using [`Commands::spawn`](crate::system::Commands::spawn), -/// or to existing entities with [`EntityCommands::insert`](crate::system::EntityCommands::insert), -/// or their [`World`](crate::world::World) equivalents. -/// -/// Components can be accessed in systems by using a [`Query`](crate::system::Query) -/// as one of the arguments. /// -/// Components can be grouped together into a [`Bundle`](crate::bundle::Bundle). +/// [orphan rule]: https://doc.rust-lang.org/book/ch10-02-traits.html#implementing-a-trait-on-a-type +/// [newtype pattern]: https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#using-the-newtype-pattern-to-implement-external-traits-on-external-types pub trait Component: Send + Sync + 'static { type Storage: ComponentStorage; } diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index abfd3513ea50e..dcad2664f68cf 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -1,27 +1,38 @@ //! Entity handling types. //! -//! In Bevy ECS, there is no monolithic data structure for an entity. Instead, the [`Entity`] -//! `struct` is just a *generational index* (a combination of an ID and a generation). Then, -//! the `Entity` maps to the specific [`Component`s](crate::component::Component). This way, -//! entities can have meaningful data attached to it. This is a fundamental design choice -//! that has been taken to enhance performance and usability. +//! An **entity** exclusively owns zero or more [component] instances, all of different types, and can dynamically acquire or lose them over its lifetime. +//! +//! See [`Entity`] to learn more. +//! +//! [component]: crate::component::Component //! //! # Usage //! -//! Here are links to the methods used to perform common operations -//! involving entities: +//! Operations involving entities and their components are performed either from a system by submitting commands, +//! or from the outside (or from an exclusive system) by directly using [`World`] methods: +//! +//! |Operation|Command|Method| +//! |:---:|:---:|:---:| +//! |Spawn a new entity|[`Commands::spawn`]|[`World::spawn`]| +//! |Spawn an entity with components|[`Commands::spawn_bundle`]|---| +//! |Despawn an entity|[`EntityCommands::despawn`]|[`World::despawn`]| +//! |Insert a component to an entity|[`EntityCommands::insert`]|[`EntityMut::insert`]| +//! |Insert multiple components to an entity|[`EntityCommands::insert_bundle`]|[`EntityMut::insert_bundle`]| +//! |Remove a component from an entity|[`EntityCommands::remove`]|[`EntityMut::remove`]| //! -//! - **Spawning an empty entity:** use [`Commands::spawn`](crate::system::Commands::spawn). -//! - **Spawning an entity with components:** use -//! [`Commands::spawn_bundle`](crate::system::Commands::spawn_bundle). -//! - **Despawning an entity:** use -//! [`EntityCommands::despawn`](crate::system::EntityCommands::despawn). -//! - **Inserting a component to an entity:** use -//! [`EntityCommands::insert`](crate::system::EntityCommands::insert). -//! - **Adding multiple components to an entity:** use -//! [`EntityCommands::insert_bundle`](crate::system::EntityCommands::insert_bundle). -//! - **Removing a component to an entity:** use -//! [`EntityCommands::remove`](crate::system::EntityCommands::remove). +//! [`World`]: crate::world::World +//! [`Commands::spawn`]: crate::system::Commands::spawn +//! [`Commands::spawn_bundle`]: crate::system::Commands::spawn_bundle +//! [`EntityCommands::despawn`]: crate::system::EntityCommands::despawn +//! [`EntityCommands::insert`]: crate::system::EntityCommands::insert +//! [`EntityCommands::insert_bundle`]: crate::system::EntityCommands::insert_bundle +//! [`EntityCommands::remove`]: crate::system::EntityCommands::remove +//! [`World::spawn`]: crate::world::World::spawn +//! [`World::spawn_bundle`]: crate::world::World::spawn_bundle +//! [`World::despawn`]: crate::world::World::despawn +//! [`EntityMut::insert`]: crate::world::EntityMut::insert +//! [`EntityMut::insert_bundle`]: crate::world::EntityMut::insert_bundle +//! [`EntityMut::remove`]: crate::world::EntityMut::remove mod map_entities; mod serde; @@ -35,15 +46,57 @@ use std::{ sync::atomic::{AtomicI64, Ordering}, }; -/// Lightweight unique ID of an entity. +/// Lightweight identifier of an [entity](crate::entity). +/// +/// The identifier is implemented using a [generational index]: a combination of an ID and a generation. +/// This allows fast insertion after data removal in an array while minimizing loss of spatial locality. +/// +/// [generational index]: https://lucassardois.medium.com/generational-indices-guide-8e3c5f7fd594 +/// +/// # Usage +/// +/// This data type is returned by iterating a `Query` that has `Entity` as part of its query fetch type parameter ([learn more]). +/// It can also be obtained by calling [`EntityCommands::id`] or [`EntityMut::id`]. +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// # +/// fn setup(mut commands: Commands) { +/// // Calling `spawn` returns `EntityCommands`. +/// let entity = commands.spawn().id(); +/// } +/// +/// fn exclusive_system(world: &mut World) { +/// // Calling `spawn` returns `EntityMut`. +/// let entity = world.spawn().id(); +/// } +/// # +/// # bevy_ecs::system::assert_is_system(setup); +/// # bevy_ecs::system::IntoExclusiveSystem::exclusive_system(exclusive_system); +/// ``` +/// +/// It can be used to refer to a specific entity to apply [`EntityCommands`], or to call [`Query::get`] (or similar methods) to access its components. /// -/// Obtained from [`World::spawn`](crate::world::World::spawn), typically via -/// [`Commands::spawn`](crate::system::Commands::spawn). Can be stored to refer to an entity in the -/// future. +/// ``` +/// # use bevy_ecs::prelude::*; +/// # +/// # #[derive(Component)] +/// # struct Expired; +/// # +/// fn dispose_expired_food(mut commands: Commands, query: Query>) { +/// for food_entity in query.iter() { +/// commands.entity(food_entity).despawn(); +/// } +/// } +/// # +/// # bevy_ecs::system::assert_is_system(dispose_expired_food); +/// ``` /// -/// `Entity` can be a part of a query, e.g. `Query<(Entity, &MyComponent)>`. -/// Components of a specific entity can be accessed using -/// [`Query::get`](crate::system::Query::get) and related methods. +/// [learn more]: crate::system::Query#entity-id-access +/// [`EntityCommands::id`]: crate::system::EntityCommands::id +/// [`EntityMut::id`]: crate::world::EntityMut::id +/// [`EntityCommands`]: crate::system::EntityCommands +/// [`Query::get`]: crate::system::Query::get #[derive(Clone, Copy, Hash, Eq, Ord, PartialEq, PartialOrd)] pub struct Entity { pub(crate) generation: u32, From cbda1cb449492ae13a67187e947401304f914066 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 21 Jun 2022 18:10:27 +0000 Subject: [PATCH 08/18] Change check_visibility to use thread-local queues instead of a channel (#4663) # Objective Further speed up visibility checking by removing the main sources of contention for the system. ## Solution - ~~Make `ComputedVisibility` a resource wrapping a `FixedBitset`.~~ - ~~Remove `ComputedVisibility` as a component.~~ ~~This adds a one-bit overhead to every entity in the app world. For a game with 100,000 entities, this is 12.5KB of memory. This is still small enough to fit entirely in most L1 caches. Also removes the need for a per-Entity change detection tick. This reduces the memory footprint of ComputedVisibility 72x.~~ ~~The decreased memory usage and less fragmented memory locality should provide significant performance benefits.~~ ~~Clearing visible entities should be significantly faster than before:~~ - ~~Setting one `u32` to 0 clears 32 entities per cycle.~~ - ~~No archetype fragmentation to contend with.~~ - ~~Change detection is applied to the resource, so there is no per-Entity update tick requirement.~~ ~~The side benefit of this design is that it removes one more "computed component" from userspace. Though accessing the values within it are now less ergonomic.~~ This PR changes `crossbeam_channel` in `check_visibility` to use a `Local>>` to mark down visible entities instead. Co-Authored-By: TheRawMeatball Co-Authored-By: Aevyrie --- crates/bevy_render/Cargo.toml | 2 +- crates/bevy_render/src/view/visibility/mod.rs | 46 ++++++++++++------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/crates/bevy_render/Cargo.toml b/crates/bevy_render/Cargo.toml index fc9796e83f83f..879dbbbf9af9d 100644 --- a/crates/bevy_render/Cargo.toml +++ b/crates/bevy_render/Cargo.toml @@ -54,9 +54,9 @@ bitflags = "1.2.1" smallvec = { version = "1.6", features = ["union", "const_generics"] } once_cell = "1.4.1" # TODO: replace once_cell with std equivalent if/when this lands: https://github.com/rust-lang/rfcs/pull/2788 downcast-rs = "1.2.0" +thread_local = "1.1" thiserror = "1.0" futures-lite = "1.4.0" -crossbeam-channel = "0.5.0" anyhow = "1.0" hex = "0.4.2" hexasphere = "7.0.0" diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 4f8a456486a19..795a860bd0e49 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -10,6 +10,8 @@ use bevy_reflect::std_traits::ReflectDefault; use bevy_reflect::Reflect; use bevy_transform::components::GlobalTransform; use bevy_transform::TransformSystem; +use std::cell::Cell; +use thread_local::ThreadLocal; use crate::{ camera::{Camera, CameraProjection, OrthographicProjection, PerspectiveProjection, Projection}, @@ -148,22 +150,30 @@ pub fn update_frusta( } pub fn check_visibility( + mut thread_queues: Local>>>, mut view_query: Query<(&mut VisibleEntities, &Frustum, Option<&RenderLayers>), With>, - mut visible_entity_query: Query<( - Entity, - &Visibility, - &mut ComputedVisibility, - Option<&RenderLayers>, - Option<&Aabb>, - Option<&NoFrustumCulling>, - Option<&GlobalTransform>, + mut visible_entity_query: ParamSet<( + Query<&mut ComputedVisibility>, + Query<( + Entity, + &Visibility, + &mut ComputedVisibility, + Option<&RenderLayers>, + Option<&Aabb>, + Option<&NoFrustumCulling>, + Option<&GlobalTransform>, + )>, )>, ) { + // Reset the computed visibility to false + for mut computed_visibility in visible_entity_query.p0().iter_mut() { + computed_visibility.is_visible = false; + } + for (mut visible_entities, frustum, maybe_view_mask) in view_query.iter_mut() { let view_mask = maybe_view_mask.copied().unwrap_or_default(); - let (visible_entity_sender, visible_entity_receiver) = crossbeam_channel::unbounded(); - - visible_entity_query.par_for_each_mut( + visible_entities.entities.clear(); + visible_entity_query.p1().par_for_each_mut( 1024, |( entity, @@ -174,12 +184,10 @@ pub fn check_visibility( maybe_no_frustum_culling, maybe_transform, )| { - // Reset visibility - computed_visibility.is_visible = false; - if !visibility.is_visible { return; } + let entity_mask = maybe_entity_mask.copied().unwrap_or_default(); if !view_mask.intersects(&entity_mask) { return; @@ -205,9 +213,15 @@ pub fn check_visibility( } computed_visibility.is_visible = true; - visible_entity_sender.send(entity).ok(); + let cell = thread_queues.get_or_default(); + let mut queue = cell.take(); + queue.push(entity); + cell.set(queue); }, ); - visible_entities.entities = visible_entity_receiver.try_iter().collect(); + + for cell in thread_queues.iter_mut() { + visible_entities.entities.append(cell.get_mut()); + } } } From 69d8da719fc6e47915da29ff5aaa7687246ceaa6 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 21 Jun 2022 20:35:26 +0000 Subject: [PATCH 09/18] Mark mutable APIs under ECS storage as pub(crate) (#5065) # Objective Closes #1557. Partially addresses #3362. Cleanup the public facing API for storage types. Most of these APIs are difficult to use safely when directly interfacing with these types, and is also currently impossible to interact with in normal ECS use as there is no `World::storages_mut`. The majority of these types should be easy enough to read, and perhaps mutate the contents, but never structurally altered without the same checks in the rest of bevy_ecs code. This both cleans up the public facing types and helps use unused code detection to remove a few of the APIs we're not using internally. ## Solution - Mark all APIs that take `&mut T` under `bevy_ecs::storage` as `pub(crate)` or `pub(super)` - Cleanup after it all. Entire type visibility changes: - `BlobVec` is `pub(super)`, only storage code should be directly interacting with it. - `SparseArray` is now `pub(crate)` for the entire type. It's an implementation detail for `Table` and `(Component)SparseSet`. - `TableMoveResult` is now `pub(crate) --- ## Changelog TODO ## Migration Guide Dear God, I hope not. --- crates/bevy_ecs/src/archetype.rs | 51 ++++++++----- crates/bevy_ecs/src/storage/blob_vec.rs | 2 +- crates/bevy_ecs/src/storage/mod.rs | 1 - crates/bevy_ecs/src/storage/sparse_set.rs | 53 ++----------- crates/bevy_ecs/src/storage/table.rs | 92 +++++++++++------------ crates/bevy_ecs/src/world/mod.rs | 2 +- 6 files changed, 82 insertions(+), 119 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index ab33119a93be6..2c01f8c1bb4b2 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -32,21 +32,35 @@ impl ArchetypeId { } } -pub enum ComponentStatus { +pub(crate) enum ComponentStatus { Added, Mutated, } pub struct AddBundle { pub archetype_id: ArchetypeId, - pub bundle_status: Vec, + pub(crate) bundle_status: Vec, } +/// Archetypes and bundles form a graph. Adding or removing a bundle moves +/// an [`Entity`] to a new [`Archetype`]. +/// +/// [`Edges`] caches the results of these moves. Each archetype caches +/// the result of a structural alteration. This can be used to monitor the +/// state of the archetype graph. +/// +/// Note: This type only contains edges the [`World`] has already traversed. +/// If any of functions return `None`, it doesn't mean there is guarenteed +/// not to be a result of adding or removing that bundle, but rather that +/// operation that has moved an entity along that edge has not been performed +/// yet. +/// +/// [`World`]: crate::world::World #[derive(Default)] pub struct Edges { - pub add_bundle: SparseArray, - pub remove_bundle: SparseArray>, - pub remove_bundle_intersection: SparseArray>, + add_bundle: SparseArray, + remove_bundle: SparseArray>, + remove_bundle_intersection: SparseArray>, } impl Edges { @@ -56,7 +70,7 @@ impl Edges { } #[inline] - pub fn insert_add_bundle( + pub(crate) fn insert_add_bundle( &mut self, bundle_id: BundleId, archetype_id: ArchetypeId, @@ -77,7 +91,11 @@ impl Edges { } #[inline] - pub fn insert_remove_bundle(&mut self, bundle_id: BundleId, archetype_id: Option) { + pub(crate) fn insert_remove_bundle( + &mut self, + bundle_id: BundleId, + archetype_id: Option, + ) { self.remove_bundle.insert(bundle_id, archetype_id); } @@ -90,7 +108,7 @@ impl Edges { } #[inline] - pub fn insert_remove_bundle_intersection( + pub(crate) fn insert_remove_bundle_intersection( &mut self, bundle_id: BundleId, archetype_id: Option, @@ -237,14 +255,14 @@ impl Archetype { } #[inline] - pub fn set_entity_table_row(&mut self, index: usize, table_row: usize) { + pub(crate) fn set_entity_table_row(&mut self, index: usize, table_row: usize) { self.table_info.entity_rows[index] = table_row; } /// # Safety /// valid component values must be immediately written to the relevant storages /// `table_row` must be valid - pub unsafe fn allocate(&mut self, entity: Entity, table_row: usize) -> EntityLocation { + pub(crate) unsafe fn allocate(&mut self, entity: Entity, table_row: usize) -> EntityLocation { self.entities.push(entity); self.table_info.entity_rows.push(table_row); @@ -254,7 +272,7 @@ impl Archetype { } } - pub fn reserve(&mut self, additional: usize) { + pub(crate) fn reserve(&mut self, additional: usize) { self.entities.reserve(additional); self.table_info.entity_rows.reserve(additional); } @@ -407,7 +425,7 @@ impl Archetypes { } #[inline] - pub fn empty_mut(&mut self) -> &mut Archetype { + pub(crate) fn empty_mut(&mut self) -> &mut Archetype { // SAFE: empty archetype always exists unsafe { self.archetypes @@ -422,7 +440,7 @@ impl Archetypes { } #[inline] - pub fn resource_mut(&mut self) -> &mut Archetype { + pub(crate) fn resource_mut(&mut self) -> &mut Archetype { // SAFE: resource archetype always exists unsafe { self.archetypes @@ -440,11 +458,6 @@ impl Archetypes { self.archetypes.get(id.index()) } - #[inline] - pub fn get_mut(&mut self, id: ArchetypeId) -> Option<&mut Archetype> { - self.archetypes.get_mut(id.index()) - } - #[inline] pub(crate) fn get_2_mut( &mut self, @@ -518,7 +531,7 @@ impl Archetypes { self.archetype_component_count } - pub fn clear_entities(&mut self) { + pub(crate) fn clear_entities(&mut self) { for archetype in &mut self.archetypes { archetype.clear_entities(); } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 7a3991d35e48e..8ce77d73a64a7 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -9,7 +9,7 @@ use bevy_ptr::{OwningPtr, Ptr, PtrMut}; /// A flat, type-erased data storage type /// /// Used to densely store homogeneous ECS data. -pub struct BlobVec { +pub(super) struct BlobVec { item_layout: Layout, capacity: usize, /// Number of elements, not bytes diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index f54a2275bfe07..571f05184cdbd 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -4,7 +4,6 @@ mod blob_vec; mod sparse_set; mod table; -pub use blob_vec::*; pub use sparse_set::*; pub use table::*; diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index f62999a2a9d70..1d5172b64a624 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -9,7 +9,7 @@ use std::{cell::UnsafeCell, hash::Hash, marker::PhantomData}; type EntityId = u32; #[derive(Debug)] -pub struct SparseArray { +pub(crate) struct SparseArray { values: Vec>, marker: PhantomData, } @@ -31,13 +31,6 @@ impl SparseArray { } impl SparseArray { - pub fn with_capacity(capacity: usize) -> Self { - Self { - values: Vec::with_capacity(capacity), - marker: PhantomData, - } - } - #[inline] pub fn insert(&mut self, index: I, value: V) { let index = index.sparse_set_index(); @@ -74,18 +67,6 @@ impl SparseArray { self.values.get_mut(index).and_then(|value| value.take()) } - #[inline] - pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { - let index = index.sparse_set_index(); - if index < self.values.len() { - return self.values[index].get_or_insert_with(func); - } - self.values.resize_with(index + 1, || None); - let value = &mut self.values[index]; - *value = Some(func()); - value.as_mut().unwrap() - } - pub fn clear(&mut self) { self.values.clear(); } @@ -108,7 +89,7 @@ pub struct ComponentSparseSet { } impl ComponentSparseSet { - pub fn new(component_info: &ComponentInfo, capacity: usize) -> Self { + pub(crate) fn new(component_info: &ComponentInfo, capacity: usize) -> Self { Self { dense: Column::with_capacity(component_info, capacity), entities: Vec::with_capacity(capacity), @@ -116,7 +97,7 @@ impl ComponentSparseSet { } } - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { self.dense.clear(); self.entities.clear(); self.sparse.clear(); @@ -138,7 +119,7 @@ impl ComponentSparseSet { /// # Safety /// The `value` pointer must point to a valid address that matches the [`Layout`](std::alloc::Layout) /// inside the [`ComponentInfo`] given when constructing this sparse set. - pub unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { + pub(crate) unsafe fn insert(&mut self, entity: Entity, value: OwningPtr<'_>, change_tick: u32) { if let Some(&dense_index) = self.sparse.get(entity.id()) { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index as usize]); @@ -209,7 +190,7 @@ impl ComponentSparseSet { /// Removes the `entity` from this sparse set and returns a pointer to the associated value (if /// it exists). #[must_use = "The returned pointer must be used to drop the removed component."] - pub fn remove_and_forget(&mut self, entity: Entity) -> Option> { + pub(crate) fn remove_and_forget(&mut self, entity: Entity) -> Option> { self.sparse.remove(entity.id()).map(|dense_index| { let dense_index = dense_index as usize; #[cfg(debug_assertions)] @@ -230,7 +211,7 @@ impl ComponentSparseSet { }) } - pub fn remove(&mut self, entity: Entity) -> bool { + pub(crate) fn remove(&mut self, entity: Entity) -> bool { if let Some(dense_index) = self.sparse.remove(entity.id()) { let dense_index = dense_index as usize; #[cfg(debug_assertions)] @@ -308,28 +289,6 @@ impl SparseSet { self.indices.push(index); self.dense.push(value); } - - // PERF: switch to this. it's faster but it has an invalid memory access on - // table_add_remove_many let dense = &mut self.dense; - // let indices = &mut self.indices; - // let dense_index = *self.sparse.get_or_insert_with(index.clone(), move || { - // if dense.len() == dense.capacity() { - // dense.reserve(64); - // indices.reserve(64); - // } - // let len = dense.len(); - // // SAFE: we set the index immediately - // unsafe { - // dense.set_len(len + 1); - // indices.set_len(len + 1); - // } - // len - // }); - // // SAFE: index either already existed or was just allocated - // unsafe { - // *self.dense.get_unchecked_mut(dense_index) = value; - // *self.indices.get_unchecked_mut(dense_index) = index; - // } } pub fn get_or_insert_with(&mut self, index: I, func: impl FnOnce() -> V) -> &mut V { diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 6150c679c88a9..37ee9498656c0 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -1,10 +1,11 @@ use crate::{ component::{ComponentId, ComponentInfo, ComponentTicks, Components}, entity::Entity, - storage::{BlobVec, SparseSet}, + storage::{blob_vec::BlobVec, SparseSet}, }; use bevy_ptr::{OwningPtr, Ptr, PtrMut}; use bevy_utils::HashMap; +use std::alloc::Layout; use std::{ cell::UnsafeCell, ops::{Index, IndexMut}, @@ -32,13 +33,13 @@ impl TableId { #[derive(Debug)] pub struct Column { - pub(crate) data: BlobVec, - pub(crate) ticks: Vec>, + data: BlobVec, + ticks: Vec>, } impl Column { #[inline] - pub fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { + pub(crate) fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { Column { // SAFE: component_info.drop() is valid for the types that will be inserted. data: unsafe { BlobVec::new(component_info.layout(), component_info.drop(), capacity) }, @@ -46,6 +47,11 @@ impl Column { } } + #[inline] + pub fn item_layout(&self) -> Layout { + self.data.layout() + } + /// Writes component data to the column at given row. /// Assumes the slot is uninitialized, drop is not called. /// To overwrite existing initialized value, use `replace` instead. @@ -53,7 +59,12 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn initialize(&mut self, row: usize, data: OwningPtr<'_>, ticks: ComponentTicks) { + pub(crate) unsafe fn initialize( + &mut self, + row: usize, + data: OwningPtr<'_>, + ticks: ComponentTicks, + ) { debug_assert!(row < self.len()); self.data.initialize_unchecked(row, data); *self.ticks.get_unchecked_mut(row).get_mut() = ticks; @@ -65,7 +76,7 @@ impl Column { /// # Safety /// Assumes data has already been allocated for the given row. #[inline] - pub unsafe fn replace(&mut self, row: usize, data: OwningPtr<'_>, change_tick: u32) { + pub(crate) unsafe fn replace(&mut self, row: usize, data: OwningPtr<'_>, change_tick: u32) { debug_assert!(row < self.len()); self.data.replace_unchecked(row, data); self.ticks @@ -74,14 +85,6 @@ impl Column { .set_changed(change_tick); } - /// # Safety - /// Assumes data has already been allocated for the given row. - #[inline] - pub unsafe fn initialize_data(&mut self, row: usize, data: OwningPtr<'_>) { - debug_assert!(row < self.len()); - self.data.initialize_unchecked(row, data); - } - #[inline] pub fn len(&self) -> usize { self.data.len() @@ -95,7 +98,7 @@ impl Column { /// # Safety /// index must be in-bounds #[inline] - pub unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { + pub(crate) unsafe fn get_ticks_unchecked_mut(&mut self, row: usize) -> &mut ComponentTicks { debug_assert!(row < self.len()); self.ticks.get_unchecked_mut(row).get_mut() } @@ -161,7 +164,7 @@ impl Column { /// - index must be in-bounds /// - no other reference to the data of the same row can exist at the same time #[inline] - pub unsafe fn get_data_unchecked_mut(&mut self, row: usize) -> PtrMut<'_> { + pub(crate) unsafe fn get_data_unchecked_mut(&mut self, row: usize) -> PtrMut<'_> { debug_assert!(row < self.data.len()); self.data.get_unchecked_mut(row) } @@ -193,14 +196,7 @@ pub struct Table { } impl Table { - pub const fn new() -> Table { - Self { - columns: SparseSet::new(), - entities: Vec::new(), - } - } - - pub fn with_capacity(capacity: usize, column_capacity: usize) -> Table { + pub(crate) fn with_capacity(capacity: usize, column_capacity: usize) -> Table { Self { columns: SparseSet::with_capacity(column_capacity), entities: Vec::with_capacity(capacity), @@ -212,7 +208,7 @@ impl Table { &self.entities } - pub fn add_column(&mut self, component_info: &ComponentInfo) { + pub(crate) fn add_column(&mut self, component_info: &ComponentInfo) { self.columns.insert( component_info.id(), Column::with_capacity(component_info, self.entities.capacity()), @@ -224,7 +220,7 @@ impl Table { /// /// # Safety /// `row` must be in-bounds - pub unsafe fn swap_remove_unchecked(&mut self, row: usize) -> Option { + pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: usize) -> Option { for column in self.columns.values_mut() { column.swap_remove_unchecked(row); } @@ -244,7 +240,7 @@ impl Table { /// /// # Safety /// Row must be in-bounds - pub unsafe fn move_to_and_forget_missing_unchecked( + pub(crate) unsafe fn move_to_and_forget_missing_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -274,7 +270,7 @@ impl Table { /// /// # Safety /// row must be in-bounds - pub unsafe fn move_to_and_drop_missing_unchecked( + pub(crate) unsafe fn move_to_and_drop_missing_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -306,7 +302,7 @@ impl Table { /// /// # Safety /// `row` must be in-bounds. `new_table` must contain every component this table has - pub unsafe fn move_to_superset_unchecked( + pub(crate) unsafe fn move_to_superset_unchecked( &mut self, row: usize, new_table: &mut Table, @@ -335,7 +331,7 @@ impl Table { } #[inline] - pub fn get_column_mut(&mut self, component_id: ComponentId) -> Option<&mut Column> { + pub(crate) fn get_column_mut(&mut self, component_id: ComponentId) -> Option<&mut Column> { self.columns.get_mut(component_id) } @@ -344,7 +340,7 @@ impl Table { self.columns.contains(component_id) } - pub fn reserve(&mut self, additional: usize) { + pub(crate) fn reserve(&mut self, additional: usize) { if self.entities.capacity() - self.entities.len() < additional { self.entities.reserve(additional); @@ -361,7 +357,7 @@ impl Table { /// /// # Safety /// the allocated row must be written to immediately with valid values in each column - pub unsafe fn allocate(&mut self, entity: Entity) -> usize { + pub(crate) unsafe fn allocate(&mut self, entity: Entity) -> usize { self.reserve(1); let index = self.entities.len(); self.entities.push(entity); @@ -397,7 +393,7 @@ impl Table { self.columns.values() } - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { self.entities.clear(); for column in self.columns.values_mut() { column.clear(); @@ -423,7 +419,7 @@ impl Default for Tables { } } -pub struct TableMoveResult { +pub(crate) struct TableMoveResult { pub swapped_entity: Option, pub new_row: usize, } @@ -444,11 +440,6 @@ impl Tables { self.tables.get(id.index()) } - #[inline] - pub fn get_mut(&mut self, id: TableId) -> Option<&mut Table> { - self.tables.get_mut(id.index()) - } - #[inline] pub(crate) fn get_2_mut(&mut self, a: TableId, b: TableId) -> (&mut Table, &mut Table) { if a.index() > b.index() { @@ -462,7 +453,7 @@ impl Tables { /// # Safety /// `component_ids` must contain components that exist in `components` - pub unsafe fn get_id_or_insert( + pub(crate) unsafe fn get_id_or_insert( &mut self, component_ids: &[ComponentId], components: &Components, @@ -488,11 +479,7 @@ impl Tables { self.tables.iter() } - pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, Table> { - self.tables.iter_mut() - } - - pub fn clear(&mut self) { + pub(crate) fn clear(&mut self) { for table in &mut self.tables { table.clear(); } @@ -527,7 +514,11 @@ mod tests { use crate::component::Component; use crate::ptr::OwningPtr; use crate::storage::Storages; - use crate::{component::Components, entity::Entity, storage::Table}; + use crate::{ + component::{ComponentTicks, Components}, + entity::Entity, + storage::Table, + }; #[derive(Component)] struct W(T); @@ -546,10 +537,11 @@ mod tests { let row = table.allocate(*entity); let value: W = W(row); OwningPtr::make(value, |value_ptr| { - table - .get_column_mut(component_id) - .unwrap() - .initialize_data(row, value_ptr); + table.get_column_mut(component_id).unwrap().initialize( + row, + value_ptr, + ComponentTicks::new(0), + ); }); }; } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 71ee4c7a3716b..b6e447e8dcd84 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1196,7 +1196,7 @@ impl World { std::ptr::copy_nonoverlapping::( value.as_ptr(), ptr.as_ptr(), - column.data.layout().size(), + column.item_layout().size(), ); column.get_ticks_unchecked_mut(0).set_changed(change_tick); } From f7a189ba5e5162c991bcae7bbc880797d7b7e03b Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Tue, 21 Jun 2022 20:50:06 +0000 Subject: [PATCH 10/18] Callable PBR functions (#4939) # Objective - Builds on top of #4938 - Make clustered-forward PBR lighting/shadows functionality callable - See #3969 for details ## Solution - Add `PbrInput` struct type containing a `StandardMaterial`, occlusion, world_position, world_normal, and frag_coord - Split functionality to calculate the unit view vector, and normal-mapped normal into `bevy_pbr::pbr_functions` - Split high-level shading flow into `pbr(in: PbrInput, N: vec3, V: vec3, is_orthographic: bool)` function in `bevy_pbr::pbr_functions` - Rework `pbr.wgsl` fragment stage entry point to make use of the new functions - This has been benchmarked on an M1 Max using `many_cubes -- sphere`. `main` had a median frame time of 15.88ms, this PR 15.99ms, which is a 0.69% frame time increase, which is within noise in my opinion. --- ## Changelog - Added: PBR shading code is now callable. Import `bevy_pbr::pbr_functions` and its dependencies, create a `PbrInput`, calculate the unit view and normal-mapped normal vectors and whether the projection is orthographic, and call `pbr()`! --- crates/bevy_pbr/src/lib.rs | 8 + crates/bevy_pbr/src/render/pbr.wgsl | 170 +++------------ crates/bevy_pbr/src/render/pbr_functions.wgsl | 196 ++++++++++++++++++ 3 files changed, 232 insertions(+), 142 deletions(-) create mode 100644 crates/bevy_pbr/src/render/pbr_functions.wgsl diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index c0b5668e1a761..e3ac3914512cb 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -64,6 +64,8 @@ pub const SHADOWS_HANDLE: HandleUntyped = HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 11350275143789590502); pub const PBR_SHADER_HANDLE: HandleUntyped = HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 4805239651767701046); +pub const PBR_FUNCTIONS_HANDLE: HandleUntyped = + HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 16550102964439850292); pub const SHADOW_SHADER_HANDLE: HandleUntyped = HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 1836745567947005696); @@ -104,6 +106,12 @@ impl Plugin for PbrPlugin { "render/shadows.wgsl", Shader::from_wgsl ); + load_internal_asset!( + app, + PBR_FUNCTIONS_HANDLE, + "render/pbr_functions.wgsl", + Shader::from_wgsl + ); load_internal_asset!(app, PBR_SHADER_HANDLE, "render/pbr.wgsl", Shader::from_wgsl); load_internal_asset!( app, diff --git a/crates/bevy_pbr/src/render/pbr.wgsl b/crates/bevy_pbr/src/render/pbr.wgsl index 7c7c2823265b5..3f169d5104f66 100644 --- a/crates/bevy_pbr/src/render/pbr.wgsl +++ b/crates/bevy_pbr/src/render/pbr.wgsl @@ -6,6 +6,7 @@ #import bevy_pbr::clustered_forward #import bevy_pbr::lighting #import bevy_pbr::shadows +#import bevy_pbr::pbr_functions struct FragmentInput { [[builtin(front_facing)]] is_front: bool; @@ -24,22 +25,31 @@ struct FragmentInput { [[stage(fragment)]] fn fragment(in: FragmentInput) -> [[location(0)]] vec4 { var output_color: vec4 = material.base_color; - #ifdef VERTEX_COLORS +#ifdef VERTEX_COLORS output_color = output_color * in.color; - #endif +#endif if ((material.flags & STANDARD_MATERIAL_FLAGS_BASE_COLOR_TEXTURE_BIT) != 0u) { output_color = output_color * textureSample(base_color_texture, base_color_sampler, in.uv); } - // // NOTE: Unlit bit not set means == 0 is true, so the true case is if lit + // NOTE: Unlit bit not set means == 0 is true, so the true case is if lit if ((material.flags & STANDARD_MATERIAL_FLAGS_UNLIT_BIT) == 0u) { + // Prepare a 'processed' StandardMaterial by sampling all textures to resolve + // the material members + var pbr_input: PbrInput; + + pbr_input.material.base_color = output_color; + pbr_input.material.reflectance = material.reflectance; + pbr_input.material.flags = material.flags; + pbr_input.material.alpha_cutoff = material.alpha_cutoff; + // TODO use .a for exposure compensation in HDR var emissive: vec4 = material.emissive; if ((material.flags & STANDARD_MATERIAL_FLAGS_EMISSIVE_TEXTURE_BIT) != 0u) { emissive = vec4(emissive.rgb * textureSample(emissive_texture, emissive_sampler, in.uv).rgb, 1.0); } + pbr_input.material.emissive = emissive; - // calculate non-linear roughness from linear perceptualRoughness var metallic: f32 = material.metallic; var perceptual_roughness: f32 = material.perceptual_roughness; if ((material.flags & STANDARD_MATERIAL_FLAGS_METALLIC_ROUGHNESS_TEXTURE_BIT) != 0u) { @@ -48,158 +58,34 @@ fn fragment(in: FragmentInput) -> [[location(0)]] vec4 { metallic = metallic * metallic_roughness.b; perceptual_roughness = perceptual_roughness * metallic_roughness.g; } - let roughness = perceptualRoughnessToRoughness(perceptual_roughness); + pbr_input.material.metallic = metallic; + pbr_input.material.perceptual_roughness = perceptual_roughness; var occlusion: f32 = 1.0; if ((material.flags & STANDARD_MATERIAL_FLAGS_OCCLUSION_TEXTURE_BIT) != 0u) { occlusion = textureSample(occlusion_texture, occlusion_sampler, in.uv).r; } + pbr_input.occlusion = occlusion; - var N: vec3 = normalize(in.world_normal); - -#ifdef VERTEX_TANGENTS -#ifdef STANDARDMATERIAL_NORMAL_MAP - // NOTE: The mikktspace method of normal mapping explicitly requires that these NOT be - // normalized nor any Gram-Schmidt applied to ensure the vertex normal is orthogonal to the - // vertex tangent! Do not change this code unless you really know what you are doing. - // http://www.mikktspace.com/ - var T: vec3 = in.world_tangent.xyz; - var B: vec3 = in.world_tangent.w * cross(N, T); -#endif -#endif + pbr_input.frag_coord = in.frag_coord; + pbr_input.world_position = in.world_position; + pbr_input.world_normal = in.world_normal; - if ((material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u) { - if (!in.is_front) { - N = -N; -#ifdef VERTEX_TANGENTS -#ifdef STANDARDMATERIAL_NORMAL_MAP - T = -T; - B = -B; -#endif -#endif - } - } + pbr_input.is_orthographic = view.projection[3].w == 1.0; + pbr_input.N = prepare_normal( + in.world_normal, #ifdef VERTEX_TANGENTS #ifdef STANDARDMATERIAL_NORMAL_MAP - let TBN = mat3x3(T, B, N); - // Nt is the tangent-space normal. - var Nt: vec3; - if ((material.flags & STANDARD_MATERIAL_FLAGS_TWO_COMPONENT_NORMAL_MAP) != 0u) { - // Only use the xy components and derive z for 2-component normal maps. - Nt = vec3(textureSample(normal_map_texture, normal_map_sampler, in.uv).rg * 2.0 - 1.0, 0.0); - Nt.z = sqrt(1.0 - Nt.x * Nt.x - Nt.y * Nt.y); - } else { - Nt = textureSample(normal_map_texture, normal_map_sampler, in.uv).rgb * 2.0 - 1.0; - } - // Normal maps authored for DirectX require flipping the y component - if ((material.flags & STANDARD_MATERIAL_FLAGS_FLIP_NORMAL_MAP_Y) != 0u) { - Nt.y = -Nt.y; - } - // NOTE: The mikktspace method of normal mapping applies maps the tangent-space normal from - // the normal map texture in this way to be an EXACT inverse of how the normal map baker - // calculates the normal maps so there is no error introduced. Do not change this code - // unless you really know what you are doing. - // http://www.mikktspace.com/ - N = normalize(Nt.x * T + Nt.y * B + Nt.z * N); + in.world_tangent, #endif #endif - - if ((material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_OPAQUE) != 0u) { - // NOTE: If rendering as opaque, alpha should be ignored so set to 1.0 - output_color.a = 1.0; - } else if ((material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK) != 0u) { - if (output_color.a >= material.alpha_cutoff) { - // NOTE: If rendering as masked alpha and >= the cutoff, render as fully opaque - output_color.a = 1.0; - } else { - // NOTE: output_color.a < material.alpha_cutoff should not is not rendered - // NOTE: This and any other discards mean that early-z testing cannot be done! - discard; - } - } - - var V: vec3; - // If the projection is not orthographic - let is_orthographic = view.projection[3].w == 1.0; - if (is_orthographic) { - // Orthographic view vector - V = normalize(vec3(view.view_proj[0].z, view.view_proj[1].z, view.view_proj[2].z)); - } else { - // Only valid for a perpective projection - V = normalize(view.world_position.xyz - in.world_position.xyz); - } - - // Neubelt and Pettineo 2013, "Crafting a Next-gen Material Pipeline for The Order: 1886" - let NdotV = max(dot(N, V), 0.0001); - - // Remapping [0,1] reflectance to F0 - // See https://google.github.io/filament/Filament.html#materialsystem/parameterization/remapping - let reflectance = material.reflectance; - let F0 = 0.16 * reflectance * reflectance * (1.0 - metallic) + output_color.rgb * metallic; - - // Diffuse strength inversely related to metallicity - let diffuse_color = output_color.rgb * (1.0 - metallic); - - let R = reflect(-V, N); - - // accumulate color - var light_accum: vec3 = vec3(0.0); - - let view_z = dot(vec4( - view.inverse_view[0].z, - view.inverse_view[1].z, - view.inverse_view[2].z, - view.inverse_view[3].z - ), in.world_position); - let cluster_index = fragment_cluster_index(in.frag_coord.xy, view_z, is_orthographic); - let offset_and_count = unpack_offset_and_count(cluster_index); - for (var i: u32 = offset_and_count[0]; i < offset_and_count[0] + offset_and_count[1]; i = i + 1u) { - let light_id = get_light_id(i); - let light = point_lights.data[light_id]; - var shadow: f32 = 1.0; - if ((mesh.flags & MESH_FLAGS_SHADOW_RECEIVER_BIT) != 0u - && (light.flags & POINT_LIGHT_FLAGS_SHADOWS_ENABLED_BIT) != 0u) { - shadow = fetch_point_shadow(light_id, in.world_position, in.world_normal); - } - let light_contrib = point_light(in.world_position.xyz, light, roughness, NdotV, N, V, R, F0, diffuse_color); - light_accum = light_accum + light_contrib * shadow; - } - - let n_directional_lights = lights.n_directional_lights; - for (var i: u32 = 0u; i < n_directional_lights; i = i + 1u) { - let light = lights.directional_lights[i]; - var shadow: f32 = 1.0; - if ((mesh.flags & MESH_FLAGS_SHADOW_RECEIVER_BIT) != 0u - && (light.flags & DIRECTIONAL_LIGHT_FLAGS_SHADOWS_ENABLED_BIT) != 0u) { - shadow = fetch_directional_shadow(i, in.world_position, in.world_normal); - } - let light_contrib = directional_light(light, roughness, NdotV, N, V, R, F0, diffuse_color); - light_accum = light_accum + light_contrib * shadow; - } - - let diffuse_ambient = EnvBRDFApprox(diffuse_color, 1.0, NdotV); - let specular_ambient = EnvBRDFApprox(F0, perceptual_roughness, NdotV); - - output_color = vec4( - light_accum + - (diffuse_ambient + specular_ambient) * lights.ambient_color.rgb * occlusion + - emissive.rgb * output_color.a, - output_color.a); - - output_color = cluster_debug_visualization( - output_color, - view_z, - is_orthographic, - offset_and_count, - cluster_index, + in.uv, + in.is_front, ); + pbr_input.V = calculate_view(in.world_position, pbr_input.is_orthographic); - // tone_mapping - output_color = vec4(reinhard_luminance(output_color.rgb), output_color.a); - // Gamma correction. - // Not needed with sRGB buffer - // output_color.rgb = pow(output_color.rgb, vec3(1.0 / 2.2)); + output_color = pbr(pbr_input); } return output_color; diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl new file mode 100644 index 0000000000000..a2349fcbdeee3 --- /dev/null +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -0,0 +1,196 @@ +#define_import_path bevy_pbr::pbr_functions + +// NOTE: This ensures that the world_normal is normalized and if +// vertex tangents and normal maps then normal mapping may be applied. +fn prepare_normal( + world_normal: vec3, +#ifdef VERTEX_TANGENTS +#ifdef STANDARDMATERIAL_NORMAL_MAP + world_tangent: vec4, +#endif +#endif + uv: vec2, + is_front: bool, +) -> vec3 { + var N: vec3 = normalize(world_normal); + +#ifdef VERTEX_TANGENTS +#ifdef STANDARDMATERIAL_NORMAL_MAP + // NOTE: The mikktspace method of normal mapping explicitly requires that these NOT be + // normalized nor any Gram-Schmidt applied to ensure the vertex normal is orthogonal to the + // vertex tangent! Do not change this code unless you really know what you are doing. + // http://www.mikktspace.com/ + var T: vec3 = world_tangent.xyz; + var B: vec3 = world_tangent.w * cross(N, T); +#endif +#endif + + if ((material.flags & STANDARD_MATERIAL_FLAGS_DOUBLE_SIDED_BIT) != 0u) { + if (!is_front) { + N = -N; +#ifdef VERTEX_TANGENTS +#ifdef STANDARDMATERIAL_NORMAL_MAP + T = -T; + B = -B; +#endif +#endif + } + } + +#ifdef VERTEX_TANGENTS +#ifdef STANDARDMATERIAL_NORMAL_MAP + // Nt is the tangent-space normal. + var Nt: vec3; + if ((material.flags & STANDARD_MATERIAL_FLAGS_TWO_COMPONENT_NORMAL_MAP) != 0u) { + // Only use the xy components and derive z for 2-component normal maps. + Nt = vec3(textureSample(normal_map_texture, normal_map_sampler, uv).rg * 2.0 - 1.0, 0.0); + Nt.z = sqrt(1.0 - Nt.x * Nt.x - Nt.y * Nt.y); + } else { + Nt = textureSample(normal_map_texture, normal_map_sampler, uv).rgb * 2.0 - 1.0; + } + // Normal maps authored for DirectX require flipping the y component + if ((material.flags & STANDARD_MATERIAL_FLAGS_FLIP_NORMAL_MAP_Y) != 0u) { + Nt.y = -Nt.y; + } + // NOTE: The mikktspace method of normal mapping applies maps the tangent-space normal from + // the normal map texture in this way to be an EXACT inverse of how the normal map baker + // calculates the normal maps so there is no error introduced. Do not change this code + // unless you really know what you are doing. + // http://www.mikktspace.com/ + N = normalize(Nt.x * T + Nt.y * B + Nt.z * N); +#endif +#endif + + return N; +} + +// NOTE: Correctly calculates the view vector depending on whether +// the projection is orthographic or perspective. +fn calculate_view( + world_position: vec4, + is_orthographic: bool, +) -> vec3 { + var V: vec3; + if (is_orthographic) { + // Orthographic view vector + V = normalize(vec3(view.view_proj[0].z, view.view_proj[1].z, view.view_proj[2].z)); + } else { + // Only valid for a perpective projection + V = normalize(view.world_position.xyz - world_position.xyz); + } + return V; +} + +struct PbrInput { + material: StandardMaterial; + occlusion: f32; + frag_coord: vec4; + world_position: vec4; + world_normal: vec3; + N: vec3; + V: vec3; + is_orthographic: bool; +}; + +fn pbr( + in: PbrInput, +) -> vec4 { + var output_color: vec4 = in.material.base_color; + + // TODO use .a for exposure compensation in HDR + let emissive = in.material.emissive; + + // calculate non-linear roughness from linear perceptualRoughness + let metallic = in.material.metallic; + let perceptual_roughness = in.material.perceptual_roughness; + let roughness = perceptualRoughnessToRoughness(perceptual_roughness); + + let occlusion = in.occlusion; + + if ((in.material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_OPAQUE) != 0u) { + // NOTE: If rendering as opaque, alpha should be ignored so set to 1.0 + output_color.a = 1.0; + } else if ((in.material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK) != 0u) { + if (output_color.a >= in.material.alpha_cutoff) { + // NOTE: If rendering as masked alpha and >= the cutoff, render as fully opaque + output_color.a = 1.0; + } else { + // NOTE: output_color.a < in.material.alpha_cutoff should not is not rendered + // NOTE: This and any other discards mean that early-z testing cannot be done! + discard; + } + } + + // Neubelt and Pettineo 2013, "Crafting a Next-gen Material Pipeline for The Order: 1886" + let NdotV = max(dot(in.N, in.V), 0.0001); + + // Remapping [0,1] reflectance to F0 + // See https://google.github.io/filament/Filament.html#materialsystem/parameterization/remapping + let reflectance = in.material.reflectance; + let F0 = 0.16 * reflectance * reflectance * (1.0 - metallic) + output_color.rgb * metallic; + + // Diffuse strength inversely related to metallicity + let diffuse_color = output_color.rgb * (1.0 - metallic); + + let R = reflect(-in.V, in.N); + + // accumulate color + var light_accum: vec3 = vec3(0.0); + + let view_z = dot(vec4( + view.inverse_view[0].z, + view.inverse_view[1].z, + view.inverse_view[2].z, + view.inverse_view[3].z + ), in.world_position); + let cluster_index = fragment_cluster_index(in.frag_coord.xy, view_z, in.is_orthographic); + let offset_and_count = unpack_offset_and_count(cluster_index); + for (var i: u32 = offset_and_count[0]; i < offset_and_count[0] + offset_and_count[1]; i = i + 1u) { + let light_id = get_light_id(i); + let light = point_lights.data[light_id]; + var shadow: f32 = 1.0; + if ((mesh.flags & MESH_FLAGS_SHADOW_RECEIVER_BIT) != 0u + && (light.flags & POINT_LIGHT_FLAGS_SHADOWS_ENABLED_BIT) != 0u) { + shadow = fetch_point_shadow(light_id, in.world_position, in.world_normal); + } + let light_contrib = point_light(in.world_position.xyz, light, roughness, NdotV, in.N, in.V, R, F0, diffuse_color); + light_accum = light_accum + light_contrib * shadow; + } + + let n_directional_lights = lights.n_directional_lights; + for (var i: u32 = 0u; i < n_directional_lights; i = i + 1u) { + let light = lights.directional_lights[i]; + var shadow: f32 = 1.0; + if ((mesh.flags & MESH_FLAGS_SHADOW_RECEIVER_BIT) != 0u + && (light.flags & DIRECTIONAL_LIGHT_FLAGS_SHADOWS_ENABLED_BIT) != 0u) { + shadow = fetch_directional_shadow(i, in.world_position, in.world_normal); + } + let light_contrib = directional_light(light, roughness, NdotV, in.N, in.V, R, F0, diffuse_color); + light_accum = light_accum + light_contrib * shadow; + } + + let diffuse_ambient = EnvBRDFApprox(diffuse_color, 1.0, NdotV); + let specular_ambient = EnvBRDFApprox(F0, perceptual_roughness, NdotV); + + output_color = vec4( + light_accum + + (diffuse_ambient + specular_ambient) * lights.ambient_color.rgb * occlusion + + emissive.rgb * output_color.a, + output_color.a); + + output_color = cluster_debug_visualization( + output_color, + view_z, + in.is_orthographic, + offset_and_count, + cluster_index, + ); + + // tone_mapping + output_color = vec4(reinhard_luminance(output_color.rgb), output_color.a); + // Gamma correction. + // Not needed with sRGB buffer + // output_color.rgb = pow(output_color.rgb, vec3(1.0 / 2.2)); + + return output_color; +} From 56d3a19a1670310f53fdb4441304223879865b2c Mon Sep 17 00:00:00 2001 From: colepoirier Date: Tue, 21 Jun 2022 22:57:59 +0000 Subject: [PATCH 11/18] depend on dioxus(and bevy)-maintained fork of stretch (taffy) (#4716) # Objective DioxusLabs and Bevy have taken over maintaining what was our abandoned ui layout dependency [stretch](https://github.com/vislyhq/stretch). Dioxus' fork has had a lot of work done on it by @alice-i-cecile, @Weibye , @jkelleyrtp, @mockersf, @HackerFoo, @TimJentzsch and a dozen other contributors and now is in much better shape than stretch was. The updated crate is called taffy and is available on github [here](https://github.com/DioxusLabs/taffy) ([taffy](https://crates.io/crates/taffy) on crates.io). The goal of this PR is to replace stretch v0.3.2 with taffy v0.1.0. ## Solution I changed the bevy_ui Cargo.toml to depend on taffy instead of stretch and fixed all the errors rustc complained about. --- ## Changelog Changed bevy_ui layout dependency from stretch to taffy (the maintained fork of stretch). fixes #677 ## Migration Guide The public api of taffy is different from that of stretch so please advise me on what to do here @alice-i-cecile. --- crates/bevy_ui/Cargo.toml | 6 +- crates/bevy_ui/src/flex/convert.rs | 130 ++++++++++++------------- crates/bevy_ui/src/flex/mod.rs | 146 ++++++++++++++--------------- deny.toml | 5 - 4 files changed, 133 insertions(+), 154 deletions(-) diff --git a/crates/bevy_ui/Cargo.toml b/crates/bevy_ui/Cargo.toml index 968a14a53da9f..170b26b0ff0d5 100644 --- a/crates/bevy_ui/Cargo.toml +++ b/crates/bevy_ui/Cargo.toml @@ -19,7 +19,9 @@ bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.8.0-dev" } bevy_input = { path = "../bevy_input", version = "0.8.0-dev" } bevy_log = { path = "../bevy_log", version = "0.8.0-dev" } bevy_math = { path = "../bevy_math", version = "0.8.0-dev" } -bevy_reflect = { path = "../bevy_reflect", version = "0.8.0-dev", features = ["bevy"] } +bevy_reflect = { path = "../bevy_reflect", version = "0.8.0-dev", features = [ + "bevy", +] } bevy_render = { path = "../bevy_render", version = "0.8.0-dev" } bevy_sprite = { path = "../bevy_sprite", version = "0.8.0-dev" } bevy_text = { path = "../bevy_text", version = "0.8.0-dev" } @@ -28,7 +30,7 @@ bevy_window = { path = "../bevy_window", version = "0.8.0-dev" } bevy_utils = { path = "../bevy_utils", version = "0.8.0-dev" } # other -stretch = "0.3.2" +taffy = "0.1.0" serde = { version = "1", features = ["derive"] } smallvec = { version = "1.6", features = ["union", "const_generics"] } bytemuck = { version = "1.5", features = ["derive"] } diff --git a/crates/bevy_ui/src/flex/convert.rs b/crates/bevy_ui/src/flex/convert.rs index 3c73b1f566714..57c17cdad9dcc 100644 --- a/crates/bevy_ui/src/flex/convert.rs +++ b/crates/bevy_ui/src/flex/convert.rs @@ -1,13 +1,13 @@ use crate::{ - AlignContent, AlignItems, AlignSelf, Direction, Display, FlexDirection, FlexWrap, - JustifyContent, PositionType, Size, Style, UiRect, Val, + AlignContent, AlignItems, AlignSelf, Display, FlexDirection, FlexWrap, JustifyContent, + PositionType, Size, Style, UiRect, Val, }; pub fn from_rect( scale_factor: f64, rect: UiRect, -) -> stretch::geometry::Rect { - stretch::geometry::Rect { +) -> taffy::geometry::Rect { + taffy::geometry::Rect { start: from_val(scale_factor, rect.left), end: from_val(scale_factor, rect.right), // NOTE: top and bottom are intentionally flipped. stretch has a flipped y-axis @@ -16,8 +16,8 @@ pub fn from_rect( } } -pub fn from_f32_size(scale_factor: f64, size: Size) -> stretch::geometry::Size { - stretch::geometry::Size { +pub fn from_f32_size(scale_factor: f64, size: Size) -> taffy::geometry::Size { + taffy::geometry::Size { width: (scale_factor * size.width as f64) as f32, height: (scale_factor * size.height as f64) as f32, } @@ -26,19 +26,17 @@ pub fn from_f32_size(scale_factor: f64, size: Size) -> stretch::geometry::S pub fn from_val_size( scale_factor: f64, size: Size, -) -> stretch::geometry::Size { - stretch::geometry::Size { +) -> taffy::geometry::Size { + taffy::geometry::Size { width: from_val(scale_factor, size.width), height: from_val(scale_factor, size.height), } } -pub fn from_style(scale_factor: f64, value: &Style) -> stretch::style::Style { - stretch::style::Style { - overflow: stretch::style::Overflow::Visible, +pub fn from_style(scale_factor: f64, value: &Style) -> taffy::style::Style { + taffy::style::Style { display: value.display.into(), position_type: value.position_type.into(), - direction: value.direction.into(), flex_direction: value.flex_direction.into(), flex_wrap: value.flex_wrap.into(), align_items: value.align_items.into(), @@ -56,117 +54,107 @@ pub fn from_style(scale_factor: f64, value: &Style) -> stretch::style::Style { min_size: from_val_size(scale_factor, value.min_size), max_size: from_val_size(scale_factor, value.max_size), aspect_ratio: match value.aspect_ratio { - Some(value) => stretch::number::Number::Defined(value), - None => stretch::number::Number::Undefined, + Some(value) => taffy::number::Number::Defined(value), + None => taffy::number::Number::Undefined, }, } } -pub fn from_val(scale_factor: f64, val: Val) -> stretch::style::Dimension { +pub fn from_val(scale_factor: f64, val: Val) -> taffy::style::Dimension { match val { - Val::Auto => stretch::style::Dimension::Auto, - Val::Percent(value) => stretch::style::Dimension::Percent(value / 100.0), - Val::Px(value) => stretch::style::Dimension::Points((scale_factor * value as f64) as f32), - Val::Undefined => stretch::style::Dimension::Undefined, + Val::Auto => taffy::style::Dimension::Auto, + Val::Percent(value) => taffy::style::Dimension::Percent(value / 100.0), + Val::Px(value) => taffy::style::Dimension::Points((scale_factor * value as f64) as f32), + Val::Undefined => taffy::style::Dimension::Undefined, } } -impl From for stretch::style::AlignItems { +impl From for taffy::style::AlignItems { fn from(value: AlignItems) -> Self { match value { - AlignItems::FlexStart => stretch::style::AlignItems::FlexStart, - AlignItems::FlexEnd => stretch::style::AlignItems::FlexEnd, - AlignItems::Center => stretch::style::AlignItems::Center, - AlignItems::Baseline => stretch::style::AlignItems::Baseline, - AlignItems::Stretch => stretch::style::AlignItems::Stretch, + AlignItems::FlexStart => taffy::style::AlignItems::FlexStart, + AlignItems::FlexEnd => taffy::style::AlignItems::FlexEnd, + AlignItems::Center => taffy::style::AlignItems::Center, + AlignItems::Baseline => taffy::style::AlignItems::Baseline, + AlignItems::Stretch => taffy::style::AlignItems::Stretch, } } } -impl From for stretch::style::AlignSelf { +impl From for taffy::style::AlignSelf { fn from(value: AlignSelf) -> Self { match value { - AlignSelf::Auto => stretch::style::AlignSelf::Auto, - AlignSelf::FlexStart => stretch::style::AlignSelf::FlexStart, - AlignSelf::FlexEnd => stretch::style::AlignSelf::FlexEnd, - AlignSelf::Center => stretch::style::AlignSelf::Center, - AlignSelf::Baseline => stretch::style::AlignSelf::Baseline, - AlignSelf::Stretch => stretch::style::AlignSelf::Stretch, + AlignSelf::Auto => taffy::style::AlignSelf::Auto, + AlignSelf::FlexStart => taffy::style::AlignSelf::FlexStart, + AlignSelf::FlexEnd => taffy::style::AlignSelf::FlexEnd, + AlignSelf::Center => taffy::style::AlignSelf::Center, + AlignSelf::Baseline => taffy::style::AlignSelf::Baseline, + AlignSelf::Stretch => taffy::style::AlignSelf::Stretch, } } } -impl From for stretch::style::AlignContent { +impl From for taffy::style::AlignContent { fn from(value: AlignContent) -> Self { match value { - AlignContent::FlexStart => stretch::style::AlignContent::FlexStart, - AlignContent::FlexEnd => stretch::style::AlignContent::FlexEnd, - AlignContent::Center => stretch::style::AlignContent::Center, - AlignContent::Stretch => stretch::style::AlignContent::Stretch, - AlignContent::SpaceBetween => stretch::style::AlignContent::SpaceBetween, - AlignContent::SpaceAround => stretch::style::AlignContent::SpaceAround, + AlignContent::FlexStart => taffy::style::AlignContent::FlexStart, + AlignContent::FlexEnd => taffy::style::AlignContent::FlexEnd, + AlignContent::Center => taffy::style::AlignContent::Center, + AlignContent::Stretch => taffy::style::AlignContent::Stretch, + AlignContent::SpaceBetween => taffy::style::AlignContent::SpaceBetween, + AlignContent::SpaceAround => taffy::style::AlignContent::SpaceAround, } } } -impl From for stretch::style::Direction { - fn from(value: Direction) -> Self { - match value { - Direction::Inherit => stretch::style::Direction::Inherit, - Direction::LeftToRight => stretch::style::Direction::LTR, - Direction::RightToLeft => stretch::style::Direction::RTL, - } - } -} - -impl From for stretch::style::Display { +impl From for taffy::style::Display { fn from(value: Display) -> Self { match value { - Display::Flex => stretch::style::Display::Flex, - Display::None => stretch::style::Display::None, + Display::Flex => taffy::style::Display::Flex, + Display::None => taffy::style::Display::None, } } } -impl From for stretch::style::FlexDirection { +impl From for taffy::style::FlexDirection { fn from(value: FlexDirection) -> Self { match value { - FlexDirection::Row => stretch::style::FlexDirection::Row, - FlexDirection::Column => stretch::style::FlexDirection::Column, - FlexDirection::RowReverse => stretch::style::FlexDirection::RowReverse, - FlexDirection::ColumnReverse => stretch::style::FlexDirection::ColumnReverse, + FlexDirection::Row => taffy::style::FlexDirection::Row, + FlexDirection::Column => taffy::style::FlexDirection::Column, + FlexDirection::RowReverse => taffy::style::FlexDirection::RowReverse, + FlexDirection::ColumnReverse => taffy::style::FlexDirection::ColumnReverse, } } } -impl From for stretch::style::JustifyContent { +impl From for taffy::style::JustifyContent { fn from(value: JustifyContent) -> Self { match value { - JustifyContent::FlexStart => stretch::style::JustifyContent::FlexStart, - JustifyContent::FlexEnd => stretch::style::JustifyContent::FlexEnd, - JustifyContent::Center => stretch::style::JustifyContent::Center, - JustifyContent::SpaceBetween => stretch::style::JustifyContent::SpaceBetween, - JustifyContent::SpaceAround => stretch::style::JustifyContent::SpaceAround, - JustifyContent::SpaceEvenly => stretch::style::JustifyContent::SpaceEvenly, + JustifyContent::FlexStart => taffy::style::JustifyContent::FlexStart, + JustifyContent::FlexEnd => taffy::style::JustifyContent::FlexEnd, + JustifyContent::Center => taffy::style::JustifyContent::Center, + JustifyContent::SpaceBetween => taffy::style::JustifyContent::SpaceBetween, + JustifyContent::SpaceAround => taffy::style::JustifyContent::SpaceAround, + JustifyContent::SpaceEvenly => taffy::style::JustifyContent::SpaceEvenly, } } } -impl From for stretch::style::PositionType { +impl From for taffy::style::PositionType { fn from(value: PositionType) -> Self { match value { - PositionType::Relative => stretch::style::PositionType::Relative, - PositionType::Absolute => stretch::style::PositionType::Absolute, + PositionType::Relative => taffy::style::PositionType::Relative, + PositionType::Absolute => taffy::style::PositionType::Absolute, } } } -impl From for stretch::style::FlexWrap { +impl From for taffy::style::FlexWrap { fn from(value: FlexWrap) -> Self { match value { - FlexWrap::NoWrap => stretch::style::FlexWrap::NoWrap, - FlexWrap::Wrap => stretch::style::FlexWrap::Wrap, - FlexWrap::WrapReverse => stretch::style::FlexWrap::WrapReverse, + FlexWrap::NoWrap => taffy::style::FlexWrap::NoWrap, + FlexWrap::Wrap => taffy::style::FlexWrap::Wrap, + FlexWrap::WrapReverse => taffy::style::FlexWrap::WrapReverse, } } } diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 607daaacbc26a..8bc4a2fe10571 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -14,15 +14,15 @@ use bevy_transform::components::Transform; use bevy_utils::HashMap; use bevy_window::{Window, WindowId, WindowScaleFactorChanged, Windows}; use std::fmt; -use stretch::{number::Number, Stretch}; +use taffy::{number::Number, Taffy}; pub struct FlexSurface { - entity_to_stretch: HashMap, - window_nodes: HashMap, - stretch: Stretch, + entity_to_taffy: HashMap, + window_nodes: HashMap, + taffy: Taffy, } -// SAFE: as long as MeasureFunc is Send + Sync. https://github.com/vislyhq/stretch/issues/69 +// SAFE: as long as MeasureFunc is Send + Sync. https://github.com/DioxusLabs/taffy/issues/146 // TODO: remove allow on lint - https://github.com/bevyengine/bevy/issues/3666 #[allow(clippy::non_send_fields_in_send_ty)] unsafe impl Send for FlexSurface {} @@ -30,16 +30,16 @@ unsafe impl Sync for FlexSurface {} fn _assert_send_sync_flex_surface_impl_safe() { fn _assert_send_sync() {} - _assert_send_sync::>(); - _assert_send_sync::>(); - // FIXME https://github.com/vislyhq/stretch/issues/69 - // _assert_send_sync::(); + _assert_send_sync::>(); + _assert_send_sync::>(); + // FIXME https://github.com/DioxusLabs/taffy/issues/146 + // _assert_send_sync::(); } impl fmt::Debug for FlexSurface { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_struct("FlexSurface") - .field("entity_to_stretch", &self.entity_to_stretch) + .field("entity_to_taffy", &self.entity_to_taffy) .field("window_nodes", &self.window_nodes) .finish() } @@ -48,9 +48,9 @@ impl fmt::Debug for FlexSurface { impl Default for FlexSurface { fn default() -> Self { Self { - entity_to_stretch: Default::default(), + entity_to_taffy: Default::default(), window_nodes: Default::default(), - stretch: Stretch::new(), + taffy: Taffy::new(), } } } @@ -58,17 +58,15 @@ impl Default for FlexSurface { impl FlexSurface { pub fn upsert_node(&mut self, entity: Entity, style: &Style, scale_factor: f64) { let mut added = false; - let stretch = &mut self.stretch; - let stretch_style = convert::from_style(scale_factor, style); - let stretch_node = self.entity_to_stretch.entry(entity).or_insert_with(|| { + let taffy = &mut self.taffy; + let taffy_style = convert::from_style(scale_factor, style); + let taffy_node = self.entity_to_taffy.entry(entity).or_insert_with(|| { added = true; - stretch.new_node(stretch_style, Vec::new()).unwrap() + taffy.new_node(taffy_style, &Vec::new()).unwrap() }); if !added { - self.stretch - .set_style(*stretch_node, stretch_style) - .unwrap(); + self.taffy.set_style(*taffy_node, taffy_style).unwrap(); } } @@ -79,46 +77,44 @@ impl FlexSurface { calculated_size: CalculatedSize, scale_factor: f64, ) { - let stretch = &mut self.stretch; - let stretch_style = convert::from_style(scale_factor, style); - let measure = Box::new(move |constraints: stretch::geometry::Size| { - let mut size = convert::from_f32_size(scale_factor, calculated_size.size); - match (constraints.width, constraints.height) { - (Number::Undefined, Number::Undefined) => {} - (Number::Defined(width), Number::Undefined) => { - size.height = width * size.height / size.width; - size.width = width; - } - (Number::Undefined, Number::Defined(height)) => { - size.width = height * size.width / size.height; - size.height = height; + let taffy = &mut self.taffy; + let taffy_style = convert::from_style(scale_factor, style); + let measure = taffy::node::MeasureFunc::Boxed(Box::new( + move |constraints: taffy::geometry::Size| { + let mut size = convert::from_f32_size(scale_factor, calculated_size.size); + match (constraints.width, constraints.height) { + (Number::Undefined, Number::Undefined) => {} + (Number::Defined(width), Number::Undefined) => { + size.height = width * size.height / size.width; + size.width = width; + } + (Number::Undefined, Number::Defined(height)) => { + size.width = height * size.width / size.height; + size.height = height; + } + (Number::Defined(width), Number::Defined(height)) => { + size.width = width; + size.height = height; + } } - (Number::Defined(width), Number::Defined(height)) => { - size.width = width; - size.height = height; - } - } - Ok(size) - }); + size + }, + )); - if let Some(stretch_node) = self.entity_to_stretch.get(&entity) { - self.stretch - .set_style(*stretch_node, stretch_style) - .unwrap(); - self.stretch - .set_measure(*stretch_node, Some(measure)) - .unwrap(); + if let Some(taffy_node) = self.entity_to_taffy.get(&entity) { + self.taffy.set_style(*taffy_node, taffy_style).unwrap(); + self.taffy.set_measure(*taffy_node, Some(measure)).unwrap(); } else { - let stretch_node = stretch.new_leaf(stretch_style, measure).unwrap(); - self.entity_to_stretch.insert(entity, stretch_node); + let taffy_node = taffy.new_leaf(taffy_style, measure).unwrap(); + self.entity_to_taffy.insert(entity, taffy_node); } } pub fn update_children(&mut self, entity: Entity, children: &Children) { - let mut stretch_children = Vec::with_capacity(children.len()); + let mut taffy_children = Vec::with_capacity(children.len()); for child in children.iter() { - if let Some(stretch_node) = self.entity_to_stretch.get(child) { - stretch_children.push(*stretch_node); + if let Some(taffy_node) = self.entity_to_taffy.get(child) { + taffy_children.push(*taffy_node); } else { warn!( "Unstyled child in a UI entity hierarchy. You are using an entity \ @@ -127,27 +123,27 @@ without UI components as a child of an entity with UI components, results may be } } - let stretch_node = self.entity_to_stretch.get(&entity).unwrap(); - self.stretch - .set_children(*stretch_node, stretch_children) + let taffy_node = self.entity_to_taffy.get(&entity).unwrap(); + self.taffy + .set_children(*taffy_node, &taffy_children) .unwrap(); } pub fn update_window(&mut self, window: &Window) { - let stretch = &mut self.stretch; + let taffy = &mut self.taffy; let node = self.window_nodes.entry(window.id()).or_insert_with(|| { - stretch - .new_node(stretch::style::Style::default(), Vec::new()) + taffy + .new_node(taffy::style::Style::default(), &Vec::new()) .unwrap() }); - stretch + taffy .set_style( *node, - stretch::style::Style { - size: stretch::geometry::Size { - width: stretch::style::Dimension::Points(window.physical_width() as f32), - height: stretch::style::Dimension::Points(window.physical_height() as f32), + taffy::style::Style { + size: taffy::geometry::Size { + width: taffy::style::Dimension::Points(window.physical_width() as f32), + height: taffy::style::Dimension::Points(window.physical_height() as f32), }, ..Default::default() }, @@ -160,28 +156,26 @@ without UI components as a child of an entity with UI components, results may be window_id: WindowId, children: impl Iterator, ) { - let stretch_node = self.window_nodes.get(&window_id).unwrap(); + let taffy_node = self.window_nodes.get(&window_id).unwrap(); let child_nodes = children - .map(|e| *self.entity_to_stretch.get(&e).unwrap()) - .collect::>(); - self.stretch - .set_children(*stretch_node, child_nodes) - .unwrap(); + .map(|e| *self.entity_to_taffy.get(&e).unwrap()) + .collect::>(); + self.taffy.set_children(*taffy_node, &child_nodes).unwrap(); } pub fn compute_window_layouts(&mut self) { for window_node in self.window_nodes.values() { - self.stretch - .compute_layout(*window_node, stretch::geometry::Size::undefined()) + self.taffy + .compute_layout(*window_node, taffy::geometry::Size::undefined()) .unwrap(); } } - pub fn get_layout(&self, entity: Entity) -> Result<&stretch::result::Layout, FlexError> { - if let Some(stretch_node) = self.entity_to_stretch.get(&entity) { - self.stretch - .layout(*stretch_node) - .map_err(FlexError::StretchError) + pub fn get_layout(&self, entity: Entity) -> Result<&taffy::layout::Layout, FlexError> { + if let Some(taffy_node) = self.entity_to_taffy.get(&entity) { + self.taffy + .layout(*taffy_node) + .map_err(FlexError::TaffyError) } else { warn!( "Styled child in a non-UI entity hierarchy. You are using an entity \ @@ -195,7 +189,7 @@ with UI components as a child of an entity without UI components, results may be #[derive(Debug)] pub enum FlexError { InvalidHierarchy, - StretchError(stretch::Error), + TaffyError(taffy::Error), } #[allow(clippy::too_many_arguments)] diff --git a/deny.toml b/deny.toml index 4f4f007f73374..bbabea5a99605 100644 --- a/deny.toml +++ b/deny.toml @@ -25,11 +25,6 @@ allow = [ ] default = "deny" -[[licenses.clarify]] -name = "stretch" -expression = "MIT" -license-files = [] - [bans] multiple-versions = "deny" wildcards = "deny" From bac2a5e47be60386b4c094aec4bf07d470f7c8c0 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 21 Jun 2022 18:24:45 -0700 Subject: [PATCH 12/18] Allow custom sorting implementations. --- crates/bevy_core_pipeline/src/core_2d/mod.rs | 5 +-- crates/bevy_render/src/render_phase/draw.rs | 45 ++++++-------------- crates/bevy_render/src/render_phase/mod.rs | 10 ++--- 3 files changed, 17 insertions(+), 43 deletions(-) diff --git a/crates/bevy_core_pipeline/src/core_2d/mod.rs b/crates/bevy_core_pipeline/src/core_2d/mod.rs index e7d4eb20451cb..69ecc9332844a 100644 --- a/crates/bevy_core_pipeline/src/core_2d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_2d/mod.rs @@ -23,7 +23,6 @@ use bevy_render::{ render_phase::{ batch_phase_system, sort_phase_system, BatchedPhaseItem, CachedRenderPipelinePhaseItem, DrawFunctionId, DrawFunctions, EntityPhaseItem, PhaseItem, RenderPhase, - RenderPhaseSortMode, }, render_resource::CachedRenderPipelineId, RenderApp, RenderStage, @@ -93,8 +92,8 @@ impl PhaseItem for Transparent2d { } #[inline] - fn sort_mode() -> RenderPhaseSortMode { - RenderPhaseSortMode::Stable + fn sort(items: &mut [Self]) { + items.sort_by_key(|item| item.sort_key()) } } diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 7f54a8e720b98..0b6b63bfc27fb 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -30,31 +30,12 @@ pub trait Draw: Send + Sync + 'static { ); } -/// Configures how a render phase is sorted. For more information, see [`PhaseItem::sort_mode`]. -pub enum RenderPhaseSortMode { - /// Use a stable sort that preserves the order of items with the same sort key. This is - /// required for proper batching based on external criteria, and for stability in - /// order-dependent transparency, or other drawing techniques where draw order defines the - /// final appearance, including those that do not use a depth buffer. - Stable, - /// The default: allows unstable sorting. Usually faster than Stable. - Unstable, - /// Unsorted. Omits sorting entirely. - Unsorted, -} - -impl Default for RenderPhaseSortMode { - fn default() -> Self { - Self::Unstable - } -} - /// An item which will be drawn to the screen. A phase item should be queued up for rendering /// during the [`RenderStage::Queue`](crate::RenderStage::Queue) stage. /// Afterwards it will be sorted and rendered automatically in the /// [`RenderStage::PhaseSort`](crate::RenderStage::PhaseSort) stage and /// [`RenderStage::Render`](crate::RenderStage::Render) stage, respectively. -pub trait PhaseItem: Send + Sync + 'static { +pub trait PhaseItem: Sized + Send + Sync + 'static { /// The type used for ordering the items. The smallest values are drawn first. type SortKey: Ord; /// Determines the order in which the items are drawn during the corresponding [`RenderPhase`](super::RenderPhase). @@ -62,22 +43,21 @@ pub trait PhaseItem: Send + Sync + 'static { /// Specifies the [`Draw`] function used to render the item. fn draw_function(&self) -> DrawFunctionId; - /// Specifies what kind of sort to apply to the phase. Generally if the same type - /// implements [`BatchedPhaseItem`], this should return [`RenderPhaseSortMode::Stable`]. + /// Sorts a slice of phase items into render order. Generally if the same type + /// implements [`BatchedPhaseItem`], this should use a stable sort like [`slice::sort_by_key`]. /// In almost all other cases, this should not be altered from the default, - /// [`RenderPhaseSortMode::Unstable`], as this provides the best balance of CPU and GPU + /// which uses a unstable sort, as this provides the best balance of CPU and GPU /// performance. /// - /// It's generally only advised to use [`RenderPhaseSortMode::Unsorted`] if and only if - /// the renderer supports a depth prepass, which is by default not supported by the rest - /// of Bevy's first party rendering crates. Even then, this may have a negative impact - /// on GPU-side performance due to overdraw. + /// Implementors can optionally not sort the list at all. This is generally adviseble if and + /// only if the renderer supports a depth prepass, which is by default not supported by + /// the rest of Bevy's first party rendering crates. Even then, this may have a negative + /// impact on GPU-side performance due to overdraw. /// - /// It's advised to always profile for performance changes when changing this to - /// a different value. + /// It's advised to always profile for performance changes when changing this implementation. #[inline] - fn sort_mode() -> RenderPhaseSortMode { - Default::default() + fn sort(items: &mut [Self]) { + items.sort_unstable_by_key(|item| item.sort_key()) } } @@ -209,8 +189,7 @@ pub trait CachedRenderPipelinePhaseItem: PhaseItem { /// to render them in a single draw call. /// /// If this is implemented on a type, the implementation of [`PhaseItem::sort_mode`] should -/// be changed to return [`RenderPhaseSortMode::Stable`], or incorrect/suboptimal batching -/// may result. +/// be changed to implement a stable sort, or incorrect/suboptimal batching may result. pub trait BatchedPhaseItem: EntityPhaseItem { /// Range in the vertex buffer of this item fn batch_range(&self) -> &Option>; diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 992a8b993b2f4..d22693a742232 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -29,11 +29,7 @@ impl RenderPhase { /// Sorts all of its [`PhaseItems`](PhaseItem). pub fn sort(&mut self) { - match I::sort_mode() { - RenderPhaseSortMode::Stable => self.items.sort_by_key(|d| d.sort_key()), - RenderPhaseSortMode::Unstable => self.items.sort_unstable_by_key(|d| d.sort_key()), - RenderPhaseSortMode::Unsorted => {} - } + I::sort(&mut self.items) } } @@ -103,8 +99,8 @@ mod tests { unimplemented!(); } - fn sort_mode() -> RenderPhaseSortMode { - RenderPhaseSortMode::Stable + fn sort(items: &mut [Self]) { + item.sort_by_key(|item| item.sort_key()) } } impl EntityPhaseItem for TestPhaseItem { From 1200c0ca3e6036adea27b4683ac3f8cad39528c4 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 21 Jun 2022 18:30:06 -0700 Subject: [PATCH 13/18] Use radsort for a further speedup --- crates/bevy_core_pipeline/Cargo.toml | 2 +- crates/bevy_core_pipeline/src/core_3d/mod.rs | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/bevy_core_pipeline/Cargo.toml b/crates/bevy_core_pipeline/Cargo.toml index 89602f5048756..26c8ffee15116 100644 --- a/crates/bevy_core_pipeline/Cargo.toml +++ b/crates/bevy_core_pipeline/Cargo.toml @@ -28,4 +28,4 @@ bevy_transform = { path = "../bevy_transform", version = "0.8.0-dev" } bevy_utils = { path = "../bevy_utils", version = "0.8.0-dev" } serde = { version = "1", features = ["derive"] } - +radsort = "0.1" diff --git a/crates/bevy_core_pipeline/src/core_3d/mod.rs b/crates/bevy_core_pipeline/src/core_3d/mod.rs index 6342d8471291b..472929f6d7d8c 100644 --- a/crates/bevy_core_pipeline/src/core_3d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_3d/mod.rs @@ -98,6 +98,11 @@ impl PhaseItem for Opaque3d { fn draw_function(&self) -> DrawFunctionId { self.draw_function } + + #[inline] + fn sort(items: &mut [Self]) { + radsort::sort_by_key(items, |item| item.distance) + } } impl EntityPhaseItem for Opaque3d { @@ -133,6 +138,11 @@ impl PhaseItem for AlphaMask3d { fn draw_function(&self) -> DrawFunctionId { self.draw_function } + + #[inline] + fn sort(items: &mut [Self]) { + radsort::sort_by_key(items, |item| item.distance) + } } impl EntityPhaseItem for AlphaMask3d { @@ -168,6 +178,11 @@ impl PhaseItem for Transparent3d { fn draw_function(&self) -> DrawFunctionId { self.draw_function } + + #[inline] + fn sort(items: &mut [Self]) { + radsort::sort_by_key(items, |item| item.distance) + } } impl EntityPhaseItem for Transparent3d { From 7f1615a135c966a1e1ea86b8b6a65df4ad076422 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 01:33:27 -0700 Subject: [PATCH 14/18] Fix CI --- crates/bevy_core_pipeline/src/core_3d/mod.rs | 6 +++--- crates/bevy_render/src/render_phase/draw.rs | 2 +- crates/bevy_render/src/render_phase/mod.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_core_pipeline/src/core_3d/mod.rs b/crates/bevy_core_pipeline/src/core_3d/mod.rs index 472929f6d7d8c..e3b230a1e7e20 100644 --- a/crates/bevy_core_pipeline/src/core_3d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_3d/mod.rs @@ -101,7 +101,7 @@ impl PhaseItem for Opaque3d { #[inline] fn sort(items: &mut [Self]) { - radsort::sort_by_key(items, |item| item.distance) + radsort::sort_by_key(items, |item| item.distance); } } @@ -141,7 +141,7 @@ impl PhaseItem for AlphaMask3d { #[inline] fn sort(items: &mut [Self]) { - radsort::sort_by_key(items, |item| item.distance) + radsort::sort_by_key(items, |item| item.distance); } } @@ -181,7 +181,7 @@ impl PhaseItem for Transparent3d { #[inline] fn sort(items: &mut [Self]) { - radsort::sort_by_key(items, |item| item.distance) + radsort::sort_by_key(items, |item| item.distance); } } diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 0b6b63bfc27fb..788ff49b32110 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -188,7 +188,7 @@ pub trait CachedRenderPipelinePhaseItem: PhaseItem { /// Batching is an optimization that regroups multiple items in the same vertex buffer /// to render them in a single draw call. /// -/// If this is implemented on a type, the implementation of [`PhaseItem::sort_mode`] should +/// If this is implemented on a type, the implementation of [`PhaseItem::sort`] should /// be changed to implement a stable sort, or incorrect/suboptimal batching may result. pub trait BatchedPhaseItem: EntityPhaseItem { /// Range in the vertex buffer of this item diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index d22693a742232..6abd8bfe4ae89 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -100,7 +100,7 @@ mod tests { } fn sort(items: &mut [Self]) { - item.sort_by_key(|item| item.sort_key()) + items.sort_by_key(|item| item.sort_key()); } } impl EntityPhaseItem for TestPhaseItem { From b228ef3e151661195d6a4f3e2862fe43d2d46833 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 09:44:11 -0700 Subject: [PATCH 15/18] Shut up clippy --- crates/bevy_render/src/render_phase/draw.rs | 2 +- crates/bevy_render/src/render_phase/mod.rs | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 788ff49b32110..b4612ee446bd2 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -57,7 +57,7 @@ pub trait PhaseItem: Sized + Send + Sync + 'static { /// It's advised to always profile for performance changes when changing this implementation. #[inline] fn sort(items: &mut [Self]) { - items.sort_unstable_by_key(|item| item.sort_key()) + items.sort_unstable_by_key(|item| item.sort_key()); } } diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 6abd8bfe4ae89..464cbdcbe92a6 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -29,7 +29,7 @@ impl RenderPhase { /// Sorts all of its [`PhaseItems`](PhaseItem). pub fn sort(&mut self) { - I::sort(&mut self.items) + I::sort(&mut self.items); } } @@ -98,10 +98,6 @@ mod tests { fn draw_function(&self) -> DrawFunctionId { unimplemented!(); } - - fn sort(items: &mut [Self]) { - items.sort_by_key(|item| item.sort_key()); - } } impl EntityPhaseItem for TestPhaseItem { fn entity(&self) -> bevy_ecs::entity::Entity { From edf15086d45a878b0dfd6b41ffec640249525625 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 09:55:31 -0700 Subject: [PATCH 16/18] Use radix sort for Shadow as well --- crates/bevy_pbr/Cargo.toml | 1 + crates/bevy_pbr/src/render/light.rs | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/crates/bevy_pbr/Cargo.toml b/crates/bevy_pbr/Cargo.toml index 37107c44d433a..46bf0f4e8c4f0 100644 --- a/crates/bevy_pbr/Cargo.toml +++ b/crates/bevy_pbr/Cargo.toml @@ -28,3 +28,4 @@ bevy_window = { path = "../bevy_window", version = "0.8.0-dev" } bitflags = "1.2" # direct dependency required for derive macro bytemuck = { version = "1", features = ["derive"] } +radsort = "0.1" diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index 63cf180e0e810..bf50d1a8e5fc3 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1421,6 +1421,11 @@ impl PhaseItem for Shadow { fn draw_function(&self) -> DrawFunctionId { self.draw_function } + + #[inline] + fn sort(items: &mut [Self]) { + radsort::sort_by_key(items, |item| item.distance); + } } impl EntityPhaseItem for Shadow { From 12dedb775be95efded8578dd8cf62e999bf30949 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 09:58:21 -0700 Subject: [PATCH 17/18] Add missing semicolon --- crates/bevy_core_pipeline/src/core_2d/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_core_pipeline/src/core_2d/mod.rs b/crates/bevy_core_pipeline/src/core_2d/mod.rs index 69ecc9332844a..4e1e8098e93c9 100644 --- a/crates/bevy_core_pipeline/src/core_2d/mod.rs +++ b/crates/bevy_core_pipeline/src/core_2d/mod.rs @@ -93,7 +93,7 @@ impl PhaseItem for Transparent2d { #[inline] fn sort(items: &mut [Self]) { - items.sort_by_key(|item| item.sort_key()) + items.sort_by_key(|item| item.sort_key()); } } From 373a8ec61f0cdc3aff01c142605139b0eb840624 Mon Sep 17 00:00:00 2001 From: James Liu Date: Thu, 23 Jun 2022 02:57:31 -0700 Subject: [PATCH 18/18] Fix doc comment Co-authored-by: Robert Swain --- crates/bevy_render/src/render_phase/draw.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index b4612ee446bd2..bb44126eb60d6 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -49,7 +49,7 @@ pub trait PhaseItem: Sized + Send + Sync + 'static { /// which uses a unstable sort, as this provides the best balance of CPU and GPU /// performance. /// - /// Implementors can optionally not sort the list at all. This is generally adviseble if and + /// Implementers can optionally not sort the list at all. This is generally advisable if and /// only if the renderer supports a depth prepass, which is by default not supported by /// the rest of Bevy's first party rendering crates. Even then, this may have a negative /// impact on GPU-side performance due to overdraw.