[Merged by Bors] - Improve render phase documentation by kurtkuehnert · Pull Request #7016 · bevyengine/bevy

kurtkuehnert · 2022-12-23T22:33:31Z

Objective

The documentation of the bevy_render crate is still pretty incomplete.
This PR follows up on #6885 and improves the documentation of the render_phase module.
This module contains one of our most important rendering abstractions and the current documentation is pretty confusing. This PR tries to clarify what all of these pieces are for and how they work together to form bevy`s modular rendering logic.

Solution

Code Reformating

I have moved the rangefinder into the render_phase module since it is only used there.
I have moved the PhaseItem (and the BatchedPhaseItem) from render_phase::draw over to render_phase::mod. This does not change the public-facing API since they are reexported anyway, but this change makes the relation between RenderPhase and PhaseItem clear and easier to discover.

Documentation

revised all documentation in the render_phase module
added a module-level explanation of how RenderPhases, RenderPasses, PhaseItems, Draw functions, and RenderCommands relate to each other and how they are used

Changelog

The rangefinder module has been moved into the render_phase module.

Migration Guide

The rangefinder module has been moved into the render_phase module.

//old
use bevy::render::rangefinder::*;

// new
use bevy::render::render_phase::rangefinder::*;

james7132 · 2022-12-23T23:47:21Z

@kurtkuehnert Do you think you can isolate this from the other PRs? Chained PRs like this tend to be a nightmare to contend with in terms of merge conflicts, and even harder to review while those other PRs are in flux.

kurtkuehnert · 2022-12-24T00:11:22Z

I could probably separate this PR from ~~#7017~~ #7013 due to its small scope. But this PR largely depends on your #6885 and is meant to be merged after it. I could split them, but I fear that this would lead to inconsistencies, which would require further PRs to fix. It should be possible to rebase this PR if/once #6885 is merged.

joseph-gio · 2022-12-24T00:12:47Z

I could probably separate this PR from #7017

Did you mean #7013?

kurtkuehnert · 2022-12-24T00:24:54Z

@JoJoJet Yes.

kurtkuehnert · 2022-12-24T09:11:35Z

@james7132 This PR now only depends on #6885 and should be rebased+merged once your PR has been merged.

kurtkuehnert · 2022-12-24T09:14:42Z

For now, reviewers can just take a look at commit 384a287 following.

james7132 · 2023-01-04T01:42:59Z

@kurtkuehnert with #6885 merged, can you rebase this PR so it can be reviewed?

kurtkuehnert · 2023-01-04T10:45:26Z

@james7132 I have rebased this PR onto main.
It is now ready for review.

james7132

Just a cursory run through. Will give a thorough review later.

crates/bevy_render/src/render_phase/draw_state.rs

crates/bevy_render/src/render_phase/mod.rs

crates/bevy_render/src/render_phase/draw.rs

crates/bevy_render/src/render_phase/draw_state.rs

james7132 · 2023-01-05T19:38:48Z

crates/bevy_render/src/render_phase/mod.rs

Suggested change

/// A render phase sorts and renders all [`PhaseItem`]s (entities) that are assigned to it.

/// A collection of all rendering commands for a single rendering phase for a single view.

///

/// These commands are included as individual [`PhaseItem`] that encodes how to draw one ore more

/// entities to the screen.

It is not really collecting rendering commands, but rather PhaseItems.
How about

/// A collection of all [`PhaseItem`]s for a single rendering phase for a single view.

In a way, each phase item is a bundle of rendering commands. As a summary of what the type does, I'd rather us avoid referential descriptions, especially when it's not clear to the normal user what a PhaseItem is from a glance.

I use the term rendering instructions instead of rendering commands because the latter are stored in DrawFunctionsInternal instead. I think this is a good compromise. Do you agree?

/// A collection of all rendering instructions, that will be executed by the GPU, for a /// single rendering phase for a single view.

Works for me, though it could be a bit confusing since "instructions" usually imply CPU/GPU machine instructions.

Mhh, yeah both are not ideal. I will change it back to commands if other reviewers agree that this is the better term. 👍

crates/bevy_render/src/render_phase/draw.rs

alice-i-cecile

Generally a clear improvement.

Some doc suggestions, and TODO comments should be moved into issues instead so they can be better tracked and discussed.

crates/bevy_render/src/render_phase/draw.rs

crates/bevy_render/src/render_phase/draw_state.rs

crates/bevy_render/src/render_phase/mod.rs

djeedai

Mostly OK for me, some minor corrections.

crates/bevy_render/src/render_phase/draw.rs

crates/bevy_render/src/render_phase/draw_state.rs

crates/bevy_render/src/render_phase/mod.rs

alice-i-cecile · 2023-01-11T21:18:31Z

If you would like, then I can extend the documentation with further details tomorrow.

Once this is in I intend to merge this PR :)

crates/bevy_render/src/render_phase/draw.rs

kurtkuehnert · 2023-01-12T09:38:10Z

Once this is in I intend to merge this PR :)

@alice-i-cecile I have addressed the feedback :).

alice-i-cecile · 2023-01-12T15:11:38Z

bors r+

# Objective The documentation of the bevy_render crate is still pretty incomplete. This PR follows up on #6885 and improves the documentation of the `render_phase` module. This module contains one of our most important rendering abstractions and the current documentation is pretty confusing. This PR tries to clarify what all of these pieces are for and how they work together to form bevy`s modular rendering logic. ## Solution ### Code Reformating - I have moved the `rangefinder` into the `render_phase` module since it is only used there. - I have moved the `PhaseItem` (and the `BatchedPhaseItem`) from `render_phase::draw` over to `render_phase::mod`. This does not change the public-facing API since they are reexported anyway, but this change makes the relation between `RenderPhase` and `PhaseItem` clear and easier to discover. ### Documentation - revised all documentation in the `render_phase` module - added a module-level explanation of how `RenderPhase`s, `RenderPass`es, `PhaseItem`s, `Draw` functions, and `RenderCommands` relate to each other and how they are used --- ## Changelog - The `rangefinder` module has been moved into the `render_phase` module. ## Migration Guide - The `rangefinder` module has been moved into the `render_phase` module. ```rust //old use bevy::render::rangefinder::*; // new use bevy::render::render_phase::rangefinder::*; ```

bors · 2023-01-12T15:50:49Z

Pull request successfully merged into main.

Build succeeded:

# Objective The documentation of the bevy_render crate is still pretty incomplete. This PR follows up on bevyengine#6885 and improves the documentation of the `render_phase` module. This module contains one of our most important rendering abstractions and the current documentation is pretty confusing. This PR tries to clarify what all of these pieces are for and how they work together to form bevy`s modular rendering logic. ## Solution ### Code Reformating - I have moved the `rangefinder` into the `render_phase` module since it is only used there. - I have moved the `PhaseItem` (and the `BatchedPhaseItem`) from `render_phase::draw` over to `render_phase::mod`. This does not change the public-facing API since they are reexported anyway, but this change makes the relation between `RenderPhase` and `PhaseItem` clear and easier to discover. ### Documentation - revised all documentation in the `render_phase` module - added a module-level explanation of how `RenderPhase`s, `RenderPass`es, `PhaseItem`s, `Draw` functions, and `RenderCommands` relate to each other and how they are used --- ## Changelog - The `rangefinder` module has been moved into the `render_phase` module. ## Migration Guide - The `rangefinder` module has been moved into the `render_phase` module. ```rust //old use bevy::render::rangefinder::*; // new use bevy::render::render_phase::rangefinder::*; ```

IceSentry added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Dec 23, 2022

james7132 requested review from james7132 and superdump December 23, 2022 23:47

kurtkuehnert force-pushed the improve_render_phase_documentation branch from 9286dc2 to 30d3f3d Compare December 24, 2022 09:09

improved documentation

218a848

kurtkuehnert force-pushed the improve_render_phase_documentation branch from 70d3233 to 78195de Compare January 4, 2023 10:28

james7132 requested changes Jan 4, 2023

View reviewed changes

crates/bevy_render/src/render_phase/draw_state.rs Show resolved Hide resolved

crates/bevy_render/src/render_phase/mod.rs Show resolved Hide resolved

james7132 requested a review from alice-i-cecile January 4, 2023 18:15

kurtkuehnert commented Jan 5, 2023

View reviewed changes

crates/bevy_render/src/render_phase/draw.rs Show resolved Hide resolved

james7132 requested changes Jan 5, 2023

View reviewed changes

kurtkuehnert force-pushed the improve_render_phase_documentation branch from d37e989 to a641ca1 Compare January 8, 2023 09:37

alice-i-cecile reviewed Jan 11, 2023

View reviewed changes

crates/bevy_render/src/render_phase/draw.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jan 11, 2023

View reviewed changes

crates/bevy_render/src/render_phase/draw.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Jan 11, 2023

View reviewed changes

addressed feedback

d7e2357

kurtkuehnert force-pushed the improve_render_phase_documentation branch from bce5792 to d7e2357 Compare January 11, 2023 13:04

alice-i-cecile approved these changes Jan 11, 2023

View reviewed changes

alice-i-cecile requested review from djeedai and james7132 January 11, 2023 15:58

djeedai approved these changes Jan 11, 2023

View reviewed changes

improved docs

bbacd91

IceSentry reviewed Jan 11, 2023

View reviewed changes

crates/bevy_render/src/render_phase/mod.rs Show resolved Hide resolved

IceSentry reviewed Jan 11, 2023

View reviewed changes

crates/bevy_render/src/render_phase/draw.rs Outdated Show resolved Hide resolved

IceSentry approved these changes Jan 11, 2023

View reviewed changes

improved docs

976892a

kurtkuehnert mentioned this pull request Jan 12, 2023

Merge Draw function and RenderCommand #7172

Closed

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 12, 2023

bors bot changed the title ~~Improve render phase documentation~~ [Merged by Bors] - Improve render phase documentation Jan 12, 2023

bors bot closed this Jan 12, 2023

-/// A render phase sorts and renders all [`PhaseItem`]s (entities) that are assigned to it.
+/// A collection of all rendering commands for a single rendering phase for a single view.
+///
+/// These commands are included as individual [`PhaseItem`] that encodes how to draw one ore more
+/// entities to the screen.

Uh oh!

Conversation

kurtkuehnert commented Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Code Reformating

Documentation

Changelog

Migration Guide

Uh oh!

james7132 commented Dec 23, 2022

Uh oh!

kurtkuehnert commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-gio commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtkuehnert commented Dec 24, 2022

Uh oh!

kurtkuehnert commented Dec 24, 2022

Uh oh!

kurtkuehnert commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james7132 commented Jan 4, 2023

Uh oh!

kurtkuehnert commented Jan 4, 2023

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james7132 Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

kurtkuehnert Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

kurtkuehnert Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james7132 Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

kurtkuehnert Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djeedai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kurtkuehnert commented Dec 23, 2022 •

edited

Loading

kurtkuehnert commented Dec 24, 2022 •

edited

Loading

joseph-gio commented Dec 24, 2022 •

edited

Loading

kurtkuehnert commented Dec 24, 2022 •

edited

Loading

kurtkuehnert Jan 8, 2023 •

edited

Loading