As I understand it, the draw manager regular mesh drawing handles multiple viewports by allocating additional batches for attributes as needed. Is there a reason this needs to rebuild batches?

I think it would be good to use regular mesh logic and data structures as much as possible, with the aim of eventually moving this code into the draw module and integrating as just another draw_cache_impl_ file.

In D15428#418374, @Brecht Van Lommel (brecht) wrote:

As I understand it, the draw manager regular mesh drawing handles multiple viewports by allocating additional batches for attributes as needed. Is there a reason this needs to rebuild batches?

I think it would be good to use regular mesh logic and data structures as much as possible, with the aim of eventually moving this code into the draw module and integrating as just another draw_cache_impl_ file.

That would take too much VRAM bandwidth. Remember that while most sculpt tools incrementally update the GPU mesh, some do not (e.g. elastic deform, the filters).

In D15428#422308, @Joseph Eagar (joeedh) wrote:

That would take too much VRAM bandwidth. Remember that while most sculpt tools incrementally update the GPU mesh, some do not (e.g. elastic deform, the filters).

Is the idea that interleaved storage in one buffer is faster than multiple smaller buffers? It's not obvious to me that this saves much VRAM bandwith.

But regardless, since the data already seems to be interleaved it wouldn't make sense to do the work of changing that and verifying if it helps as part of this refactor, since it's a much bigger change. So no need to revisit that here.

The overall idea of splitting the updating and drawing into two separate parts make sense.

It's difficult to review this patch since it seems to be work in progress still. Changes to remove the max vcol limit from operators/modifiers also should not be part of this.

So I'm not sure if you are asking for code review here, design feedback, .. ? Or if this is just a WIP that needs no feedback right now.

In D15428#423478, @Brecht Van Lommel (brecht) wrote:

In D15428#422308, @Joseph Eagar (joeedh) wrote:

That would take too much VRAM bandwidth. Remember that while most sculpt tools incrementally update the GPU mesh, some do not (e.g. elastic deform, the filters).

Is the idea that interleaved storage in one buffer is faster than multiple smaller buffers? It's not obvious to me that this saves much VRAM bandwith.

But regardless, since the data already seems to be interleaved it wouldn't make sense to do the work of changing that and verifying if it helps as part of this refactor, since it's a much bigger change. So no need to revisit that here.

Sorry, I wasn't clear. I just meant that PBVH shares GPU batches across all open viewports. Having separate VBOs with different attribute layouts for each open viewport is not feasible; it would use an enormous amount of VRAM bandwidth to send all that duplicate data to the GPU. The better solution is to merge the attribute layouts from all viewports into a single shared set of VBOs.

I did not review it yet, but giving my two cents on what has been discussed.

Is the idea that interleaved storage in one buffer is faster than multiple smaller buffers? It's not obvious to me that this saves much VRAM bandwith.

Do not use interlieved storage in new code. It can be useful for packing small attributes together but has no real benefit for attributes that are at least 4bytes long. Most not so old GPUs will have enough caches for each individual input streams. Meaning that separate VBOs don't have any performance penalty on the GPU. Also this makes the CPU code much simpler and filling the VBOs should be just a memcpy. I am not aware if this is the current design of the PBVH drawing.

Each attribute should be its own VBO as we now have GPU_BATCH_VBO_MAX_LEN = 16. With this, you can have one GPUBatch per viewport / Attribute set. And thoses should not be a big overhead to keep cached.

I would like to make a precision: When I wrote in T97665 that we need 1 VBO for the whole PBVH I was talking about positions and normals. Attributes can still be in their own VBOs. I was also talking about not fragmenting the mesh into small VBOs.

In D15428#423985, @Clément Foucault (fclem) wrote:

I did not review it yet, but giving my two cents on what has been discussed.

Is the idea that interleaved storage in one buffer is faster than multiple smaller buffers? It's not obvious to me that this saves much VRAM bandwith.

Do not use interlieved storage in new code. It can be useful for packing small attributes together but has no real benefit for attributes that are at least 4bytes long. Most not so old GPUs will have enough caches for each individual input streams. Meaning that separate VBOs don't have any performance penalty on the GPU. Also this makes the CPU code much simpler and filling the VBOs should be just a memcpy. I am not aware if this is the current design of the PBVH drawing.

Each attribute should be its own VBO as we now have GPU_BATCH_VBO_MAX_LEN = 16. With this, you can have one GPUBatch per viewport / Attribute set. And thoses should not be a big overhead to keep cached.

I would like to make a precision: When I wrote in T97665 that we need 1 VBO for the whole PBVH I was talking about positions and normals. Attributes can still be in their own VBOs. I was also talking about not fragmenting the mesh into small VBOs.

I hadn't realized you could attach more than one VBO to a batch. I'm going to play around with this.

Okay, we're going to have to completely rewrite this. I don't think we can reuse any code from the draw cache system.

@Clément Foucault (fclem) @Kévin Dietrich (kevindietrich) This patch is ready for another round of review. Since various high priority bugs depend on this patch it would be ideal to land this soon.