Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Extract common RenderPhase code into render method #7013

Conversation

kurtkuehnert
Copy link
Contributor

@kurtkuehnert kurtkuehnert commented Dec 23, 2022

Objective

All RenderPhases follow the same render procedure.
The same code is duplicated multiple times across the codebase.

Solution

I simply extracted this code into a method on the RenderPhase.
This avoids code duplication and makes setting up new RenderPhases easier.


Changelog

Changed

You can now set up the rendering code of a RenderPhase directly using the RenderPhase::render method, instead of implementing it manually in your render graph node.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Dec 23, 2022
@kurtkuehnert kurtkuehnert force-pushed the render_phase_common_render_method branch from 9286dc2 to e245a26 Compare December 23, 2022 22:26
kurtkuehnert added a commit to kurtkuehnert/bevy that referenced this pull request Dec 24, 2022
@kurtkuehnert kurtkuehnert force-pushed the render_phase_common_render_method branch from e245a26 to e13ec74 Compare December 24, 2022 09:26
@kurtkuehnert
Copy link
Contributor Author

Rebased this PR onto main. It should be easier to review now.

@james7132 james7132 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 Dec 26, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot pushed a commit that referenced this pull request Dec 27, 2022
# Objective

All `RenderPhases` follow the same render procedure.
The same code is duplicated multiple times across the codebase.

## Solution

I simply extracted this code into a method on the `RenderPhase`. 
This avoids code duplication and makes setting up new `RenderPhases` easier.

---

## Changelog

### Changed

You can now set up the rendering code of a `RenderPhase` directly using the `RenderPhase::render` method, instead of implementing it manually in your render graph node.
@bors bors bot changed the title Extract common RenderPhase code into render method [Merged by Bors] - Extract common RenderPhase code into render method Dec 27, 2022
@bors bors bot closed this Dec 27, 2022
@hymm hymm mentioned this pull request Dec 28, 2022
bors bot pushed a commit that referenced this pull request Jan 2, 2023
# Objective

- The recently merged PR #7013 does not allow multiple `RenderPhase`s to share the same `RenderPass`.
- Due to the introduced overhead we want to minimize the number of `RenderPass`es recorded during each frame.

## Solution

- Take a constructed `TrackedRenderPass` instead of a `RenderPassDiscriptor` as a parameter to the `RenderPhase::render` method.

---

## Changelog

To enable multiple `RenderPhases` to share the same `TrackedRenderPass`,
the `RenderPhase::render` signature has changed.

```rust
pub fn render<'w>(
  &self,
  render_pass: &mut TrackedRenderPass<'w>,
  world: &'w World,
  view: Entity)
```


Co-authored-by: Kurt Kühnert <51823519+kurtkuehnert@users.noreply.github.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

All `RenderPhases` follow the same render procedure.
The same code is duplicated multiple times across the codebase.

## Solution

I simply extracted this code into a method on the `RenderPhase`. 
This avoids code duplication and makes setting up new `RenderPhases` easier.

---

## Changelog

### Changed

You can now set up the rendering code of a `RenderPhase` directly using the `RenderPhase::render` method, instead of implementing it manually in your render graph node.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…ine#7043)

# Objective

- The recently merged PR bevyengine#7013 does not allow multiple `RenderPhase`s to share the same `RenderPass`.
- Due to the introduced overhead we want to minimize the number of `RenderPass`es recorded during each frame.

## Solution

- Take a constructed `TrackedRenderPass` instead of a `RenderPassDiscriptor` as a parameter to the `RenderPhase::render` method.

---

## Changelog

To enable multiple `RenderPhases` to share the same `TrackedRenderPass`,
the `RenderPhase::render` signature has changed.

```rust
pub fn render<'w>(
  &self,
  render_pass: &mut TrackedRenderPass<'w>,
  world: &'w World,
  view: Entity)
```


Co-authored-by: Kurt Kühnert <51823519+kurtkuehnert@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

All `RenderPhases` follow the same render procedure.
The same code is duplicated multiple times across the codebase.

## Solution

I simply extracted this code into a method on the `RenderPhase`. 
This avoids code duplication and makes setting up new `RenderPhases` easier.

---

## Changelog

### Changed

You can now set up the rendering code of a `RenderPhase` directly using the `RenderPhase::render` method, instead of implementing it manually in your render graph node.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…ine#7043)

# Objective

- The recently merged PR bevyengine#7013 does not allow multiple `RenderPhase`s to share the same `RenderPass`.
- Due to the introduced overhead we want to minimize the number of `RenderPass`es recorded during each frame.

## Solution

- Take a constructed `TrackedRenderPass` instead of a `RenderPassDiscriptor` as a parameter to the `RenderPhase::render` method.

---

## Changelog

To enable multiple `RenderPhases` to share the same `TrackedRenderPass`,
the `RenderPhase::render` signature has changed.

```rust
pub fn render<'w>(
  &self,
  render_pass: &mut TrackedRenderPass<'w>,
  world: &'w World,
  view: Entity)
```


Co-authored-by: Kurt Kühnert <51823519+kurtkuehnert@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants