Page MenuHome

Fix T80873: Grab active vertex preview not working with shape keys

Authored by Pablo Dobarro (pablodp606) on Sep 17 2020, 3:37 PM.



When a Shape Key is active, use the PBVH deformed coordinates for the

Diff Detail

rB Blender
T80873 (branched from master)
Build Status
Buildable 10263
Build 10263: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Sep 17 2020, 3:37 PM

@Sergey Sharybin (sergey) This fixes the preview when the mesh with shape key does not have a subsurf modifier. In that case, it looks like the PBVH deformed coordinates are also affected by the subsurf. Is this the intended behaivour? Shouldn't Subdivision Surface be considered a constructive modifier and skip deforming the PBVH?


Any specific reason this is inside of if (ss->mvert) { check?

Can this become something like:

/* Always grab active shape key if the sculpt happens on shapekey. */
if (ss->shapekey_active) {
  const MVert *mverts = BKE_pbvh_get_verts(ss->pbvh);
  return mverts[index].co;

/* Sculpting on the base mesh. */
if (ss->mvert) {
  return ss->mvert[index].co;

/* Everything else, such as sculpting on multires. */
return SCULPT_vertex_co_get(ss, index);

Tweak comments if needed.

Shouldn't Subdivision Surface be considered a constructive modifier and skip deforming the PBVH?

This depends on Use Deform Only option. If you would not deform PBVH coordinates with subsurf, you will not be able to easily sculpt on a subdivided low-poly mesh.

@Sergey Sharybin (sergey) But it looks like even with Use Deform Only, subsurf keep deforming the PBVH. In this example, the mesh has a shape key, so the white wireframe is always previewing the coordinates from BKE_pbvh_get_verts.
For the "Grab Active Vertex" functionality to work as expected I think that it idea would be to have the Subsurf modifier enabled, but the PBVH not being deformed.

Can you try applying P1640 ?

NOTE: The patch would need to be adjusted to properly support weight paint where the Only Deform is implicit to False AFAIR.

With that patch it looks like it is not deforming a mesh with shape keys

I asked @Julien Kaspar (JulienKaspar) on, but I think that for low poly meshes (where you will almost use grab brushes all the time), you actually want to deform the mesh using the cage instead of the deformed vertices after subsurf. Maybe we should use the same option that we have for edit mode in this modifier to toggle on/off this behavior in sculpt mode.

The value of the key is set to 0, meaning it has no effect on the final mesh shape.

Set it to 1 for quick test/workaround, and see where is the geometry or PBVH update is missing. Note that this is likely not related to this specific issue you are looking into.

And please understand how system was designed to work, how it is expected to work, and whether there is simply a bug somewhere, or a design limitation. And do not combine bigger design discussions, suggestions, questions with a particular (bug)fix.

I tested this a bit further and it seems to revert the behaviour of sculpting shapekeys to how it worked in 2.83.
But when using the Multires modfieir and enabling the "Sculpt Base Mesh" option instead of using the subdiv modifier it is still broken.
For now this is at least fixing the main issue but the cage should still be the unmodified edit mode geometry. Maybe that can be fixed with a separate patch?
From what I heard the behaviour with multires is more complicated and could be tackled seperately?

@Sergey Sharybin (sergey) @Pablo Dobarro (pablodp606) If this seems fine code wise then I would say push this in as a temp fix. At least with this patch it's no longer broken.
The ideal behaviour can also be discussed and implemented afterwards.

@Pablo Dobarro (pablodp606), I really suggest to get a step back and bring system back in the behavior it was designed for. It should be relatively straightforward process:

  1. Document the branching in this patch.
  2. Finish the crazysapce fix I've linked some comments above.
  3. Fix any other bugs in the current code.

The new/better behavior is possible, but please do it after such crucial for production issue is brought back under control.

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review Update
This revision is now accepted and ready to land.Sep 30 2020, 11:31 AM