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

MAX_DIRECTIONAL_LIGHTS must be defined to have the Lights struct #6997

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Dec 21, 2022

Objective

Solution

  • Lights struct only have the directional_lights field when MAX_DIRECTIONAL_LIGHTS is defined

@mockersf mockersf added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Dec 21, 2022
@IceSentry
Copy link
Contributor

IceSentry commented Dec 21, 2022

When running the post_processing example with this PR I get.

2022-12-21T17:16:06.229004Z ERROR bevy_render::render_resource::pipeline_cache: failed to process shader:
error: unknown type: 'Lights'
   ┌─ wgsl:68:22
   │
68 │ var<uniform> lights: Lights;
   │                      ^^^^^^ unknown type

So this needs to be added anywhere that Lights is used for this to work correctly.

@mockersf
Copy link
Member Author

oh right that example was already crashing... changed to not have the directional_lights field then, so the struct is still present.
the Lights struct is used as a binding, not sure what to replace it with when MAX_DIRECTIONAL_LIGHTS is not available

@aevyrie
Copy link
Member

aevyrie commented Dec 21, 2022

This also fixes #6799, turns out my issue was a duplicate.

@IceSentry
Copy link
Contributor

not sure what to replace it with when MAX_DIRECTIONAL_LIGHTS is not available

Maybe just hardcode it to 0?

Just tested the latest commit and it works correctly for the post_processing example

@Elabajaba
Copy link
Contributor

Elabajaba commented Jan 26, 2023

This could probably be updated now that cascaded shadow maps has landed, since it's the same error/fix for that. (assuming we won't get #5703 in before next release)

Getting failures on both shader_prepass and post_processing now.

ERROR bevy_render::render_resource::pipeline_cache: failed to process shader:
error: expected expression, found '#'
   ┌─ wgsl:38:41
   │
38 │     cascades: array<DirectionalCascade, #{MAX_CASCADES_PER_LIGHT}>,
   │                                         ^ expected expression

                                  ^ expected expression

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Does this work? It’s not obvious to me that it would but if it does, ok I guess. :) As was mentioned, the cascade also need fixing, could you do that too, please?

@@ -41,8 +41,10 @@ struct DirectionalLight {
let DIRECTIONAL_LIGHT_FLAGS_SHADOWS_ENABLED_BIT: u32 = 1u;

struct Lights {
#ifdef MAX_DIRECTIONAL_LIGHTS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifdef MAX_DIRECTIONAL_LIGHTS
#ifdef MAX_DIRECTIONAL_LIGHTS

// NOTE: this array size must be kept in sync with the constants defined in bevy_pbr/src/render/light.rs
directional_lights: array<DirectionalLight, #{MAX_DIRECTIONAL_LIGHTS}u>,
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif

@Elabajaba
Copy link
Contributor

Made a PR to fix the issue with MAX_CASCADES_PER_LIGHT being undefined sometimes so it doesn't need to be added to this one. #7380

@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Jan 27, 2023
@superdump
Copy link
Contributor

I feel like this approach will only work if the binding isn't used. Because we comment out a member of the struct, we change its data layout. So if anything is bound, it will be misinterpreted. Same for #7380. I'm not sure how to solve it at this moment. I feel like the shader failing to compile is an indication that the use of the import should be fixed.

@superdump
Copy link
Contributor

I implemented what I think is a better fix in #7419

bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes #6799
- Fixes #6996
- Fixes #7375 
- Supercedes #6997
- Supercedes #7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
@mockersf mockersf closed this Jan 30, 2023
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fix `post_processing` and `shader_prepass` examples as they fail when compiling shaders due to missing shader defs
- Fixes bevyengine#6799
- Fixes bevyengine#6996
- Fixes bevyengine#7375 
- Supercedes bevyengine#6997
- Supercedes bevyengine#7380 

## Solution

- The prepass was broken due to a missing `MAX_CASCADES_PER_LIGHT` shader def. Add it.
- The shader used in the `post_processing` example is applied to a 2D mesh, so use the correct mesh2d_view_bindings shader import.
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
Projects
None yet
6 participants