Sculpt undo does not revert changes propagated to dependent shape keys. #117424

New Issue

Closed

opened 2024-01-22 21:34:51 +01:00 by Alexander Gavrilov · 7 comments

Alexander Gavrilov commented

2024-01-22 21:34:51 +01:00

Member

Blender Version

Broken: 4.1 Alpha
Worked: 4.0.2

Caused by 4d7274b7f4

Short description of error

Sculpting the basis shape key propagates the changes to other shape keys in order to preserve the relative difference. However, undoing does not revert the propagated effects.

Exact steps for others to reproduce the error

Add Basis and Key 1 shape keys to the default cube.
Set the influence of Key 1 to 1.0, but select Basis as active.
Enter Sculpt and paint a stroke.
=> The change is in Basis as expected, and Key 1 has no effect (can be tested by muting).
Undo the stroke.
=> There is no apparent effect, but toggling Key 1 reveals that now it provides the deformation. I.e. undo was applied only to the shape stored in Basis.

**Blender Version** Broken: 4.1 Alpha Worked: 4.0.2 Caused by 4d7274b7f43e572e3d63ec1aac47646415beeea0 **Short description of error** Sculpting the basis shape key propagates the changes to other shape keys in order to preserve the relative difference. However, undoing does not revert the propagated effects. **Exact steps for others to reproduce the error** - Add Basis and Key 1 shape keys to the default cube. - Set the influence of Key 1 to 1.0, but select Basis as active. - Enter Sculpt and paint a stroke. => The change is in Basis as expected, and Key 1 has no effect (can be tested by muting). - Undo the stroke. => There is no apparent effect, but toggling Key 1 reveals that now it provides the deformation. I.e. undo was applied only to the shape stored in Basis.

Alexander Gavrilov added the

labels 2024-01-22 21:34:52 +01:00

Alexander Gavrilov added this to the Sculpt, Paint & Texture project 2024-01-22 21:37:52 +01:00

Philipp Oeser commented

2024-01-23 10:15:43 +01:00

Member

Can confirm, this is a regression from 4.0, will check

Can confirm, this is a regression from 4.0, will check

Philipp Oeser added

Sculpt, Paint & Texture

and removed

labels 2024-01-23 10:16:26 +01:00

Philipp Oeser commented

2024-01-23 14:14:21 +01:00

Member

Caused by 4d7274b7f4

CC @HooglyBoogly

Reverting to something like the following works (not sure exactly why -- and it seems to defeat the purpose of the commit?)

diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.cc b/source/blender/editors/sculpt_paint/sculpt_undo.cc
index b0cc8ff3dbf..4a993e0647a 100644
--- a/source/blender/editors/sculpt_paint/sculpt_undo.cc
+++ b/source/blender/editors/sculpt_paint/sculpt_undo.cc
@@ -445,7 +445,7 @@ static bool restore_coords(
     MutableSpan<float3> positions = ss->vert_positions;
 
     if (ss->shapekey_active) {
-      MutableSpan<float3> vertCos(static_cast<float3 *>(ss->shapekey_active->data),
+      MutableSpan<float3> vertCos(reinterpret_cast<float3 *>(BKE_keyblock_convert_to_vertcos(ob, ss->shapekey_active)),
                                   ss->shapekey_active->totelem);
 
       if (!unode.orig_position.is_empty()) {

Caused by 4d7274b7f43e572e3d63ec1aac47646415beeea0 CC @HooglyBoogly Reverting to something like the following works (not sure exactly why -- and it seems to defeat the purpose of the commit?) ```diff diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.cc b/source/blender/editors/sculpt_paint/sculpt_undo.cc index b0cc8ff3dbf..4a993e0647a 100644 --- a/source/blender/editors/sculpt_paint/sculpt_undo.cc +++ b/source/blender/editors/sculpt_paint/sculpt_undo.cc @@ -445,7 +445,7 @@ static bool restore_coords( MutableSpan<float3> positions = ss->vert_positions; if (ss->shapekey_active) { - MutableSpan<float3> vertCos(static_cast<float3 *>(ss->shapekey_active->data), + MutableSpan<float3> vertCos(reinterpret_cast<float3 *>(BKE_keyblock_convert_to_vertcos(ob, ss->shapekey_active)), ss->shapekey_active->totelem); if (!unode.orig_position.is_empty()) { ```

Alexander Gavrilov commented

2024-01-23 14:31:58 +01:00

Author

Member

Reverting to something like the following works (not sure exactly why -- and it seems to defeat the purpose of the commit?)

Well, there seem to be two major things BKE_keyblock_convert_to_vertcos does:

It copies the data. Btw, does your change ensure memory is freed correctly?
It correctly handles the data layout for curves, but that is probably not relevant here.

The reason things broke is that the actual shape key data should be only changed through SCULPT_vertcos_to_key, which also applies the propagation logic. If you update the internal data directly it sees no changes to propagate.

> Reverting to something like the following works (not sure exactly why -- and it seems to defeat the purpose of the commit?) Well, there seem to be two major things BKE_keyblock_convert_to_vertcos does: 1. It copies the data. Btw, does your change ensure memory is freed correctly? 2. It correctly handles the data layout for curves, but that is probably not relevant here. The reason things broke is that the actual shape key data should be only changed through `SCULPT_vertcos_to_key`, which also applies the propagation logic. If you update the internal data directly it sees no changes to propagate.

Philipp Oeser commented

2024-01-23 16:00:56 +01:00

Member

OK, that means we would need something like this?

diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.cc b/source/blender/editors/sculpt_paint/sculpt_undo.cc
index b0cc8ff3dbf..9fd690991a8 100644
--- a/source/blender/editors/sculpt_paint/sculpt_undo.cc
+++ b/source/blender/editors/sculpt_paint/sculpt_undo.cc
@@ -445,8 +445,8 @@ static bool restore_coords(
     MutableSpan<float3> positions = ss->vert_positions;
 
     if (ss->shapekey_active) {
-      MutableSpan<float3> vertCos(static_cast<float3 *>(ss->shapekey_active->data),
-                                  ss->shapekey_active->totelem);
+      Array<float3> vertCos(Span<float3>(
+          {static_cast<float3 *>(ss->shapekey_active->data), ss->shapekey_active->totelem}));
 
       if (!unode.orig_position.is_empty()) {
         if (ss->deform_modifiers_active) {
@@ -467,11 +467,11 @@ static bool restore_coords(
       }
 
       /* Propagate new coords to keyblock. */
-      SCULPT_vertcos_to_key(ob, ss->shapekey_active, vertCos);
+      SCULPT_vertcos_to_key(ob, ss->shapekey_active, vertCos.as_span());
 
       /* PBVH uses its own vertex array, so coords should be */
       /* propagated to PBVH here. */
-      BKE_pbvh_vert_coords_apply(ss->pbvh, vertCos);
+      BKE_pbvh_vert_coords_apply(ss->pbvh, vertCos.as_span());
     }
     else {
       if (!unode.orig_position.is_empty()) {

4d7274b7f4 also got rid of other usages of BKE_keyblock_convert_to_vertcos (along with its "copying" behavior), so these might have to be checked as well @HooglyBoogly ?

OK, that means we would need something like this? ```diff diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.cc b/source/blender/editors/sculpt_paint/sculpt_undo.cc index b0cc8ff3dbf..9fd690991a8 100644 --- a/source/blender/editors/sculpt_paint/sculpt_undo.cc +++ b/source/blender/editors/sculpt_paint/sculpt_undo.cc @@ -445,8 +445,8 @@ static bool restore_coords( MutableSpan<float3> positions = ss->vert_positions; if (ss->shapekey_active) { - MutableSpan<float3> vertCos(static_cast<float3 *>(ss->shapekey_active->data), - ss->shapekey_active->totelem); + Array<float3> vertCos(Span<float3>( + {static_cast<float3 *>(ss->shapekey_active->data), ss->shapekey_active->totelem})); if (!unode.orig_position.is_empty()) { if (ss->deform_modifiers_active) { @@ -467,11 +467,11 @@ static bool restore_coords( } /* Propagate new coords to keyblock. */ - SCULPT_vertcos_to_key(ob, ss->shapekey_active, vertCos); + SCULPT_vertcos_to_key(ob, ss->shapekey_active, vertCos.as_span()); /* PBVH uses its own vertex array, so coords should be */ /* propagated to PBVH here. */ - BKE_pbvh_vert_coords_apply(ss->pbvh, vertCos); + BKE_pbvh_vert_coords_apply(ss->pbvh, vertCos.as_span()); } else { if (!unode.orig_position.is_empty()) { ``` 4d7274b7f4 also got rid of other usages of `BKE_keyblock_convert_to_vertcos` (along with its "copying" behavior), so these might have to be checked as well @HooglyBoogly ?

Hans Goudey commented

2024-01-23 16:35:16 +01:00

Member

Thanks for investigating this. Might as well just revert that part of the commit and switch back to BKE_keyblock_convert_to_vertcos in this case.

As background, part of the reason there was often a separately allocated array is that there was a distinction between MVert and the float[3] data stored for shape keys. I've been trying to remove that slowly over time, But I guess these have to be separate arrays for BKE_pbvh_vert_coords_apply to work properly.

Thanks for investigating this. Might as well just revert that part of the commit and switch back to `BKE_keyblock_convert_to_vertcos` in this case. As background, part of the reason there was often a separately allocated array is that there was a distinction between `MVert` and the `float[3]` data stored for shape keys. I've been trying to remove that slowly over time, But I guess these have to be separate arrays for `BKE_pbvh_vert_coords_apply` to work properly.

Philipp Oeser commented

2024-01-24 09:25:09 +01:00

Member

@HooglyBoogly : will you take care of this?

@HooglyBoogly : will you take care of this?

Alexander Gavrilov commented

2024-01-24 12:14:06 +01:00

Author

Member

As background, part of the reason there was often a separately allocated array is that there was a distinction between MVert and the float[3] data stored for shape keys. I've been trying to remove that slowly over time, But I guess these have to be separate arrays for BKE_pbvh_vert_coords_apply to work properly.

Shape keys aren't simple coordinate arrays, for some reason they are designed to support more complex layouts, and the purpose of BKE_keyblock_convert_to_vertcos is to encapsulate the internal structure. Currently for meshes they are actually simple coordinate arrays, but curves use more complex structs (and moreover they are different for bezier and b-spline segments, which can be mixed in the same curve datablock - so for curves the shape key buffer isn't even an array with one fixed element size).

For modification the important point is that to propagate changes to other keys it's necessary to know what the changes are, which means comparing the old an new values, so you can't modify shape key data in place.

> As background, part of the reason there was often a separately allocated array is that there was a distinction between `MVert` and the `float[3]` data stored for shape keys. I've been trying to remove that slowly over time, But I guess these have to be separate arrays for `BKE_pbvh_vert_coords_apply` to work properly. Shape keys aren't simple coordinate arrays, for some reason they are designed to support more complex layouts, and the purpose of `BKE_keyblock_convert_to_vertcos` is to encapsulate the internal structure. Currently for meshes they are actually simple coordinate arrays, but curves use more complex structs (and moreover they are different for bezier and b-spline segments, which can be mixed in the same curve datablock - so for curves the shape key buffer isn't even an array with one fixed element size). For modification the important point is that to propagate changes to other keys it's necessary to know what the changes are, which means comparing the old an new values, so you can't modify shape key data in place.

Hans Goudey referenced this issue from a commit

2024-01-25 21:42:24 +01:00

Fix #117424: Sculpt undo misses changes propagated to shape keys

Hans Goudey closed this issue

2024-01-25 21:42:26 +01:00

Blender Bot added

and removed

labels 2024-01-25 21:42:28 +01:00

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117424

No description provided.