addressed feedback

Author: kurtkuehnert

crates/bevy_core_pipeline/src/core_3d/mod.rs

@@ -29,13 +29,8 @@ use bevy_render::{
     prelude::Msaa,
     render_graph::{EmptyNode, RenderGraph, SlotInfo, SlotType},
     render_phase::{
-<<<<<<< HEAD
         sort_phase_system, CachedRenderPipelinePhaseItem, DrawFunctionId, DrawFunctions, PhaseItem,
         RenderPhase,
-=======
-        sort_phase_system, CachedRenderPipelinePhaseItem, DrawFunctionId, DrawFunctions,
-        PhaseItem, RenderPhase,
->>>>>>> b411da63f (Flatten EntityPhaseItem into PhaseItem)
     },
     render_resource::{
         CachedRenderPipelineId, Extent3d, TextureDescriptor, TextureDimension, TextureFormat,

crates/bevy_render/src/render_phase/draw.rs

@@ -20,17 +20,18 @@ use std::{any::TypeId, fmt::Debug, hash::Hash};
 // Todo: consider renaming this to `DrawFunction`
 /// A draw function used to draw [`PhaseItem`]s.
 ///
-/// Therefore the draw function can retrieve and query the required ECS data from the render world.
+/// The the draw function can retrieve and query the required ECS data from the render world.
 ///
-/// This trait can either be implemented directly or implicitly composed out of multiple modular [`RenderCommand`]s.
+/// This trait can either be implemented directly or implicitly composed out of multiple modular
+/// [`RenderCommand`]s. For more details and an example see the [`RenderCommand`] documentation.
 pub trait Draw<P: PhaseItem>: Send + Sync + 'static {
     /// Prepares the draw function to be used. This is called once and only once before the phase
-    /// begins. There may be zero or more `draw` calls following a call to this function..
+    /// begins. There may be zero or more `draw` calls following a call to this function.
     /// Implementing this is optional.
     #[allow(unused_variables)]
     fn prepare(&mut self, world: &'_ World) {}
 
-    /// Draws a [`PhaseItem`] by issuing one or more draw calls via the [`TrackedRenderPass`].
+    /// Draws a [`PhaseItem`] by issuing zero or more `draw` calls draw calls via the [`TrackedRenderPass`].
     fn draw<'w>(
         &mut self,
         world: &'w World,
@@ -54,7 +55,7 @@ pub struct DrawFunctionsInternal<P: PhaseItem> {
 }
 
 impl<P: PhaseItem> DrawFunctionsInternal<P> {
-    // Todo: add comment
+    /// Prepares all draw function. This is called once and only once before the phase begins.
     pub fn prepare(&mut self, world: &World) {
         for function in &mut self.draw_functions {
             function.prepare(world);
@@ -132,13 +133,19 @@ impl<P: PhaseItem> DrawFunctions<P> {
     }
 }
 
-/// [`RenderCommand`]s are modular standardized pieces of render logic that can be composed into [`Draw`] functions.
+/// [`RenderCommand`]s are modular standardized pieces of render logic that can be composed into
+/// [`Draw`] functions.
 ///
-/// To turn a stateless render command into a usable draw function it has to be wrapped by a [`RenderCommandState`].
-/// This is done automatically when registering a render command as a [`Draw`] function via the [`AddRenderCommand::add_render_command`] method.
+/// To turn a stateless render command into a usable draw function it has to be wrapped by a
+/// [`RenderCommandState`].
+/// This is done automatically when registering a render command as a [`Draw`] function via the
+/// [`AddRenderCommand::add_render_command`] method.
 ///
-/// Compared to the draw function the required ECS data is fetched automatically (by the [`RenderCommandState`]) from the render world.
-/// Therefore the three types [`Param`](RenderCommand::Param), [`ViewWorldQuery`](RenderCommand::ViewWorldQuery) and [`WorldQuery`](RenderCommand::WorldQuery) are used.
+/// Compared to the draw function the required ECS data is fetched automatically
+/// (by the [`RenderCommandState`]) from the render world.
+/// Therefore the three types [`Param`](RenderCommand::Param),
+/// [`ViewWorldQuery`](RenderCommand::ViewWorldQuery) and
+/// [`ItemWorldQuery`](RenderCommand::ItemWorldQuery) are used.
 /// They specify which information is required to execute the render command.
 ///
 /// Multiple render commands can be combined together by wrapping them in a tuple.
@@ -157,15 +164,18 @@ impl<P: PhaseItem> DrawFunctions<P> {
 /// );
 /// ```
 pub trait RenderCommand<P: PhaseItem> {
-    /// Specifies all ECS data required by [`RenderCommand::render`].
+    /// Specifies the general ECS data (e.g. resources) required by [`RenderCommand::render`].
     /// All parameters have to be read only.
     type Param: SystemParam + 'static;
-    // Todo: add comment
+    /// Specifies the ECS data of the view required by [`RenderCommand::render`].
+    /// All components have to be accessed read only.
     type ViewWorldQuery: ReadOnlyWorldQuery;
-    // Todo: add comment
+    /// Specifies the ECS data of the item required by [`RenderCommand::render`].
+    /// All components have to be accessed read only.
     type ItemWorldQuery: ReadOnlyWorldQuery;
 
-    /// Renders a [`PhaseItem`] by recording commands (e.g. setting pipelines, binding bind groups, issuing draw calls, etc.) via the [`TrackedRenderPass`].
+    /// Renders a [`PhaseItem`] by recording commands (e.g. setting pipelines, binding bind groups,
+    /// issuing draw calls, etc.) via the [`TrackedRenderPass`].
     fn render<'w>(
         item: &P,
         view: ROQueryItem<'w, Self::ViewWorldQuery>,
@@ -208,7 +218,9 @@ macro_rules! render_command_tuple_impl {
 all_tuples!(render_command_tuple_impl, 0, 15, C, V, E);
 
 /// Wraps a [`RenderCommand`] into a state so that it can be used as a [`Draw`] function.
-/// Therefore the [`RenderCommand::Param`], [`RenderCommand::ViewWorldQuery`] and [`RenderCommand::WorldQuery`] are queried from the ECS and passed to the command.
+///
+/// The [`RenderCommand::Param`], [`RenderCommand::ViewWorldQuery`] and
+/// [`RenderCommand::ItemWorldQuery`] are fetched from the ECS and passed to the command.
 pub struct RenderCommandState<P: PhaseItem + 'static, C: RenderCommand<P>> {
     state: SystemState<C::Param>,
     view: QueryState<C::ViewWorldQuery>,
@@ -230,7 +242,8 @@ impl<P: PhaseItem, C: RenderCommand<P> + Send + Sync + 'static> Draw<P> for Rend
 where
     C::Param: ReadOnlySystemParam,
 {
-    // Todo: add comment
+    /// Prepares the render command to be used. This is called once and only once before the phase
+    /// begins. There may be zero or more `draw` calls following a call to this function.
     fn prepare(&mut self, world: &'_ World) {
         self.view.update_archetypes(world);
         self.entity.update_archetypes(world);
@@ -247,7 +260,7 @@ where
         let param = self.state.get(world);
         let view = self.view.get_manual(world, view).unwrap();
         let entity = self.entity.get_manual(world, item.entity()).unwrap();
-        // Todo: handle/log `RenderCommand` failure
+        // TODO: handle/log `RenderCommand` failure
         C::render(item, view, entity, param, pass);
     }
 }
@@ -286,7 +299,7 @@ impl AddRenderCommand for App {
     }
 }
 
-// Todo: If this is always needed, combine this with PhaseItem?
+// TODO: If this is always needed, combine this with PhaseItem?
 pub trait CachedRenderPipelinePhaseItem: PhaseItem {
     fn cached_pipeline(&self) -> CachedRenderPipelineId;
 }
@@ -297,8 +310,7 @@ pub struct SetItemPipeline;
 impl<P: CachedRenderPipelinePhaseItem> RenderCommand<P> for SetItemPipeline {
     type Param = SRes<PipelineCache>;
     type ViewWorldQuery = ();
-    type WorldQuery = ();
-
+    type ItemWorldQuery = ();
     #[inline]
     fn render<'w>(
         item: &P,

crates/bevy_render/src/render_phase/draw_state.rs

@@ -12,7 +12,9 @@ use wgpu::{IndexFormat, RenderPass};
 
 /// Tracks the state of a [`TrackedRenderPass`].
 ///
-/// This is used to skip redundant operations on the [`TrackedRenderPass`] (e.g. setting an already set pipeline, binding an already bound bind group).
+/// This is used to skip redundant operations on the [`TrackedRenderPass`] (e.g. setting an already
+/// set pipeline, binding an already bound bind group). These operations can otherwise be fairly
+/// costly due to IO to the GPU, so deduplicating these calls results in a speedup.
 #[derive(Debug, Default)]
 pub struct DrawState {
     pipeline: Option<RenderPipelineId>,

crates/bevy_render/src/render_phase/mod.rs

@@ -1,31 +1,44 @@
-//! The modular rendering abstractions that queue, prepare, sort and draw entities as part of separate phases.
+//! A modular rendering abstraction responsible for queuing, preparing, sorting and drawing entities
+//! as part of separate phases.
 //!
-//! To draw an entity, a corresponding [`PhaseItem`] has to be added to one of the renderers multiple [`RenderPhase`]s (e.g. opaque, transparent, shadow, etc).
+//! To draw an entity, a corresponding [`PhaseItem`] has to be added to one of the renderers
+//! multiple [`RenderPhase`]s (e.g. opaque, transparent, shadow, etc).
 //! This must be done in the [`RenderStage::Queue`](crate::RenderStage::Queue).
-//! After that the [`RenderPhase`] sorts them in the [`RenderStage::PhaseSort`](crate::RenderStage::PhaseSort).
-//! Finally the [`PhaseItem`]s are rendered using a single [`TrackedRenderPass`], during the [`RenderStage::Render`](crate::RenderStage::Render).
+//! After that the [`RenderPhase`] sorts them in the
+//! [`RenderStage::PhaseSort`](crate::RenderStage::PhaseSort).
+//! Finally the [`PhaseItem`]s are rendered using a single [`TrackedRenderPass`], during the
+//! [`RenderStage::Render`](crate::RenderStage::Render).
 //!
 //! Therefore each [`PhaseItem`] is assigned a [`Draw`] function.
-//! These set up the state of the [`TrackedRenderPass`] (i.e. select the [`RenderPipeline`](crate::render_resource::RenderPipeline), configure the [`BindGroup`](crate::render_resource::BindGroup)s, etc.) and then issue a draw call, for the corresponding [`PhaseItem`].
+//! These set up the state of the [`TrackedRenderPass`] (i.e. select the
+//! [`RenderPipeline`](crate::render_resource::RenderPipeline), configure the
+//! [`BindGroup`](crate::render_resource::BindGroup)s, etc.) and then issue a draw call,
+//! for the corresponding [`PhaseItem`].
 //!
-//! The [`Draw`] function trait can either be implemented directly or such a function can be created by composing multiple [`RenderCommand`]s.
+//! The [`Draw`] function trait can either be implemented directly or such a function can be
+//! created by composing multiple [`RenderCommand`]s.
 
 mod draw;
 mod draw_state;
 mod rangefinder;
 
-use bevy_ecs::entity::Entity;
 pub use draw::*;
 pub use draw_state::*;
 pub use rangefinder::*;
 
 use bevy_ecs::prelude::*;
-use bevy_ecs::world::World;
 use std::ops::Range;
 
-/// A render phase sorts and renders all [`PhaseItem`]s (entities) that are assigned to it.
+/// A collection of all rendering instructions, that will be executed by the GPU, for a
+/// single rendering phase for a single view.
 ///
-/// It corresponds to exactly one [`TrackedRenderPass`] and thus renders all items into the same texture attachments (e.g. color, depth, stencil).
+/// Each view (camera, or shadow-casting light, etc.) can have one or multiple render phases.
+/// They are used to queue entities for rendering.
+/// Multiple phases might be required due to different sorting/batching behaviours
+/// (e.g. opaque: front to back, transparent: back to front) or because one phase depends on
+/// the rendered texture of the previous phase (e.g. for screen-space reflections).
+/// All [`PhaseItem`]s are then rendered using a single [`TrackedRenderPass`].
+/// The render pass might be reused for multiple phases to reduce GPU overhead.
 #[derive(Component)]
 pub struct RenderPhase<I: PhaseItem> {
     pub items: Vec<I>,
@@ -49,6 +62,7 @@ impl<I: PhaseItem> RenderPhase<I> {
         I::sort(&mut self.items);
     }
 
+    /// Renders all of its [`PhaseItem`]s using their corresponding draw functions.
     pub fn render<'w>(
         &self,
         render_pass: &mut TrackedRenderPass<'w>,
@@ -93,12 +107,13 @@ impl<I: BatchedPhaseItem> RenderPhase<I> {
     }
 }
 
-/// An item (entity) which will be drawn to the screen.
+/// An item (entity) which will be drawn to a texture or the screen, as part of a [`RenderPhase`].
 ///
-/// A phase item has to be queued up for rendering during the [`RenderStage::Queue`](crate::RenderStage::Queue) stage.
+/// A phase item has to be queued up for rendering during the
+/// [`RenderStage::Queue`](crate::RenderStage::Queue).
 /// Afterwards it will be sorted and rendered automatically in the
-/// [`RenderStage::PhaseSort`](crate::RenderStage::PhaseSort) stage and
-/// [`RenderStage::Render`](crate::RenderStage::Render) stage, respectively.
+/// [`RenderStage::PhaseSort`](crate::RenderStage::PhaseSort) and
+/// [`RenderStage::Render`](crate::RenderStage::Render), respectively.
 pub trait PhaseItem: Sized + Send + Sync + 'static {
     /// The type used for ordering the items. The smallest values are drawn first.
     /// This order can be calculated using the [`ViewRangefinder3d`],