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

Prepass cleanup #1

Merged
merged 4 commits into from
Oct 13, 2022
Merged

Prepass cleanup #1

merged 4 commits into from
Oct 13, 2022

Conversation

JMS55
Copy link

@JMS55 JMS55 commented Oct 13, 2022

  • Cleanup and make consistent the names of various resources.
    • Rename DepthPrepass to just Prepass for various resources.
    • Not sure if it was correct to rename DEPTH_PREPASS_FORMAT to PREPASS_FORMAT in prepass.rs. I'm not sure I understand what the depth stencil texture is used for.
  • Add PrepassPlugin within MaterialPlugin (not sure if I did this correctly).
  • Impl Default for PrepassSettings, with everything enabled.
  • Improve PrepassSettings documentation.
  • Change info!() to warn!() when missing prepass shaders, better error messages.
  • Removed PrepassSettings from the 3d_scene example, as it wasn't used.

Copy link
Owner

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Couple of nits, but otherwise looks good.

crates/bevy_pbr/src/lib.rs Outdated Show resolved Hide resolved
|| key.contains(MeshPipelineKey::ALPHA_MASK)
{
let frag_shader_handle = if let Some(handle) = &self.material_fragment_shader {
handle.clone()
} else {
info!("no frag");
DEPTH_PREPASS_SHADER_HANDLE.typed::<Shader>()
warn!("Missing Material::prepass_fragment_shader() for material with UUID {}. Rendering may be incorrect.", M::TYPE_UUID);
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, my old info log was just for debugging stuff. I'm not sure it's actually a mistake to not specify a custom prepass shader. Or at least, I'm not sure it deserves a warn, people just writing some simple materials shouldn't need to think about this I think. Maybe just log it as a trace instead?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I'm not sure about this either. The shader/material stuff isn't clear to me, so I don't know if this is actually ever going to be a problem. We discussed issues with custom materials overwriting fragment/vertex shaders, and how that plays into writing normals, and how that interacts with alpha masks, but yeah. I'm not sure.

examples/3d/3d_scene.rs Outdated Show resolved Hide resolved
examples/shader/shader_material.rs Show resolved Hide resolved
@IceSentry IceSentry merged commit 0b9f67b into IceSentry:depth-prepass Oct 13, 2022
IceSentry pushed a commit that referenced this pull request Jan 2, 2023
IceSentry pushed a commit that referenced this pull request Feb 26, 2023
IceSentry pushed a commit that referenced this pull request Jun 26, 2024
…3905)

# Objective

- first part of bevyengine#13900 

## Solution

- split `check_light_mesh_visibility `into
`check_dir_light_mesh_visibility `and
`check_point_light_mesh_visibility` for better review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants