-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix webgl2 #20286
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 webgl2 #20286
Conversation
|
This would require users to download |
|
Thank you for the quick turnaround. I tested the reflection probe example on mac, webgpu and webgl. All works as expected. The diff also looks good to me but good point about using assets/textures folder.
A couple of solutions i have in mind:
|
|
Id rather leave improving DX to another PR, given that the alternative atm is a revert. I have a few ideas but im not sure which is best. Any are better than a revert though :p |
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 think this is fine. The plugin naming is not something I will block on but if people agree with me then it would be nice to change before merging.
One idea for a follow-up - we could add a feature to optionally compile-in the texture instead of loading it from an asset directory. That would give people the option of that convenience, though perhaps it has other downsides like ram usage for the binary, unless that part can be paged out after upload to vram.
ac43819 to
af6eadd
Compare
crates/bevy_pbr/src/lib.rs
Outdated
| pub texture: Handle<Image>, | ||
| } | ||
|
|
||
| impl FromWorld for Bluenoise { |
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.
Could we make this a system rather than a FromWorld impl? Something like init_blue_noise
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.
it seemed to be pretty sensitive to ordering, i dont want to figure out porting this to RenderStartup in this PR
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.
Yeah, I'm okay with porting it later
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.
Removed this declaration for FromWorld and put the code block back into PBR plugin with a conditional feature flag.
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 would prefer if the texture was embedded but gated by a feature flag and if someone spawns a GeneratedEnvironmentMapLight component without the feature flag we log a warning saying they need to enable the feature. A bit like what the tonemapping_luts feature flag does. The current approach requires user to copy the texture which is a bit less nice but not a blocker.
With that said, the PR as is does fix the issue, we can always implement the feature flag thing later.
|
I created a PR against this branch to address some of @IceSentry feedback about using feature flags. I also added a fallback to a RNG based noise which still looks decent. Since this PR already has all approvals I don't consider this a blocker, can be reviewed/merged as a separate PR as well. I will get feedback on my proposed changes from @atlv24 when I get a chance. |
Add blue noise texture feature and fall back to rng
|
You added a new feature but didn't update the readme. Please run |
| debug = ["bevy_internal/debug"] | ||
|
|
||
| # Include spatio-temporal blue noise KTX2 file used by generated environment maps, Solari and atmosphere | ||
| bluenoise_texture = [ |
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.
Shouldn't we be enabling this feature in other features, e.g. for solari or env maps?
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.
Its not needed in solari just yet, we'll let Jasmine add the feature when she starts using it. And gen env maps dont need this either thanks to the fallback, so for now its fine i believe
|
@mate-h thanks for fixing CI! i think we're good to go |
# Objective - bevyengine#19076 (comment) broke webgl2 and bloated binary size by a megabyte - Fixes bevyengine#20276 ## Solution - Don't embed stbn.ktx2, just load it for now - Gate gen env maps by limit checks - Extract a plugin to do this cleanly - Only load stbn.ktx2 when its needed ## Testing - reflection_probes example - someone please test webgl2 --------- Co-authored-by: Máté Homolya <mate.homolya@gmail.com>
# Objective - bevyengine#19076 (comment) broke webgl2 and bloated binary size by a megabyte - Fixes bevyengine#20276 ## Solution - Don't embed stbn.ktx2, just load it for now - Gate gen env maps by limit checks - Extract a plugin to do this cleanly - Only load stbn.ktx2 when its needed ## Testing - reflection_probes example - someone please test webgl2 --------- Co-authored-by: Máté Homolya <mate.homolya@gmail.com>
Objective
Solution
Testing