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

Fix incorrect function calls to hsv_to_rgb in render debug code. #14260

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

Soulghost
Copy link
Contributor

@Soulghost Soulghost commented Jul 10, 2024

Objective

Solution

  • correct the input parameters at these call sites.

Testing

  1. Use a 3D scene example with PBR lighting and shadows enabled, such as the shadow_caster_receiver and load_gltf example, for testing.
  2. Enable relevant shader defines in crates/bevy_pbr/src/pbr_material.rs for the StandardMaterial.
impl Material for StandardMaterial {
    // ...
    fn specialize(
            _pipeline: &MaterialPipeline<Self>,
            descriptor: &mut RenderPipelineDescriptor,
            _layout: &MeshVertexBufferLayoutRef,
            key: MaterialPipelineKey<Self>,
        ) -> Result<(), SpecializedMeshPipelineError> {
            // ...
            // shader_defs.push("CLUSTERED_FORWARD_DEBUG_Z_SLICES".into());
            // shader_defs.push("CLUSTERED_FORWARD_DEBUG_CLUSTER_COHERENCY".into());
            shader_defs.push("DIRECTIONAL_LIGHT_SHADOW_MAP_DEBUG_CASCADES".into());
            // ...
    }
}

Showcase

CLUSTERED_FORWARD_DEBUG_Z_SLICES

  • example: examples/3d/shadow_caster_receiver.rs
    Screenshot2024_07_10_143150

CLUSTERED_FORWARD_DEBUG_CLUSTER_COHERENCY

  • example: examples/3d/shadow_caster_receiver.rs
    Screenshot2024_07_10_143312

DIRECTIONAL_LIGHT_SHADOW_MAP_DEBUG_CASCADES

For this one, we need to use a large scene and modity the CascadeShadowConfigBuilder, here is a simple patch for the load_gltf example:

diff --git a/examples/3d/load_gltf.rs b/examples/3d/load_gltf.rs
index 358446238..9403aa288 100644
--- a/examples/3d/load_gltf.rs
+++ b/examples/3d/load_gltf.rs
@@ -18,7 +18,7 @@ fn main() {
 fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
     commands.spawn((
         Camera3dBundle {
-            transform: Transform::from_xyz(0.7, 0.7, 1.0)
+            transform: Transform::from_xyz(0.7, 0.7, 2.0)
                 .looking_at(Vec3::new(0.0, 0.3, 0.0), Vec3::Y),
             ..default()
         },
@@ -39,30 +39,40 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
         // We also adjusted the shadow map to be larger since we're
         // only using a single cascade.
         cascade_shadow_config: CascadeShadowConfigBuilder {
-            num_cascades: 1,
-            maximum_distance: 1.6,
+            num_cascades: 5,
+            maximum_distance: 20.0,
             ..default()
         }
         .into(),
         ..default()
     });
+
     commands.spawn(SceneBundle {
         scene: asset_server
             .load(GltfAssetLabel::Scene(0).from_asset("models/FlightHelmet/FlightHelmet.gltf")),
         ..default()
     });
+
+    for i in 1..=10 {
+        commands.spawn(SceneBundle {
+            scene: asset_server
+                .load(GltfAssetLabel::Scene(0).from_asset("models/FlightHelmet/FlightHelmet.gltf")),
+            transform: Transform::from_xyz(i as f32 * 0.5, 0.0, i as f32 * -2.0),
+            ..default()
+        });
+    }
 }
 
 fn animate_light_direction(
     time: Res<Time>,
     mut query: Query<&mut Transform, With<DirectionalLight>>,
 ) {
-    for mut transform in &mut query {
-        transform.rotation = Quat::from_euler(
-            EulerRot::ZYX,
-            0.0,
-            time.elapsed_seconds() * PI / 5.0,
-            -FRAC_PI_4,
-        );
-    }
+    // for mut transform in &mut query {
+    //     transform.rotation = Quat::from_euler(
+    //         EulerRot::ZYX,
+    //         0.0,
+    //         time.elapsed_seconds() * PI / 5.0,
+    //         -FRAC_PI_4,
+    //     );
+    // }
 }

Screenshot2024_07_10_145737

Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

Really appreciate the detailed PR description. Everything checks out.

crates/bevy_pbr/src/render/clustered_forward.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/clustered_forward.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/shadows.wgsl Outdated Show resolved Hide resolved
@ickshonpe ickshonpe added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Testing A change that impacts how we test Bevy or how users test their apps labels Jul 10, 2024
Soulghost and others added 3 commits July 10, 2024 16:30
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
@janhohenheim janhohenheim 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 Jul 17, 2024
@janhohenheim janhohenheim added this to the 0.14.1 milestone Jul 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 22, 2024
Merged via the queue into bevyengine:main with commit 462da1e Jul 22, 2024
28 checks passed
mockersf pushed a commit that referenced this pull request Aug 2, 2024
)

# Objective

- Fixes #14139

## Solution

- correct the input parameters at these call sites.

## Testing

1. Use a 3D scene example with PBR lighting and shadows enabled, such as
the `shadow_caster_receiver` and `load_gltf` example, for testing.
2. Enable relevant shader defines in crates/bevy_pbr/src/pbr_material.rs
for the StandardMaterial.
```rust
impl Material for StandardMaterial {
    // ...
    fn specialize(
            _pipeline: &MaterialPipeline<Self>,
            descriptor: &mut RenderPipelineDescriptor,
            _layout: &MeshVertexBufferLayoutRef,
            key: MaterialPipelineKey<Self>,
        ) -> Result<(), SpecializedMeshPipelineError> {
            // ...
            // shader_defs.push("CLUSTERED_FORWARD_DEBUG_Z_SLICES".into());
            // shader_defs.push("CLUSTERED_FORWARD_DEBUG_CLUSTER_COHERENCY".into());
            shader_defs.push("DIRECTIONAL_LIGHT_SHADOW_MAP_DEBUG_CASCADES".into());
            // ...
    }
}
``` 

## Showcase
### CLUSTERED_FORWARD_DEBUG_Z_SLICES
- example: examples/3d/shadow_caster_receiver.rs

![Screenshot2024_07_10_143150](https://github.com/bevyengine/bevy/assets/6300263/fbd12712-5cb9-489d-a7d1-ed55f72fb234)

### CLUSTERED_FORWARD_DEBUG_CLUSTER_COHERENCY
- example: examples/3d/shadow_caster_receiver.rs

![Screenshot2024_07_10_143312](https://github.com/bevyengine/bevy/assets/6300263/8eca5d7a-27b6-4ff5-9f8d-d10b49b3f990)

### DIRECTIONAL_LIGHT_SHADOW_MAP_DEBUG_CASCADES
For this one, we need to use a large scene and modity the
`CascadeShadowConfigBuilder`, here is a simple patch for the `load_gltf`
example:
```
diff --git a/examples/3d/load_gltf.rs b/examples/3d/load_gltf.rs
index 358446238..9403aa288 100644
--- a/examples/3d/load_gltf.rs
+++ b/examples/3d/load_gltf.rs
@@ -18,7 +18,7 @@ fn main() {
 fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
     commands.spawn((
         Camera3dBundle {
-            transform: Transform::from_xyz(0.7, 0.7, 1.0)
+            transform: Transform::from_xyz(0.7, 0.7, 2.0)
                 .looking_at(Vec3::new(0.0, 0.3, 0.0), Vec3::Y),
             ..default()
         },
@@ -39,30 +39,40 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
         // We also adjusted the shadow map to be larger since we're
         // only using a single cascade.
         cascade_shadow_config: CascadeShadowConfigBuilder {
-            num_cascades: 1,
-            maximum_distance: 1.6,
+            num_cascades: 5,
+            maximum_distance: 20.0,
             ..default()
         }
         .into(),
         ..default()
     });
+
     commands.spawn(SceneBundle {
         scene: asset_server
             .load(GltfAssetLabel::Scene(0).from_asset("models/FlightHelmet/FlightHelmet.gltf")),
         ..default()
     });
+
+    for i in 1..=10 {
+        commands.spawn(SceneBundle {
+            scene: asset_server
+                .load(GltfAssetLabel::Scene(0).from_asset("models/FlightHelmet/FlightHelmet.gltf")),
+            transform: Transform::from_xyz(i as f32 * 0.5, 0.0, i as f32 * -2.0),
+            ..default()
+        });
+    }
 }
 
 fn animate_light_direction(
     time: Res<Time>,
     mut query: Query<&mut Transform, With<DirectionalLight>>,
 ) {
-    for mut transform in &mut query {
-        transform.rotation = Quat::from_euler(
-            EulerRot::ZYX,
-            0.0,
-            time.elapsed_seconds() * PI / 5.0,
-            -FRAC_PI_4,
-        );
-    }
+    // for mut transform in &mut query {
+    //     transform.rotation = Quat::from_euler(
+    //         EulerRot::ZYX,
+    //         0.0,
+    //         time.elapsed_seconds() * PI / 5.0,
+    //         -FRAC_PI_4,
+    //     );
+    // }
 }
``` 

![Screenshot2024_07_10_145737](https://github.com/bevyengine/bevy/assets/6300263/c5c71894-f9f7-45fa-9b4f-598e324b42d0)

---------

Co-authored-by: ickshonpe <david.curthoys@googlemail.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-Bug An unexpected or incorrect behavior C-Testing A change that impacts how we test Bevy or how users test their apps 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.

The function hsv_to_rgb in wgsl is defined or called incorrectly
4 participants