Sculpt: Improve PBVH draw performance for meshes #110621

Hans Goudey · 2023-07-30T06:08:41+02:00

Hans Goudey commented

2023-07-30 06:08:41 +02:00

Reduce overhead of copying attribute data into GPU buffers when the
PBVH is active. The existing lambda with a FunctionRef callback had
a significant overhead. While that was reduced by 25917f0165
already, even making the foreach_faces lambda into a template gave
significant overhead compared to simpler loops. Instead, separate
value conversion and iteration over visible triangles in a way that the
compiler is able to optimize more easily.

According to the GPU module, it's also better to use raw data access
than GPU_vertbuf_raw_step, since the data format strides aren't
meant to vary by platform, and the runtime stride can have a
noticeable performance impact.

Also avoid recalculating face normals, since they're already used to
calculate vertex normals anyway (since ac02f94caf).

I tested the runtime of the initial data-upload after entering sculpt
mode with a 16 million vertex mesh. Before, that took 1350 ms, after
it took 680 ms, which is almost a 2x improvement.

It's possible that a similar optimization could be applied to multires
or dynamic topology sculpting, but that can be looked at later too.

Reduce overhead of copying attribute data into GPU buffers when the PBVH is active. The existing lambda with a FunctionRef callback had a significant overhead. While that was reduced by 25917f0165b5feea6f04 already, even making the `foreach_faces` lambda into a template gave significant overhead compared to simpler loops. Instead, separate value conversion and iteration over visible triangles in a way that the compiler is able to optimize more easily. According to the GPU module, it's also better to use raw data access than `GPU_vertbuf_raw_step`, since the data format strides aren't meant to vary by platform, and the runtime stride can have a noticeable performance impact. Also avoid recalculating face normals, since they're already used to calculate vertex normals anyway (since ac02f94caf5bba5218579). I tested the runtime of the initial data-upload after entering sculpt mode with a 16 million vertex mesh. Before, that took 1350 ms, after it took 680 ms, which is almost a 2x improvement. It's possible that a similar optimization could be applied to multires or dynamic topology sculpting, but that can be looked at later too.

👍 3 ❤️ 3 🎉 3 🚀 3

Hans Goudey added 2 commits 2023-07-30 06:08:53 +02:00

377126670e Replace lambda with FunctionRef callback

40543b7365 Avoid recalculating face normals

Hans Goudey added this to the Sculpt, Paint & Texture project 2023-07-30 06:09:02 +02:00

Hans Goudey requested review from Sergey Sharybin 2023-07-30 06:09:09 +02:00

Ray molenkamp reviewed 2023-07-30 07:01:20 +02:00

source/blender/draw/intern/draw_pbvh.cc

						
				@ -116,0 +146,4 @@

				  return {unit_float_to_ushort_clamp(BLI_color_from_srgb_table[value.r]),

				          unit_float_to_ushort_clamp(BLI_color_from_srgb_table[value.g]),

				          unit_float_to_ushort_clamp(BLI_color_from_srgb_table[value.b]),

				          ushort(value.a * 257)};

Ray molenkamp commented

2023-07-30 07:01:20 +02:00

257 feels out of place, this is either a typo or needs a comment.

Hans Goudey commented

2023-07-31 03:39:13 +02:00

Yeah, not sure about that :/ It's from 65900d88a8. I just moved the existing conversion to a separate function here.

Yeah, not sure about that :/ It's from 65900d88a8317c207885ae4a3993272112114f36. I just moved the existing conversion to a separate function here.

Sergey Sharybin commented

2023-07-31 11:56:52 +02:00

It is indeed confusing, bot the the purpose of this patch it feels better to minimize the scope of the change, and handle this conversion quirk separately. This could include:

Use proper scaling factor or document why 257 is used.
The BLI_color_from_srgb_table is backed for the 0 .. 255 range of sRGB values, which maps to 0 .. 1 linear pace values, so no t obvious why clamping is needed. We can save some branching here.
We might want to investigate use of srgb_to_linearrgb_v3_v3 instead of a LUT, as it could give better performance.

It is indeed confusing, bot the the purpose of this patch it feels better to minimize the scope of the change, and handle this conversion quirk separately. This could include: - Use proper scaling factor or document why 257 is used. - The `BLI_color_from_srgb_table` is backed for the 0 .. 255 range of sRGB values, which maps to 0 .. 1 linear pace values, so no t obvious why clamping is needed. We can save some branching here. - We might want to investigate use of `srgb_to_linearrgb_v3_v3` instead of a LUT, as it could give better performance.

Evan Wilson commented

2023-08-07 22:02:47 +02:00

It appears that it was:
scol[3] = unit_float_to_ushort_clamp(mcol2->a * (1.0f / 255.0f))
prior to that commit.

Source: https://archive.blender.org/developer/D15428

It appears that it was: `scol[3] = unit_float_to_ushort_clamp(mcol2->a * (1.0f / 255.0f))` prior to that commit. Source: https://archive.blender.org/developer/D15428

Sergey Sharybin approved these changes 2023-07-31 12:00:38 +02:00

Sergey Sharybin left a comment

2x is a great speedup!

Does it only affect the initial data upload, or does it also helps updates when doing brush strokes?

From reading the code it seems fine. Couple of minor thing at the end of the patch, which I do not think worth an extra review iteration.

2x is a great speedup! Does it only affect the initial data upload, or does it also helps updates when doing brush strokes? From reading the code it seems fine. Couple of minor thing at the end of the patch, which I do not think worth an extra review iteration.

source/blender/draw/intern/draw_pbvh.cc Outdated

						
				@ -1359,13 +1359,16 @@ GPUBatch *DRW_pbvh_lines_get(PBVHBatches *batches,

				                             int *r_prim_count,

				                             bool do_coarse_grids)

				{

				  // SCOPED_TIMER(__func__);

Sergey Sharybin commented

2023-07-31 11:58:00 +02:00

Perhaps can be removed?

HooglyBoogly marked this conversation as resolved

source/blender/draw/intern/draw_pbvh.cc Outdated

						
				@ -1367,2 +1368,4 @@

				  *r_prim_count = 0;

				  return batch.lines;

				  return nullptr;

Sergey Sharybin commented

2023-07-31 11:58:15 +02:00

And this quite sure can be removed :)

HooglyBoogly marked this conversation as resolved

Hans Goudey added 2 commits 2023-07-31 14:18:25 +02:00

7e2cc9a8fe Merge branch 'main' into pbvh-draw-performance

175983f7cb Remove testing code in DRW_pbvh_lines_get

Hans Goudey commented

2023-07-31 14:25:55 +02:00

Does it only affect the initial data upload, or does it also helps updates when doing brush strokes?

It seems to only have a noticeable effect on the first data upload. That's a bit strange actually, since it's the same code, so in theory I'd expect it to improve regular sculpting too, since a lot of the improvement I found was from the change to the position upload.

>Does it only affect the initial data upload, or does it also helps updates when doing brush strokes? It seems to only have a noticeable effect on the first data upload. That's a bit strange actually, since it's the same code, so in theory I'd expect it to improve regular sculpting too, since a lot of the improvement I found was from the change to the position upload.

Hans Goudey merged commit f10965dcb8 into main

2023-07-31 14:40:47 +02:00

Hans Goudey referenced this issue from a commit

2023-07-31 14:40:48 +02:00

Sculpt: Improve PBVH draw performance for meshes

Hans Goudey deleted branch pbvh-draw-performance

2023-07-31 14:40:49 +02:00

Julien Kaspar added the

Module

Sculpt, Paint & Texture

label 2023-08-25 11:35:45 +02:00

Julien Kaspar removed this from the Sculpt, Paint & Texture project 2023-08-25 11:35:49 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Sculpt: Improve PBVH draw performance for meshes #110621