The mesh filter was using the functions for standard brush updates, but
bounding box and normals updates are not necessary to compute when the
filter is running.
By only redrawing the mesh and tagging the normals to update when
necessary the filter should be more responsive. Bounding boxes and
normals are always updated when the filter ends.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- sculpt-mesh-filter-updates (branched from master)
- Build Status
Buildable 10522 Build 10522: arc lint + arc unit
Event Timeline
Comment Actions
Found several issues noted in comments below.
Generally logic seems fine, but I am missing some general understanding of this area of code to do a proper review on that aspect to be honest.
| source/blender/blenkernel/intern/pbvh.c | ||
|---|---|---|
| 3075–3077 | Not sure I understand those checks, are you assuming that if the first item is tagged as smooth then everything is smooth (and conversely) ? | |
| 3075–3079 | Those return a boolean, please use proper 'conversion' to bool then (return pbvh->(mpoly[0].flag & ME_SMOOTH) != 0;, etc.) | |
| 3079 | I believe this should have been & ? | |
| source/blender/editors/sculpt_paint/sculpt.c | ||
| 7444–7447 | Those lines can be removed, you already call ED_region_tag_redraw() above… | |
| source/blender/editors/sculpt_paint/sculpt_filter_mesh.c | ||
| 701 | This comment should be moved into the new sculpt_mesh_filter_needs_normal_updates helper. | |
| source/blender/blenkernel/intern/pbvh.c | ||
|---|---|---|
| 3075–3077 | Yes, that is how drawing in sculpt mode works. It can only draw the entire mesh in smooth or in flat shading, depending on the first element. | |
| source/blender/blenkernel/intern/pbvh.c | ||
|---|---|---|
| 3075–3077 | Is it possible to deduplicate that logic? Like call this function in pbvh_update_draw_buffer_cb and pass that to the GPU functions? | |
| source/blender/editors/sculpt_paint/sculpt.c | ||
| 7431 | This function is named as if it's a general redraw function for sculpt tools, but it's only used specifically for filtering. It seems to duplicate some subset of SCULPT_flush_update_step. I find it hard to understand what is supposed to be called when. Would this be more clear as a new SculptUpdateType for SCULPT_flush_update_step? | |
| source/blender/blenkernel/intern/pbvh.c | ||
|---|---|---|
| 3075–3077 | Yes, I can use this function in the drawing code. Should that be part of this patch? | |
| source/blender/editors/sculpt_paint/sculpt.c | ||
| 7431 | This optimization is only implemented for the filter, but the idea is to add the same functions to the paint_brush operator. So, brushes that don't need bounding boxes and normal updates will only redraw after each step and update everything in a single step when the stroke ends, brushes that needs update will use SCULPT_flush_update_step with the required updates. | |
Comment Actions
- review update
| source/blender/editors/sculpt_paint/sculpt_filter_mesh.c | ||
|---|---|---|
| 701 | But this comment says the same as the name of the function, should I add it anyway? | |
| source/blender/editors/sculpt_paint/sculpt_filter_mesh.c | ||
|---|---|---|
| 701 | Ah, fair point. | |
Comment Actions
Is this ok for 2.92? I'm now testing the performance of the normal updates with the filter and I would like to check how things work without bounding box updates as well
Comment Actions
It's not obvious to me that this is correct. Aren't bounding boxes used for clipping nodes that are outside the viewport, and so if they are wrong it could skip drawing nodes that come into the viewport as the result of the filter?
It's also not clear to me why a filter like smooth does not need normal updates.
| source/blender/editors/sculpt_paint/sculpt.c | ||
|---|---|---|
| 7431 | The naming is still not obvious, nor is there a comment explaining the difference. The function should be named in such a way that is clear that this is some faster version of SCULPT_flush_update_step, or merged and adding a parameter to indicate some work can be skipped. | |
Comment Actions
It's not obvious to me that this is correct. Aren't bounding boxes used for clipping nodes that are outside the viewport, and so if they are wrong it could skip drawing nodes that come into the viewport as the result of the filter?
True, but I would consider that a corner case. Updating the bounding boxes of the entire mesh takes a lot of time of each filter iteration that in 99% of the cases is not going to be used (and when it fails, it is just a temporally viewport glitch, the deformation will still be correct). Also, the "delay viewport updates" does a similar thing when navigating the viewport.
It's also not clear to me why a filter like smooth does not need normal updates.
Filters that don't read the normals to calculate the new position don't need to update them after each iteration, they work the same with invalid normals. When the filter ends, all normals are recalculated at once to leave the mesh ready for the next action.
After this patch, I'm planning to add this same optimization to the brushes, as there are a lot of them that don't need normals or bounding box updates to work correctly, but more than half of their execution time is spent on updating this data (the grab brush, for example).
Comment Actions
For normals it seems indeed correct. For the bounding boxes, personally I would advise against doing this kind of optimization, unless it gives a really big speedup I don't think it's good to make refreshes less reliable.
Comment Actions
I tested the performance of skipping the bounding box updates. With a mesh of 1.6M vertices and using inflate, the modal callback of the filter goes from 0.0045s to 0.0070s, so 35% of the time is wasted.
If viewport clipping is an issue maybe bounding box updates can be time limited so they happen only once per second?