Fix #120761: Tag all PBVH nodes for needing a visibility update when using hide tools #120798

Sean Kim · 2024-04-18T23:22:31+02:00

Sean Kim commented

2024-04-18 23:22:31 +02:00

This PR changes behavior in the Sculpt mode Hide Gesture tools to always tag every PBVH node for a visibility update to avoid faces not updating when they are on the boundary of different nodes.

Alternate Approaches

A smarter approach could be done by comparing the bounds of each PBVH and only tagging adjacent nodes, however the added complexity does not seem worth it over the current implementation.

This PR changes behavior in the Sculpt mode Hide Gesture tools to always tag every PBVH node for a visibility update to avoid faces not updating when they are on the boundary of different nodes. ## Alternate Approaches * A smarter approach could be done by comparing the bounds of each PBVH and only tagging adjacent nodes, however the added complexity does not seem worth it over the current implementation.

Sean Kim added 1 commit 2024-04-18 23:22:41 +02:00

c4f6beaead Fix #120761 : Tag all PBVH nodes for needing a visibility update when using hide tools

Sean Kim requested review from Hans Goudey 2024-04-18 23:23:23 +02:00

Hans Goudey commented

2024-04-22 23:51:17 +02:00

I think this is actually an important optimization. Changing visibility should feel very snappy, even with meshes that have millions of faces. This requires rebuilding the GPU buffers for every single PBVH node. For reference, the change was made in 4e66769ec0, and a significant part of the performance improvement comes from not rebuilding the GPU buffers.

It's not just that this optimization in particularly is that important though, but I think it helps us to learn about how to do these operations in a non-global way. Some ideas:

Change hide update functions to work on faces rather than vertices (like the existing face_hide_update)
Handle vert to face visibility flushing ourselves in a separate loop of affected nodes. Also tag any nodes changed there.
Calculate new hide values into a newly allocated mesh-sized array, test differences on the node level afterwards
Do calculation for non-unique verts in existing loop, just to check if there are changes.

Testing which of those is fastest for cases like hiding a small portion of a 16 million vertex mesh might be fun. I can take this over though if it sounds like too much.

I think this is actually an important optimization. Changing visibility should feel very snappy, even with meshes that have millions of faces. This requires rebuilding the GPU buffers for every single PBVH node. For reference, the change was made in 4e66769ec09461384af48b0bfdafc28884f01d19, and a significant part of the performance improvement comes from not rebuilding the GPU buffers. It's not just that this optimization in particularly is _that_ important though, but I think it helps us to learn about how to do these operations in a non-global way. Some ideas: 1. Change hide update functions to work on faces rather than vertices (like the existing `face_hide_update`) 2. Handle vert to face visibility flushing ourselves in a separate loop of affected nodes. Also tag any nodes changed there. 3. Calculate new hide values into a newly allocated mesh-sized array, test differences on the node level afterwards 4. Do calculation for non-unique verts in existing loop, just to check if there are changes. Testing which of those is fastest for cases like hiding a small portion of a 16 million vertex mesh might be fun. I can take this over though if it sounds like too much.

👍 1

Sean Kim commented

2024-04-23 00:37:09 +02:00

Thanks for the further context on the GPU side of things. I wasn't aware of the follow up changes that this would cause - I can try tackling & thinking about the different approaches.

In general, one of the things about this area of code that I'd like to try and minimize is needing to remember to tag the PBVH node for visibility updates in the higher level operator code since that seems like an area that can easily have regressions. This makes me not inclined to explore 2. in a lot of depth as I feel like that goes in the opposite direction, but I'd be interested in hearing your thoughts on this too.

Edit: Didn't mean to link to that random issue.

Thanks for the further context on the GPU side of things. I wasn't aware of the follow up changes that this would cause - I can try tackling & thinking about the different approaches. In general, one of the things about this area of code that I'd like to try and minimize is needing to remember to tag the PBVH node for visibility updates in the higher level operator code since that seems like an area that can easily have regressions. This makes me not inclined to explore 2. in a lot of depth as I feel like that goes in the opposite direction, but I'd be interested in hearing your thoughts on this too. Edit: Didn't mean to link to that random issue.

Hans Goudey commented

2024-04-23 15:59:16 +02:00

In general, one of the things about this area of code that I'd like to try and minimize is needing to remember to tag the PBVH node for visibility updates in the higher level operator code since that seems like an area that can easily have regressions. This makes me not inclined to explore 2. in a lot of depth as I feel like that goes in the opposite direction, but I'd be interested in hearing your thoughts on this too.

I agree with not requiring visibility updates outside of vert_hide_update. But I don't think option 2 requires that. vert_hide_update would just contain a second loop over nodes and would run the vertex -> face visibility propagation there, tagging the node if any hide_poly value changed.

>In general, one of the things about this area of code that I'd like to try and minimize is needing to remember to tag the PBVH node for visibility updates in the higher level operator code since that seems like an area that can easily have regressions. This makes me not inclined to explore 2. in a lot of depth as I feel like that goes in the opposite direction, but I'd be interested in hearing your thoughts on this too. I agree with not requiring visibility updates outside of `vert_hide_update`. But I don't think option 2 requires that. `vert_hide_update` would just contain a second loop over nodes and would run the vertex -> face visibility propagation there, tagging the node if any `hide_poly` value changed.

Sean Kim commented

2024-04-24 02:28:19 +02:00

I agree with not requiring visibility updates outside of vert_hide_update. But I don't think option 2 requires that. vert_hide_update would just contain a second loop over nodes and would run the vertex -> face visibility propagation there, tagging the node if any hide_poly value changed.

I should have explained my reasoning a bit further - my thoughts were a bit incomplete when I made that comment. I believe that duplicating / mirroring logic that exists inside the bke::mesh_hide_vert_flush function into vert_hide_update seems like an unnecessary amount of overlap in functionality.

> I agree with not requiring visibility updates outside of `vert_hide_update`. But I don't think option 2 requires that. `vert_hide_update` would just contain a second loop over nodes and would run the vertex -> face visibility propagation there, tagging the node if any `hide_poly` value changed. I should have explained my reasoning a bit further - my thoughts were a bit incomplete when I made that comment. I believe that duplicating / mirroring logic that exists inside the `bke::mesh_hide_vert_flush` function into `vert_hide_update` seems like an unnecessary amount of overlap in functionality.

This pull request has changes conflicting with the target branch.

source/blender/editors/sculpt_paint/paint_hide.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u fix-120761:Sean-Kim-fix-120761

git checkout Sean-Kim-fix-120761

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

Fix #120761: Tag all PBVH nodes for needing a visibility update when using hide tools #120798

Alternate Approaches

Checkout