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

WebGPU: fix crashes in Firefox and Safari #16045

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

Popov72
Copy link
Contributor

@Popov72 Popov72 commented Jan 8, 2025

This PG crashes in Firefox / Safari because an empty vertex buffer is bound to the pipeline. The fix is simply to call this._createOffsetsBuffer() earlier so that the correct vertex buffer is created (cc @RolandCsibrei, to make sure it's the right fix / in the right place).

This PG crashes for the same reason, but the fix is a bit more complex as we shouldn't use an attribute in a node material if the mesh using that material doesn't support the attribute. So I've added some code to declare a regular (dummy) variable with the same name as the attribute when the latter is not supported.

Note that I also added some debug code (mostly labels) to ease debugging in WebGPU.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 8, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@deltakosh deltakosh enabled auto-merge (squash) January 8, 2025 15:27
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 8, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 8, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 8, 2025

@deltakosh deltakosh merged commit 68eb955 into BabylonJS:master Jan 8, 2025
13 of 14 checks passed
@RolandCsibrei
Copy link
Contributor

RolandCsibrei commented Jan 8, 2025

Unfortunatelly the changes in GRL has introduced an error on WebGL in Safari/Firefox/Chrome - I'm on MacOS:
[.WebGL-0x128005cf100] GL_INVALID_OPERATION: Vertex buffer is not big enough for the draw call
and doesn't create any output in Safari with the WebGPU flag enabled. It does work with Chrome WebGPU, though.

I tried to get WebGPU support working in Firefox by enabling the required flags, but no luck.

Let me do some research.

@Popov72
Copy link
Contributor Author

Popov72 commented Jan 8, 2025

Ah, too bad... It could be that the offsets buffer is not yet set when calling _createOffsetsBuffer from updateLazy.

One question I have is if offsets are always used, or if they are optional? The shader code always define the grl_offsets attribute, but if offsets are optional, the attribute shouldn't be defined, as we won't be able to pass vertex data for it. Instead, for minimal changes, you should replace the attribute declaration with a dummy variable declaration, something like:

#ifdef GREASED_LINE_USE_OFFSETS
    attribute vec3 grl_offsets;
#else
    vec3 grl_offsets = vec3(0.);
#endif

Of course, you will have to set the appropriate true/false value to GREASED_LINE_USE_OFFSETS depending on offsets usage.

@RolandCsibrei
Copy link
Contributor

I think we have two options:

  1. Add a define to the material plugin and set it when the offsets are set. This will recompile the shader.
  2. Create the offsets vertex buffer when creating the line and fill it with vec3f(0.0). This will not require shader recompilation.

Which is the better choice?

Interestengly the PG runs w/o issues on Chrome/WebGPU.

@Popov72
Copy link
Contributor Author

Popov72 commented Jan 9, 2025

I think 1/ would be better, because it would not consume a vertex buffer when we don't need it.

The PG runs in Chrome because they don't handle a limit condition the same way than in Firefox/Safari, but the specification allows both implementations.

@RolandCsibrei
Copy link
Contributor

I think 1/ would be better, because it would not consume a vertex buffer when we don't need it.

Yes, this seems to be the better option. I don't expect anyone to frequently set or unset the offsets, so there will essentially be one shader recompilation when the offsets are initially set. Changing the offsets afterward will not require shader recompilation.

I'm on it... Thanks for sharing your thoughts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants