[Merged by Bors] - enable Webgl2 optimisation in pbr under feature #3291

mockersf · 2021-12-10T17:14:48Z

Objective

3d examples fail to run in webgl2 because of unsupported texture formats or texture too large

Solution

switch to supported formats if a feature is enabled. I choose a feature instead of a build target to not conflict with a potential webgpu support

Very inspired by superdump@6813b2e, and need #3290 to work.

I named the feature webgl2, but it's only needed if one want to use PBR in webgl2. Examples using only 2D already work.

pipelined/bevy_pbr2/Cargo.toml

TheRawMeatball · 2021-12-10T17:24:48Z

Personally I'd prefer if this was chosen by default for wasm targets, since webgpu is still in beta and IMO it's more important for the common case to "just work". We could revisit this later when webgpu support is rolled out to stable browsers.

mockersf · 2021-12-10T17:45:18Z

There is already a mostly working webgpu support: https://discord.com/channels/691052431525675048/750833140746158140/915727141130338385

TheRawMeatball · 2021-12-10T18:10:56Z

WGPU's webgpu backend being mostly complete is irrelevant IMO since webgpu isn't available on any stable browsers. Right now, people will still be only using webgl2 for any releases since that's the only backend that's actually supported by browsers.

A potential option here could be to check whether wgpu itself is running on webgpu or webgl2 dynamically though.

mrk-its · 2021-12-10T19:07:39Z

A potential option here could be to check whether wgpu itself is running on webgpu or webgl2 dynamically though.

Now it is not possible AFAIR - wgpu uses webgl feature to select between webgpu and webgl2.

So I would prefer to rename this feature from webgl2 to webgl to be compatible with wgpu. And maybe let's simply add it to bevy's default features to make it default on wasm32?

superdump

This mostly looks good to me. I think it's a good idea to get something like this in for now rather than waiting on implementing the wgpu downlevel/feature/limits stuff. My only change is that I think WebGL2 supports array textures, but not cube array textures, so I think the shader def should be changed to reflect that. I would approve otherwise. :)

And as WebGL2 is supported in browsers where WebGPU is behind developer flags still, I think, then it makes sense to make webgl the default as @mrk-its proposed for now. Later when the landscape changes I guess it will make sense to change the default behaviour. What do you think on this one @cart?

superdump · 2021-12-11T15:34:39Z

pipelined/bevy_pbr2/src/render/mesh.rs

@@ -469,6 +475,9 @@ impl SpecializedPipeline for MeshPipeline {
            depth_write_enabled = true;
        }

+        #[cfg(feature = "webgl2")]
+        shader_defs.push(String::from("NO_ARRAY_TEXTURES_SUPPORT"));


I think it is specifically no cube array texture support.

textures of type D2Array seems to be an issue too, if I don't change them to D2 I don't have shadows for directional lights

Ah, ok. Well then I guess this is fine. :)

mockersf · 2021-12-11T21:51:02Z

And maybe let's simply add it to bevy's default features to make it default on wasm32?

I can either add it as a default feature, which means that everyone will have by default only one point light, and the no array textures, or add it as a feature already selected for wasm target, wich means it won't be possible to disable it on wasm. As far as I know there are no way to specify default feature set by target, rust-lang/cargo#1197 (comment) would be needed for that.

mockersf · 2021-12-11T23:06:10Z

actually bevy_render2 was already enabling wgpu/webgl based on target, so I did the same for this feature, and moved control for bevy_render2 and bevy_pbr2 to a single point in bevy_internal

mockersf · 2021-12-12T01:33:38Z

on macOS this works on chrome and firefox but it crashes safari... wgpu examples are working with safari, so maybe an issue on Bevy side

Firefox is logging this warning:

WebGL warning: getBufferSubData: Reading from a buffer with usage other than *_READ causes pipeline stalls. Copy through a STREAM_READ buffer.

and it seems that safari is crashing when calling getBufferSubData

Could we have a buffer somewhere that should be marked as read and isn't?

Chrome is working bug logs

UNSUPPORTED (log once): gldCopyBufferSubData: NEEDS IMPLEMENTATION

so... yeah Chrome 🎉

After some digging, it seems getBufferSubData is called for the vertex buffer of each mesh

pipelined/bevy_render2/src/mesh/mesh/mod.rs

mockersf · 2021-12-19T14:39:34Z

with the wgpu update, 3D examples fails with a new error on Safari:

panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
      note: label = `pbr_opaque_mesh_pipeline`
    Internal error in VERTEX | FRAGMENT | VERTEX_FRAGMENT shader: Name conflicts between uniform block field names: _group_0_binding_0

Not sure how to fix that 😞 UI and 2D examples work on safari though

superdump · 2021-12-19T17:33:00Z

group 0 binding 0 is the View uniform. The mesh pipeline uses the same shader handle for both vertex and fragment stages. The pbr pipeline specialises the mesh pipeline and uses a different shader for the fragment stage. I wonder if the vertex stage shader (mesh.wgsl) containing a fragment entrypoint with the same name (fragment) as the overridden fragment stage shader (pbr.wgsl) that's confusing it somehow...?

mockersf · 2021-12-19T17:38:18Z

Yup, and somebody did not read the // NOTE:

bevy/crates/bevy_pbr/src/render/depth.wgsl

Lines 3 to 8 in 3409579

    
           // NOTE: Keep in sync with pbr.wgsl 
        
           struct View { 
        
               view_proj: mat4x4<f32>; 
        
               projection: mat4x4<f32>; 
        
               world_position: vec3<f32>; 
        
           };

bevy/crates/bevy_pbr/src/render/mesh_view_bind_group.wgsl

Lines 1 to 10 in 3409579

    
           struct View { 
        
               view_proj: mat4x4<f32>; 
        
               inverse_view: mat4x4<f32>; 
        
               projection: mat4x4<f32>; 
        
               world_position: vec3<f32>; 
        
               near: f32; 
        
               far: f32; 
        
               width: f32; 
        
               height: f32; 
        
           };

but copy pasting to have the same in both didn't fix the issue...

superdump · 2021-12-19T17:41:01Z

depth is used for the shadow pass. But I also noted that and I wondered how shadows are even working. Hmm...

mockersf · 2021-12-20T14:58:18Z

crashes with Safari are due to gfx-rs/naga#1617

Co-authored-by: MinerSebas <[email protected]>

Co-Authored-By: Jakob Hellermann <[email protected]>

cart · 2021-12-22T20:56:22Z

This all looks reasonable to me. I can confirm that this works with the naga fixes:

cart · 2021-12-22T20:59:17Z

Firefox perf is quite bad. But chrome seems smooth (didn't do any actual frametime measurements).

cart · 2021-12-22T20:59:31Z

bors r+

# Objective - 3d examples fail to run in webgl2 because of unsupported texture formats or texture too large ## Solution - switch to supported formats if a feature is enabled. I choose a feature instead of a build target to not conflict with a potential webgpu support Very inspired by superdump@6813b2e, and need #3290 to work. I named the feature `webgl2`, but it's only needed if one want to use PBR in webgl2. Examples using only 2D already work. Co-authored-by: François <[email protected]>

bors · 2021-12-22T21:19:18Z

Pull request successfully merged into main.

Build succeeded:

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 10, 2021

MinerSebas reviewed Dec 10, 2021

View reviewed changes

pipelined/bevy_pbr2/Cargo.toml Outdated Show resolved Hide resolved

mockersf added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Dec 10, 2021

cart added this to the Bevy 0.6 milestone Dec 10, 2021

superdump requested changes Dec 11, 2021

View reviewed changes

mockersf added the O-Web Specific to web (WASM) builds label Dec 11, 2021

jakobhellermann approved these changes Dec 11, 2021

View reviewed changes

superdump approved these changes Dec 11, 2021

View reviewed changes

mockersf force-pushed the webgl2-under-feature branch from 5758523 to 69c939f Compare December 12, 2021 14:12

mockersf commented Dec 13, 2021

View reviewed changes

pipelined/bevy_render2/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved

mockersf force-pushed the webgl2-under-feature branch 2 times, most recently from 30a3310 to 25bf2eb Compare December 19, 2021 08:58

mockersf mentioned this pull request Dec 21, 2021

Deploy examples on website in wasm bevyengine/bevy-website#225

Merged

jakobhellermann mentioned this pull request Dec 21, 2021

OpenGL support #841

Closed

mockersf added 3 commits December 22, 2021 02:15

add webgl2 feature

682e077

limit texture size in webgl2

14cb1e3

change texture format

44dd4ce

mockersf and others added 8 commits December 22, 2021 02:15

remove empty default feature

db5db5a

Co-authored-by: MinerSebas <[email protected]>

rename feature

3d83ee8

log adapter in wasm

75daf14

enable webgl by default in wasm

e79e670

get point lights in a stable order

6b65a45

Co-Authored-By: Jakob Hellermann <[email protected]>

clippy

b07dccc

give names to buffer and add usage to vertex index buffer

412ef30

update for wgpu 0.12

c28ae4f

mockersf force-pushed the webgl2-under-feature branch from eaea4aa to c28ae4f Compare December 22, 2021 01:15

mockersf added 2 commits December 22, 2021 02:20

missed the feature rename somehow

6b25ca0

fix doc link

d8da48c

bors bot changed the title ~~enable Webgl2 optimisation in pbr under feature~~ [Merged by Bors] - enable Webgl2 optimisation in pbr under feature Dec 22, 2021

bors bot closed this Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - enable Webgl2 optimisation in pbr under feature #3291

[Merged by Bors] - enable Webgl2 optimisation in pbr under feature #3291

mockersf commented Dec 10, 2021

TheRawMeatball commented Dec 10, 2021

mockersf commented Dec 10, 2021

TheRawMeatball commented Dec 10, 2021

mrk-its commented Dec 10, 2021 •

edited

Loading

superdump left a comment •

edited

Loading

superdump Dec 11, 2021

mockersf Dec 11, 2021

superdump Dec 11, 2021

mockersf commented Dec 11, 2021

mockersf commented Dec 11, 2021

mockersf commented Dec 12, 2021 •

edited

Loading

mockersf commented Dec 19, 2021 •

edited

Loading

superdump commented Dec 19, 2021

mockersf commented Dec 19, 2021

superdump commented Dec 19, 2021

mockersf commented Dec 20, 2021

cart commented Dec 22, 2021

cart commented Dec 22, 2021

cart commented Dec 22, 2021

bors bot commented Dec 22, 2021

[Merged by Bors] - enable Webgl2 optimisation in pbr under feature #3291

[Merged by Bors] - enable Webgl2 optimisation in pbr under feature #3291

Conversation

mockersf commented Dec 10, 2021

Objective

Solution

TheRawMeatball commented Dec 10, 2021

mockersf commented Dec 10, 2021

TheRawMeatball commented Dec 10, 2021

mrk-its commented Dec 10, 2021 • edited Loading

superdump left a comment • edited Loading

Choose a reason for hiding this comment

superdump Dec 11, 2021

Choose a reason for hiding this comment

mockersf Dec 11, 2021

Choose a reason for hiding this comment

superdump Dec 11, 2021

Choose a reason for hiding this comment

mockersf commented Dec 11, 2021

mockersf commented Dec 11, 2021

mockersf commented Dec 12, 2021 • edited Loading

mockersf commented Dec 19, 2021 • edited Loading

superdump commented Dec 19, 2021

mockersf commented Dec 19, 2021

superdump commented Dec 19, 2021

mockersf commented Dec 20, 2021

cart commented Dec 22, 2021

cart commented Dec 22, 2021

cart commented Dec 22, 2021

bors bot commented Dec 22, 2021

mrk-its commented Dec 10, 2021 •

edited

Loading

superdump left a comment •

edited

Loading

mockersf commented Dec 12, 2021 •

edited

Loading

mockersf commented Dec 19, 2021 •

edited

Loading