Page MenuHome

Sculpt: Sculpt Trimming gestures tools
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Aug 31 2020, 11:16 PM.
Tags
None
Tokens
"Love" token, awarded by Slir."Like" token, awarded by ate1."100" token, awarded by MetinSeven."Party Time" token, awarded by lopoIsaac."Love" token, awarded by Isfuelo."Love" token, awarded by tiagoffcruz."Love" token, awarded by Brandon777."Like" token, awarded by cfnjrey."100" token, awarded by Frozen_Death_Knight.

Details

Summary

This implements Box Trim as a boolean based trimming too gesture in sculpt
mode. This is the intended way to remove parts of the sculpt instead of
using box mask and mask slice. It also creates new face sets for the new
faces created after the boolean operation.

Diff Detail

Repository
rB Blender

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Aug 31 2020, 11:16 PM

@Howard Trickey (howardt) This is creating a trimming mesh and using the code from the boolean modifier to cut the sculpt mesh, so I guess there is a lot of room for improvement. The code that filters the affected PBVH nodes is already in place in case that helps in the future.
There is also the trim line gesture operator that depends on D8722. For that one, is it better to create a plane mesh and use the boolean functions or just use a BMesh bisect like in the old trimming tool patch?

What is the result of this new gesture? Mask? Actually trimmed mesh?

source/blender/editors/sculpt_paint/paint_mask.c
404

Not sure why to use MEM_SAFE_FREE. If the memory did fail to allocate you would have crash anyway. So can simply do MEM_freeN()

@Sergey Sharybin (sergey) This cuts the mesh with a Boolean, exactly the same as D7306. (OBS doesn’t work in my new workstation, so I can’t make videos)

Interesting.
No need in OBS, just the description might be more explicitly worded. Something like:

This implements the sculpt gesture lasso and box operators for trimming operations (removing parts of the mesh), extending existing functionality.

But now it's also a bit confusing. There was a box trimming tool, how is the new one different from it?

Also feels like it would be nice to have Campbell here. Sounds more like his area of expertise than mine.

But now it's also a bit confusing. There was a box trimming tool, how is the new one different from it?

I'm not aware of any other trimming tool. The idea of having this is sculpt mode is to have it integrated with face sets and the rest of the tools and to be able to cut parts of high poly meshes using the PBVH (this is already filtering nodes that are outside the trimming area, so it may be possible to use that info to optimize the boolean solver instead of cutting the entire mesh with a regular boolean modifier)

I think there's a question in here for me, but am not sure. Right now, the cost of testing (using BVH) and excluding some faces from consideration in Boolean is not the dominant cost, so saving that because it is already done in Sculpt is not likely to make a huge improvement. It would be interesting for me to see some profile data on where the time is spent when using this in the kinds of situations you want to use it, to see where the most expensive part is.

Something I've thought about, but haven't done because it seems a bit difficult, is having in input mesh that has some fake faces that represent a large area of mesh that we know doesn't intersect with the part being intersected. The hard part is somehow understanding the "cell" structure of space -- those fake faces must be assumed to fit into space in a way that doesn't intersect other parts (this is hard to explain in words).

Again, seeing some profile data would be the best way to guide where to make improvements here.

A side note: it would be possible to avoid the round trip through BMesh, because the actual Boolean code does not rely on BMesh at all. But it would require code I haven't written yet to get all of the attributes on verts, edges, and faces right.

I'm not aware of any other trimming tool.

The one you've linked to, in D7306.

The one you've linked to, in D7306.

Ah, these are exactly the same tools and they work the same way. I abandoned the other patch because this one shares the code with all the other gesture operators and it is already using newboolean, so the old gesture code just for trimming is no longer relevant.

A side note: it would be possible to avoid the round trip through BMesh, because the actual Boolean code does not rely on BMesh at all.

If that's the case I would advice avoiding this round-trip, as it is quiet costly operation.

Ah, these are exactly the same tools and they work the same way. I abandoned the other patch because this one shares the code with all the other gesture operators and it is already using newboolean, so the old gesture code just for trimming is no longer relevant.

I see.
The D7306 has way better presentation though.

@Howard Trickey (howardt) What is your opinion on this patch as this is now (using the BMesh conversion to apply the boolean)? Do you think we can continue and commit this as it is or it may be worth waiting to have the functions to use the booleans without BMesh conversion to avoid the extra performance cost and change them later when they are available?

@Pablo Dobarro (pablodp606) My opinion is submit it as is, with the trip through BMesh. Better the have the functionality at all, if slow, than not have it at all. Unless you feel the performance is so bad at typical mesh densities as to make it unusable.

I have 3 or 4 accumulated bug reports to deal with before even thinking of trying to write code to avoid the BMesh intraconversion. Even then, I'd like to do profiling to make sure that's a noticeable part of the time compared to other things that I could speed up.

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Cleaunp: Use function pointers for trim gesture operations
  • Rebase
  • Fix Face Set gesture after rebase

@Sergey Sharybin (sergey) I think this should be ready. It still uses the BMesh conversion which can be removed in the future and the patch is now using the function pointers refactor.

@Pablo Dobarro (pablodp606), can not spot anything obviously striking. If Howard/Campbell are fine go ahead with commit.

This revision is now accepted and ready to land.Sep 7 2020, 11:53 AM
This revision was automatically updated to reflect the committed changes.