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

[Merged by Bors] - Increase the MAX_DIRECTIONAL_LIGHTS from 1 to 10 #6066

Closed

Conversation

kurtkuehnert
Copy link
Contributor

Objective

Currently we are limiting the amount of direction lights in a scene to one.

Solution

Increase the amount of direction lights from 1 to 10.
This still is not a perfect solution, but should unblock many use cases.
We could probably just store the directional lights similar to the point lights in an storage buffer, allowing for an variable amount of directional lights.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use labels Sep 22, 2022
@cart
Copy link
Member

cart commented Sep 22, 2022

Have you tested this in WebGL? I vaguely remember there being a restriction there, but maybe I'm mistaken.

@kurtkuehnert
Copy link
Contributor Author

Have you tested this in WebGL? I vaguely remember there being a restriction there, but maybe I'm mistaken.

No I have not, I am not sure how to compile and test on WebGL. Hopefully @superdump knows more.
I do not think we are exceeding the uniform size limit, so I am not sure what the issue could be.
But I am wondering why this value was not set higher to begin with, all the code was already in place.

@mockersf
Copy link
Member

mockersf commented Sep 22, 2022

doesn't work in wasm:

panicked at 'wgpu error: Validation Error

Caused by:
    In Texture::create_view
      note: label = `directional_light_shadow_map_texture_view`
    TextureView array layer count + base array layer 2 must be <= Texture depth/array layer count 1

#5900 would let us have a different const value in the shader depending on the arch 🥷

@robtfm
Copy link
Contributor

robtfm commented Sep 22, 2022

this should work if you add a check to render\light.rs @ 1120 to make sure the number of shadow-enabled directional lights does not exceed directional_shadow_maps_count defined @ 811 (which is already capped at 1 for webgl).

you might also need to sort the directional lights to get the chosen shadow-caster to be stable.

@kurtkuehnert
Copy link
Contributor Author

kurtkuehnert commented Sep 23, 2022

Thanks for your feedback.

I have now added errors if spot/directional lights exceeds the maximum supported amount.
This should simply disable their shadows if the maximum is surpassed.

I think this is pretty much what @robtfm recommended.

Do you think it would be a good idea to separate the directional lights into their own uniform/storage buffer in a follow up PR or are they simply to few to really matter?

@robtfm
Copy link
Contributor

robtfm commented Sep 23, 2022

i don't think we should disable all the directional+spotlight shadow maps when the max is exceeded. i would prefer to leave the count calculations as they were and just add the (index < directional_shadow_map_count) condition to the flag set. that should work correctly and just render as many shadow maps as the platform supports.

the errors will be emitted every frame and are not really "errors"... i would suggest either silently ignoring them (that would be my pref for the shadow map count warning, it's the same treatment as point light shadows), or using a warning and a Local to check if the warning has already been emitted similar to in assign_lights_to_clusters (my pref for the directional light count warning).

@robtfm
Copy link
Contributor

robtfm commented Sep 24, 2022

tested win32 and wasm, looks good to me

@cart cart added this to the Bevy 0.9 milestone Nov 2, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cart
Copy link
Member

cart commented Nov 3, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 3, 2022
# Objective

Currently we are limiting the amount of direction lights in a scene to one.

## Solution

Increase the amount of direction lights from 1 to 10. 
This still is not a perfect solution, but should unblock many use cases.
We could probably just store the directional lights similar to the point lights in an storage buffer, allowing for an variable amount of directional lights.


Co-authored-by: Kurt Kühnert <51823519+Ku95@users.noreply.github.com>
@bors bors bot changed the title Increase the MAX_DIRECTIONAL_LIGHTS from 1 to 10 [Merged by Bors] - Increase the MAX_DIRECTIONAL_LIGHTS from 1 to 10 Nov 3, 2022
@bors bors bot closed this Nov 3, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Currently we are limiting the amount of direction lights in a scene to one.

## Solution

Increase the amount of direction lights from 1 to 10. 
This still is not a perfect solution, but should unblock many use cases.
We could probably just store the directional lights similar to the point lights in an storage buffer, allowing for an variable amount of directional lights.


Co-authored-by: Kurt Kühnert <51823519+Ku95@users.noreply.github.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-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants