Page MenuHome

Fix T93113: Redo Panel of transform sculpt operation does not work properly
AbandonedPublic

Authored by Germano Cavalcante (mano-wii) on Jan 21 2022, 6:19 PM.

Details

Summary

The way sculpt Undo works is problematic for the redo panel (at least
for the transform operator).
When editing the values in the redo panel, they keep accumulating.

In the Redo panel, when changing a value, a Undo step needs to be
performed in order to restore the previous state of the mesh.

But sculpt_undosys_step_decode_undo ignores this Undo step because it
is_final (For some reason is_final is ignored).

Therefore, since an undo step named "Transform" is added at the
beginning of the operation (in ED_sculpt_init_transform), we can identify
this undo step and not ignore it:
if (STREQ(us->step.name, "Transform"))

In addition to this change, the value of ss->pivot_scale also needed
to be stored in the undo step (in unode->pivot_scale).

I also edited the transform operator as it was initializing some values
out of place (ss->pivot_loc, ss->pivot_rot, ss->pivot_scale).
These values are now initialized in BKE_object_sculpt_data_create).

Ref T93113

Diff Detail

Repository
rB Blender
Branch
master
Build Status
Buildable 20108
Build 20108: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Jan 21 2022, 6:19 PM
Germano Cavalcante (mano-wii) created this revision.
Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)
Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)

Already solved by rB3f9299e45d39: Sculpt: Fix redo panel bugs.
With that commit, is_final is now false, so sculpt_undosys_step_decode_undo no longer ignores the Undo Step (and we no longer need the STREQ(us->step.name, "Transform") workaround).
I didn't realize that ED_undo_pop_op fetches the operator name to identify the Undo Step and so say if it's final or not. (Seems like a weak assumption imho).

The other changes regarding pivot_scale in this patch are not really needed. The solution and always considering the initial scale as [1, 1, 1] works too.
The other proposed changes are just cosmetic.