Skip to content

Conversation

@JMS55
Copy link
Contributor

@JMS55 JMS55 commented Mar 2, 2023

Objective

Changelog

  • Removed SetShadowViewBindGroup, queue_shadow_view_bind_group(), and LightMeta::shadow_view_bind_group in favor of reusing the prepass view bind group.

Migration Guide

  • Removed SetShadowViewBindGroup, queue_shadow_view_bind_group(), and LightMeta::shadow_view_bind_group in favor of reusing the prepass view bind group.

@JMS55 JMS55 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Mar 2, 2023
@JMS55 JMS55 added this to the 0.10 milestone Mar 2, 2023
@james7132 james7132 added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@james7132 james7132 requested a review from superdump March 2, 2023 20:34
Copy link
Contributor

@geieredgar geieredgar left a comment

Choose a reason for hiding this comment

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

This looks good, I am sorry for missing that queue_shadow_view_bind_group doesn't do anything anymore and for not removing SetShadowViewBindGroup together with DrawShadowMesh. There is also another issue with my changes, for which I am currently writing a PR see #7878.

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 Mar 2, 2023
# Objective

- Remove dead code after #7784

# Changelog
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.

# Migration Guide
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.
@bors
Copy link
Contributor

bors bot commented Mar 2, 2023

Timed out.

@james7132
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Mar 2, 2023
# Objective

- Remove dead code after #7784

# Changelog
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.

# Migration Guide
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.
@bors bors bot changed the title Remove dead code after #7784 [Merged by Bors] - Remove dead code after #7784 Mar 2, 2023
@bors bors bot closed this Mar 2, 2023
bors bot pushed a commit that referenced this pull request Mar 3, 2023
# Objective

Unfortunately, there are three issues with my changes introduced by #7784.

1.  The changes left some dead code. This is already taken care of here: #7875.
2. Disabling prepass causes failures because the shadow mapping relies on the `PrepassPlugin` now.
3. Custom materials use the `prepass.wgsl` shader, but this does not always define a fragment entry point.

This PR fixes 2. and 3. and resolves #7879.

## Solution

- Add a regression test with disabled prepass.
- Split `PrepassPlugin` into two plugins:
  - `PrepassPipelinePlugin` contains the part that is required for the shadow mapping to work and is unconditionally added.
  - `PrepassPlugin` now only adds the systems and resources required for the "real" prepasses.
- Add a noop fragment entry point to `prepass.wgsl`, used if `NORMAL_PASS` is not defined.


Co-authored-by: Edgar Geier <[email protected]>
ProfLander pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
# Objective

- Remove dead code after bevyengine#7784

# Changelog
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.

# Migration Guide
- Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group.
ProfLander pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
…yengine#7878)

# Objective

Unfortunately, there are three issues with my changes introduced by bevyengine#7784.

1.  The changes left some dead code. This is already taken care of here: bevyengine#7875.
2. Disabling prepass causes failures because the shadow mapping relies on the `PrepassPlugin` now.
3. Custom materials use the `prepass.wgsl` shader, but this does not always define a fragment entry point.

This PR fixes 2. and 3. and resolves bevyengine#7879.

## Solution

- Add a regression test with disabled prepass.
- Split `PrepassPlugin` into two plugins:
  - `PrepassPipelinePlugin` contains the part that is required for the shadow mapping to work and is unconditionally added.
  - `PrepassPlugin` now only adds the systems and resources required for the "real" prepasses.
- Add a noop fragment entry point to `prepass.wgsl`, used if `NORMAL_PASS` is not defined.


Co-authored-by: Edgar Geier <[email protected]>
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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants