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.

Sign up for GitHub

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

Jump to bottom

Start Revive of Depth Prepass #5523

Closed
wants to merge 3 commits into from
Closed

Start Revive of Depth Prepass #5523

wants to merge 3 commits into from

Conversation

ChristopherBiscardi
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi commented Aug 1, 2022 •
edited
Loading

reviving: 9262c8f (#3072)

temp link to original discord thread: https://discord.com/channels/691052431525675048/1002129015936655361/1002480744360317008

Objective

Adds a depth pre-pass so that custom materials can access the depth buffer data when implementing effects such as drawing the intersection of an opaque and an alpha-blended object on the alpha blended object.

Solution

  • Describe the solution used to achieve the objective above.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • What changed as a result of this PR?
  • If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings
  • Stick to one or two sentences. If more detail is needed for a particular change, consider adding it to the "Solution" section
    • If you can't summarize the work, your change may be unreasonably large / unrelated. Consider splitting your PR to make it easier to review and merge!

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
  • Simply adding new functionality is not a breaking change.
  • Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

ChristopherBiscardi
ChristopherBiscardi commented Aug 1, 2022
View reviewed changes
crates/bevy_pbr/src/render/depth_prepass.wgsl
@@ -0,0 +1,104 @@
// NOTE: Keep in sync with pbr.wgsl
[[block]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: update syntax

@ChristopherBiscardi
Copy link
Contributor Author

ChristopherBiscardi commented Aug 1, 2022 •
edited
Loading

solved

Current issue is that I'm not sure if the RenderAsset trait should be implemented for StandardMaterial.

error[E0277]: the trait bound `pbr_material::StandardMaterial: RenderAsset` is not satisfied
   --> crates/bevy_pbr/src/render/depth_prepass.rs:415:27
    |
415 |     render_materials: Res<RenderAssets<StandardMaterial>>,
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `RenderAsset` is not implemented for `pbr_material::StandardMaterial`
    |
    = help: the following other types implement trait `RenderAsset`:
              Mesh
              bevy_render::texture::Image

Fixed with:

pub fn queue_depth_prepass_meshes<M: Material>(
    ...
    render_materials: Res<RenderMaterials<M>>,
)

ChristopherBiscardi
ChristopherBiscardi commented Aug 1, 2022
View reviewed changes
crates/bevy_pbr/src/render/depth_prepass.rs Outdated
@@ -0,0 +1,786 @@
use crate::{
draw_3d_graph, AlphaMode, MeshPipeline, MeshUniform, StandardMaterial, MeshViewBindGroup,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made TransformBindGroup, MeshViewBindGroup because the transform no longer exists. I'm not yet sure if that was the right succession.

ChristopherBiscardi
ChristopherBiscardi commented Aug 1, 2022
View reviewed changes
crates/bevy_pbr/src/render/depth_prepass.rs
impl FromWorld for DepthPrepassPipeline {
fn from_world(world: &mut World) -> Self {
let render_device = world.get_resource::<RenderDevice>().unwrap();
let mesh_pipeline = world.get_resource::<MeshPipeline>().unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PbrPipeline became MeshPipeline I think.

ChristopherBiscardi
ChristopherBiscardi commented Aug 1, 2022
View reviewed changes
crates/bevy_pbr/src/render/depth_prepass.rs
ty: BindingType::Buffer {
ty: BufferBindingType::Uniform,
has_dynamic_offset: false,
// TODO: change this to StandardMaterialUniformData::std140_size_static once crevice fixes this!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bevy uses encase now. This probably can be updated.

ChristopherBiscardi
ChristopherBiscardi commented Aug 1, 2022
View reviewed changes
crates/bevy_pbr/src/render/depth_prepass.rs
}


// TODO: ActiveCameras
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ActivesCameras are no longer a thing and the user is expected to track that themselves. I've subbed in "all 3d cameras" in the meantime.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Aug 1, 2022
@ChristopherBiscardi
Copy link
Contributor Author

The PR is almost compiling.

Two major outstanding issues are accessing the active 3d camera (ActiveCameras was removed in #4745) and some material related api changes since the original depth prepass pr used StandardMaterial and now we have the Material trait.

@ChristopherBiscardi
Copy link
Contributor Author

@superdump said they're choosing to pursue a different approach, so closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ChristopherBiscardi @alice-i-cecile