Gate LTC LUTs behind a feature and merge them to a texture array#24065
Gate LTC LUTs behind a feature and merge them to a texture array#24065beicause wants to merge 9 commits intobevyengine:mainfrom
Conversation
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Please make sure the new lut passes ktx validate: #23900 |
|
This PR makes progress on #23975, so I’ll add it to the milestone for expedited consideration With this, the |
|
I like this more than the other PR. Maybe we should rename the feature to "rect_lights" or something though? Idk |
PointLight may use ltc luts too #23400 |
|
Ahh right, nvm. |
|
How about "area_lights"? |
"area_lights" sounds like we should gate the RectLight code as well. How about "area_light_luts"? |
Why not do that? They need the LUTs to function, no? |
I'm not sure about this, it would add complexity. We didn't do it for |
kfc35
left a comment
There was a problem hiding this comment.
Checked out the branch and everything looks the same wrt RectLights. Did not find any issues in the code. As I have also mentioned, it also allows the deferred_rendering example to run too!
I just have a (long) non-blocking comment about requiring the feature for the testbed.
| name = "testbed_3d" | ||
| path = "examples/testbed/3d.rs" | ||
| doc-scrape-examples = true | ||
| required-features = ["area_light_luts"] |
There was a problem hiding this comment.
non-blocking comment: I know that I recently added a rect_light to the lights scene, but I don’t know if this should necessarily be added as a required feature in Cargo.toml, similar to how the ui testbed doesnt require the debug ui feature flag. The ui testbed doesn’t insert the debug outline scene if the feature isn’t enabled. Perhaps something similar should be done for the 3d testbed? It’ll allow the 3d testbed to keep growing in feature complexity without needing to specify a growing list of required flags.
A related question is, do we expect the app to fail loudly if the app creator adds a RectLight, but did not enable the associated feature? If the answer is no, then I think I wouldn’t require the feature here. I also then wouldn’t do any feature gating of the RectLight in the testbed so that case could theoretically be tested.
I agree that it should be added in example-run.yml though, which you have done. I also would still require it for the rect_light example as well
Co-authored-by: Kevin Chen <chen.kevin.f@gmail.com>
|
One comment: Is there somewhere we can error_once! log if area lights are used without the feature being enabled? |
Objective
Alternative to #24004.
#23288 adds ltc luts for rect light support which implicitly requires
bevy_image/ktx2andbevy_image/zstdotherwise loading ltc luts will panic.We either accept to always enable area light supoort (#24004), or add a feature to opt out it (this PR).
Solution
Gate ltc luts behind a feature and merge them to a texture array.
Testing
rect_lightexample works.