-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Fix MaterialExtension::enable_shadows and disable shadows for forward decals
#21908
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 MaterialExtension::enable_shadows and disable shadows for forward decals
#21908
Conversation
|
This is included in #21909 |
|
Reopen it. I think this fix is ready for review and might be worth for the next release. |
0ec9399 to
c4757f8
Compare
NicoZweifel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it some more I think to make it consistent that it should check the base and extension (can't activate shadows/prepass on a base and its pipeline that don't support it) but it probably doesn't have to be part of this PR.
fn enable_prepass() -> bool {
B::enable_prepass() && E::enable_prepass()
}
fn enable_shadows() -> bool {
B::enable_shadows() && E::enable_shadows()
}|
|
||
| fn enable_shadows() -> bool { | ||
| E::enable_prepass() | ||
| E::enable_shadows() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we want to ignore the base here. I think this can lead to some bugs, similar to what got fixed here. Arguably enable_shadows is not as fatal but I think silent inconsistencies and bugs could still happen when ignoring the base.
| E::enable_shadows() | |
| B::enable_shadows() && E::enable_shadows() |
I think the same applies to enable_prepass, but it might be out of scope for this PR.
|
I'm not sure if using && might not work as expected, since if the base is false, the extended cannot override it. Making them overridable makes more sense to me. |
I get that it makes sense for flexibility, but I just don't see a practical use-case where force overriding is the correct/safe way in this case. If a user wants to do this, they can always use a base with shadows/prepass enabled and extend that. I think the end user shouldn't be able to override something that isn't supported. Making the materials and pipeline agree seems like cleaner architecture imo and avoids the potential pitfalls (crashes, silent failures). There is also the assumption that the user has to use I don't think this is the right place to discuss this though so I might create an issue or draft PR for that. |
…rd decals (#21908) # Objective `MaterialExtension::enable_shadows` wrongly returns `E::enable_prepass()` instead of `E::enable_shadows()` ## Solution Fix it. Also disable shadows for forward decals because decals typically should not cast shadows. ## Testing Run the decals example.
Objective
MaterialExtension::enable_shadowswrongly returnsE::enable_prepass()instead ofE::enable_shadows()Solution
Fix it.
Also disable shadows for forward decals because decals typically should not cast shadows.
Testing
Run the decals example.