Fix T84202: Sculpt lasso mask crash after remesh.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Dec 29 2020, 2:56 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Pablo Dobarro (pablodp606)

Maniphest Tasks

T84202: Sculpt crash after remesh (mask operators, cloth brush, ...)

Summary

'Caused'/revealed by rBd29a720c45e5: Operators that fully re-create the
mesh would previously rely on sculpt_update_object called from update
code to get required sculpt-specific data layers re-added to the new
mesh.

Now instead put all code adding data to orig mesh for sculpt purpose
into a new util function (BKE_sculpt_ensure_orig_mesh_data), and call
that function when entering sculpt mode, and from voxel remesher code.

This is contonuing effort to more clearly separate orig data from evaluated
data handling/usage in sculpt code.

TODO: there are likely other code paths that would need to call that
new function?

Diff Detail

Repository

rB Blender

Branch

T84202 (branched from master)

Build Status

Buildable 11972
Build 11972: arc lint + arc unit

Event Timeline

Bastien Montagne (mont29) created this revision.Dec 29 2020, 2:56 PM

Harbormaster completed remote builds in B11911: Diff 32323.Dec 29 2020, 2:56 PM

Bastien Montagne (mont29) requested review of this revision.Dec 29 2020, 2:56 PM

TheRedWaxPolice (TheRedWaxPolice) awarded a token.Dec 29 2020, 3:07 PM

TheRedWaxPolice (TheRedWaxPolice) added a subscriber: TheRedWaxPolice (TheRedWaxPolice).

A way to check the codepaths that can change the sculpt mesh is by searching for ED_sculpt_undo_geometry_end. Maybe BKE_sculpt_ensure_orig_mesh_data can be put there?
But in any case, with this solution, what would happen if an addon changes the mesh datablock of the object while in sculpt mode with another one that does not have the mask datalayer?

Some tweak, add quadriflow remsehing handling too.

AFAICT, all other usages of ED_sculpt_undo_geometry_end (like mesh extract,
or mirror) already deal properly with those CData layers?

Harbormaster completed remote builds in B11913: Diff 32325.Dec 29 2020, 5:16 PM

In D9951#248431, @Pablo Dobarro (pablodp606) wrote:

A way to check the codepaths that can change the sculpt mesh is by searching for ED_sculpt_undo_geometry_end. Maybe BKE_sculpt_ensure_orig_mesh_data can be put there?

I'd rather not mix some undo code with some logic to update orig mesh data... Further more, afaict not all usgaes of ED_sculpt_undo_geometry_end would require BKE_sculpt_ensure_orig_mesh_data?

But in any case, with this solution, what would happen if an addon changes the mesh datablock of the object while in sculpt mode with another one that does not have the mask datalayer?

Do we actually support that????????? if we do, then it should be up to RNA set/update code to deal with it and call BKE_sculpt_ensure_orig_mesh_data I guess.

Do we actually support that?????????

That is how the Keymesh script works https://vimeo.com/416934850
Probably any other addon that does some kind of mesh manipulation in sculpt mode (like cutting masks, inserting geometry...) will also work this way.

Add update orig mesh call to RNA obdata update function.

That way scripts should be able to replace meshes during sculpt mode as well.

@Pablo Dobarro (pablodp606) would be nice if you could confirm this is working with Keymesh.

Harbormaster completed remote builds in B11921: Diff 32335.Dec 30 2020, 11:05 AM

I tried with a simple script that changes the mesh datablock while in sculpt mode with another one that does not have the mask datalayer and it works fine.

source/blender/blenkernel/intern/paint.c
2046	This needs be called both for all PBVH tyes. The CD_PAINT_MASK datalayers also needs to be created when multires is not being used, otherwise it will crash.

This revision now requires changes to proceed.Dec 30 2020, 10:41 PM

Bastien Montagne (mont29) added inline comments.Dec 31 2020, 10:56 AM

source/blender/blenkernel/intern/paint.c
2046	Are you sure about that? original code did not create it in non-multires case, and from a quick try here (default cube, switch to sculpt, draw mask), it seems to be handled properly?

Lopo Isaac (lopoIsaac) awarded a token.Dec 31 2020, 12:52 PM

Pablo Dobarro (pablodp606) added inline comments.Dec 31 2020, 6:29 PM

source/blender/blenkernel/intern/paint.c

2046

Yes. I get a crash doing those same steps. That function also allocates the mask for the base mesh if it does not exists. The original code in sculpt_update_object was the following

if (mmd == NULL) {
  if (!CustomData_has_layer(&me->vdata, CD_PAINT_MASK)) {
    BKE_sculpt_mask_layers_ensure(ob, NULL);
  }
}
else {
  if (!CustomData_has_layer(&me->ldata, CD_GRID_PAINT_MASK)) {
    BKE_sculpt_mask_layers_ensure(ob, mmd);
  }
}

Bastien Montagne (mont29) added inline comments.Jan 4 2021, 12:53 PM

source/blender/blenkernel/intern/paint.c
2046	Uuuuuh yes... butoriginal code did not when called from from `sculpt_init_session` (i.e. `ED_object_sculptmode_enter_ex`)... OK, will add it back systematically, original code was horribly confusing and wrong anyway, we can always try to optimize this later if it's really worth it, but for now let's aim at security.