Skip to content

Support vertex pulling #16826

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Jun 27, 2025

Vertex pulling is where you do your own reading of vertex data in the vertex shader instead of relying on the standard vertex attribute pipeline. I'm not sure of the best approach in Babylon but I made some minimal changes to make it work. I'd appreciate some feedback and ideas for a better way to set this up.

First, some motivation:
I want vertex pulling as a way of selecting vertex data from neighboring vertices in the IBL Shadows voxelization shader. By retrieving normal info for the provoking vertex of a triangle, I can selectively swizzle the axes of the position to maximize rasterization area and avoid missing voxels. This eliminates the need for 3-pass voxelization and opens the door for doing realtime voxelization of animated geometry (in WebGPU only due to the need for 3D storage textures).
https://playground.babylonjs.com/?snapshot=refs/pull/16826/merge#XSNYAU#131

For vertex pulling to work, we typically need:

  1. An empty vertex layout.
  2. Our needed vertex buffers assigned as storage buffers
  3. A non-indexed draw call with the number of vertices to process. This causes @builtin(vertex_index) to be sequential which is critical for fetching info about neighboring vertices.

No changes are needed to Babylon for the first two requirements. For the 3rd point though, we want to do a engine.drawArraysType even though the mesh has indices. To do this, we set material.useVertexPulling.
The shader must also not declare vertex attributes.

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 27, 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.

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 27, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 27, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 27, 2025

@Popov72
Copy link
Contributor

Popov72 commented Jul 1, 2025

It would be a great feature to have!

However, I think we should strive to improve integration into the engine so that we/the user don't have to manually manage storage buffers and so that it remains as transparent as possible.

Another issue is that we don't really want to modify the mesh at all since some render passes won't do vertex pulling

I wonder if we could just create the vertex buffers with STORAGE flags (in addition to the VERTEX + WRITE flags we currently use)... This way, the buffers should be usable in both pulling and non-pulling modes (to be tested, however, and I don't know if adding unnecessary flags has an impact on performance - STORAGE is not necessary in non-pulling mode, and VERTEX is not necessary in pulling mode).

In this case, we could add a useVertexPulling property to Material. When true, we would not bind vertex buffers to the WebGPU pipeline, but rather bind storage buffers to the shader. We would also inject a “USE_VERTEX_PULLING” define, so that the shader code can adapt accordingly (in case the code wants to support both modes)...

I'm probably missing a few things, but I think this would be a better way to support this mode.

cc @sebavan for the discussion.

@Popov72
Copy link
Contributor

Popov72 commented Jul 1, 2025

Also, in your PG example, I think you would want to calculate the normal yourself by performing the vector product of two edges of the triangle, because the normal to a vertex is generally the average of the normals of the faces to which that vertex belongs, so it is not the true normal to the face.

@sebavan
Copy link
Member

sebavan commented Jul 1, 2025

I really like where it is going :-) and I ll chat with Mike after his break.

@MiiBond MiiBond force-pushed the mbond/support-vertex-pulling branch from a4c7ec9 to 8e92eed Compare July 11, 2025 18:35
@bjsplat
Copy link
Collaborator

bjsplat commented Jul 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jul 11, 2025

@MiiBond
Copy link
Contributor Author

MiiBond commented Jul 12, 2025

I've updated the PR with:

  • vertex pulling boolean is now on the material
  • no need to manually set storage buffers for vertex buffer attributes.

I'm not sure if the way I did this is okay but it seems to work.
I still need to handle the index buffer. My first attempt wasn't successful and I think I have an issue with 16-bit vs 32-bit indices... not sure of the right way to handle this.

@bjsplat
Copy link
Collaborator

bjsplat commented Jul 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jul 12, 2025

@MiiBond
Copy link
Contributor Author

MiiBond commented Jul 12, 2025

Ah, looks like the index buffer in my test is being created as a Uint16Array and WGSL only supports u32 as a type...

@MiiBond
Copy link
Contributor Author

MiiBond commented Jul 21, 2025

Okay, I've added support for 16-bit and 32-bit indices as well as non-indexed geometry.

@MiiBond MiiBond marked this pull request as ready for review July 21, 2025 16:20
@MiiBond MiiBond changed the title WIP - Support vertex pulling Support vertex pulling Jul 21, 2025
@bjsplat
Copy link
Collaborator

bjsplat commented Jul 21, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jul 21, 2025

@@ -1057,6 +1057,7 @@ export abstract class AbstractMesh extends TransformNode implements IDisposable,
protected _buildUniformLayout(): void {
this._uniformBuffer.addUniform("world", 16);
this._uniformBuffer.addUniform("visibility", 1);
this._uniformBuffer.addUniform("hasIndices", 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnot this force changing the shaders as well ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to add this uniform in the mesh ubo?

@@ -2054,13 +2054,17 @@ export class Mesh extends AbstractMesh implements IGetSetVerticesData {

const scene = this.getScene();
const engine = scene.getEngine();
const material = subMesh.getMaterial();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we should add this here as it is inside of the hot path. We should also get the info on the _draw callers ? cc @deltakosh

// If there are vertex buffers that are not bound to the pipeline, AND there are storage buffers
// used by the shader with the same name, we will bind them to the current draw context because they
// are being used to do vertex pulling.
const availableVertexBuffers: { [key: string]: VertexBuffer } = (this._cacheRenderPipeline as any)._vertexBuffers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be amazing to have @Popov72 feedback on it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take a look (as it is, I think it's probably too much work to do in the hot path).

@Popov72
Copy link
Contributor

Popov72 commented Jul 22, 2025

Here's how I would do it:

master...Popov72:Babylon.js:MiiBond-mbond/support-vertex-pulling

Basically, I moved your code to WebGPUDrawContext.setVertexPulling. I modified it slightly to take into account overrideVertexBuffers (which are vertex buffers that take precedence over vertexBuffers when defined).

To avoid running the code on every draw call and impacting performance, I exit early if the vertex pulling parameter (true/false) is the same as the previous call. This means we don't automatically handle a change in the vertex buffer list or if the index buffer changes after the first draw of the mesh. I don't think this is a problem, though, as it's unlikely that we'll need to add/remove a vertex buffer after displaying the mesh (we can still update a buffer). Let's see if anyone complains about this, and we'll take action if necessary. cc @sebavan

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

Successfully merging this pull request may close these issues.

4 participants