Fix undo overreach in various paint modes for #69760 and related. #117417

Open
Alexander Gavrilov wants to merge 1 commits from angavrilov/blender:pr-undo-fixes into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
7 changed files with 70 additions and 16 deletions

View File

@ -16,6 +16,7 @@ struct CLG_LogRef;
struct Object;
struct Scene;
struct MemFile;
struct PropertyRNA;
struct UndoStack;
struct ViewLayer;
struct bContext;
@ -71,7 +72,9 @@ bool ED_undo_is_memfile_compatible(const bContext *C);
* For example, changing a brush property isn't stored by sculpt-mode undo steps.
* This workaround is needed until the limitation is removed, see: #61948.
*/
bool ED_undo_is_legacy_compatible_for_property(bContext *C, ID *id);
bool ED_undo_is_legacy_compatible_for_property(const bContext *C,
const ID *id,
const PropertyRNA *prop);
/**
* This function addresses the problem of restoring undo steps when multiple windows are used.

View File

@ -958,8 +958,8 @@ static void ui_apply_but_undo(uiBut *but)
}
else {
ID *id = but->rnapoin.owner_id;
if (!ED_undo_is_legacy_compatible_for_property(static_cast<bContext *>(but->block->evil_C),
id))
if (!ED_undo_is_legacy_compatible_for_property(
static_cast<bContext *>(but->block->evil_C), id, but->rnaprop))
{
skip_undo = true;
}
@ -969,10 +969,20 @@ static void ui_apply_but_undo(uiBut *but)
if (skip_undo == false) {
/* XXX: disable all undo pushes from UI changes from sculpt mode as they cause memfile undo
* steps to be written which cause lag: #71434. */
if (BKE_paintmode_get_active_from_context(static_cast<bContext *>(but->block->evil_C)) ==
PaintMode::Sculpt)
{
skip_undo = true;
bContext *C = static_cast<bContext *>(but->block->evil_C);
PaintMode paint_mode = BKE_paintmode_get_active_from_context(C);
/* Paint modes that are based on the sculpt engine. */
if (ELEM(paint_mode, PaintMode::Sculpt, PaintMode::Vertex)) {
/* If the last step already was a memfile save, still push for changes
* to sculptable datablock properties to avoid over-undo. */
ID *id = but->rnapoin.owner_id;
UndoStep *undo_step = CTX_wm_manager(C)->undo_stack->step_active;
if (!id || !undo_step || undo_step->type != BKE_UNDOSYS_TYPE_MEMFILE ||
!ELEM(GS(id->name), ID_ME, ID_OB))
{
skip_undo = true;
}
}
}

View File

@ -836,6 +836,18 @@ static int vpaint_mode_toggle_exec(bContext *C, wmOperator *op)
}
ED_object_vpaintmode_enter_ex(bmain, depsgraph, scene, ob);
BKE_paint_toolslots_brush_validate(bmain, &ts->vpaint->paint);
/* Recheck the mode after entering it, and create a sculpt style undo step,

This comparison is already stored in is_mode_set. The else clause that contains this code is only run when !is_mode_set, so things get confusing. Add a comment that states that vpaintmode was just entered, and thus the object flags have to be checked again.


(here are just some ponderings that I wanted to share; the actual feedback on this change is above this line)

I actually think that this doubly checking and deeper nesting of the code makes things too complex. For this PR I understand you want to make a minimal change, but the resulting code IMO stretches things a bit too far. BUT, looking at the current code outside of the part that this PR touched, it was IMO already too complex, so let's not bother this PR with that.

Off-topic, this is what I'd do:

  • Move the call to mesh = BKE_mesh_from_object() down to where the mesh is actually used.
  • Replace the use of (Mesh *)ob->data with mesh so that there is one consistent way to refer to the mesh.
  • Merge the if (!is_mode_set) and if (is_mode_set) conditionals, so that there is just one "enter" and one "exit" code path.
This comparison is already stored in `is_mode_set`. The `else` clause that contains this code is only run when `!is_mode_set`, so things get confusing. Add a comment that states that vpaintmode was just entered, and thus the object flags have to be checked again. ----------- (here are just some ponderings that I wanted to share; the actual feedback on this change is above this line) I actually think that this doubly checking and deeper nesting of the code makes things too complex. For this PR I understand you want to make a minimal change, but the resulting code IMO stretches things a bit too far. BUT, looking at the current code outside of the part that this PR touched, it was IMO already too complex, so let's not bother this PR with that. Off-topic, this is what I'd do: - Move the call to `mesh = BKE_mesh_from_object()` down to where the mesh is actually used. - Replace the use of `(Mesh *)ob->data` with `mesh` so that there is one consistent way to refer to the mesh. - Merge the `if (!is_mode_set)` and `if (is_mode_set)` conditionals, so that there is just one "enter" and one "exit" code path.
* same as in sculpt_mode_toggle_exec for Sculpt Mode. */
if (ob->mode & mode_flag) {

This mentions an issue in the old Phabricator notation, so better to change that to #71564. This seems to have been copied from source/blender/editors/sculpt_paint/sculpt_ops.cc, which was already changed to this notation.

This mentions an issue in the old Phabricator notation, so better to change that to `#71564`. This seems to have been copied from `source/blender/editors/sculpt_paint/sculpt_ops.cc`, which was already changed to this notation.
/* Without this the memfile undo step is used,
* while it works it causes lag when undoing the first undo step, see #71564. */
wmWindowManager *wm = CTX_wm_manager(C);
if (wm->op_undo_depth <= 1) {
undo::push_begin(ob, op);
undo::push_end(ob);
}
}
}
BKE_mesh_batch_cache_dirty_tag((Mesh *)ob->data, BKE_MESH_BATCH_DIRTY_ALL);

View File

@ -1826,9 +1826,6 @@ static void sculpt_undosys_step_decode_undo(bContext *C,
sculpt_undosys_step_decode_undo_impl(C, depsgraph, us_iter);
if (us_iter == us) {
if (us_iter->step.prev && us_iter->step.prev->type == BKE_UNDOSYS_TYPE_SCULPT) {
set_active_layer(C, &((SculptUndoStep *)us_iter->step.prev)->active_color_end);
}
break;
}

View File

@ -11,6 +11,9 @@ set(INC
../../bmesh
../../makesrna
../../windowmanager
# RNA_prototypes.h
${CMAKE_BINARY_DIR}/source/blender/makesrna
)
set(INC_SYS

View File

@ -50,6 +50,7 @@
#include "RNA_access.hh"
#include "RNA_define.hh"
#include "RNA_enum_types.hh"
#include "RNA_prototypes.h"
#include "UI_interface.hh"
#include "UI_resources.hh"
@ -447,7 +448,30 @@ bool ED_undo_is_memfile_compatible(const bContext *C)
return true;
}
bool ED_undo_is_legacy_compatible_for_property(bContext *C, ID *id)
static bool ed_undo_skip_in_paint_mode(const ID *id, const PropertyRNA *prop)
{
if (id == nullptr) {
return true;
}
/* Don't create undo steps for brush and tool settings. */
switch (GS(id->name)) {
case ID_BR:
return true;
case ID_SCE:
if (prop == nullptr) {
return true;
}
/* Retain undo steps for outliner hide toggles, which also belong to Scene. */
return BLI_findindex(RNA_struct_type_properties(&RNA_ObjectBase), prop) < 0 &&
BLI_findindex(RNA_struct_type_properties(&RNA_LayerCollection), prop) < 0;
default:
return false;
}
}
bool ED_undo_is_legacy_compatible_for_property(const bContext *C,
const ID *id,
const PropertyRNA *prop)
{
const Scene *scene = CTX_data_scene(C);
ViewLayer *view_layer = CTX_data_view_layer(C);
@ -456,10 +480,15 @@ bool ED_undo_is_legacy_compatible_for_property(bContext *C, ID *id)
Object *obact = BKE_view_layer_active_object_get(view_layer);
if (obact != nullptr) {
if (obact->mode & OB_MODE_ALL_PAINT) {
/* Don't store property changes when painting
* (only do undo pushes on brush strokes which each paint operator handles on its own). */
CLOG_INFO(&LOG, 1, "skipping undo for paint-mode");
return false;
/* Don't store tool property changes when painting.
*
* In Sculpt and Vertex Paint, most other property change undo pushes will be also
* suppressed by another check later in ui_apply_but_undo, unless preceeded by

👍 thanks for leaving this as a comment, it helps so much to understand this interdependency with other behaviours.

I wouldn't mind having a mention of the location in the code where this other check can be found. The downside would be that it's a non-code reference to code that could get out of date at some point, so use your own instinct to decide whether to add this or not.

:+1: thanks for leaving this as a comment, it helps so much to understand this interdependency with other behaviours. I wouldn't mind having a mention of the location in the code where this other check can be found. The downside would be that it's a non-code reference to code that could get out of date at some point, so use your own instinct to decide whether to add this or not.
* a memfile push; however these properties should always be blocked. */
if (ed_undo_skip_in_paint_mode(id, prop)) {
CLOG_INFO(&LOG, 1, "skipping undo for paint-mode");
return false;
}
}
if (obact->mode & OB_MODE_EDIT) {
if ((id == nullptr) || (obact->data == nullptr) ||

View File

@ -3358,7 +3358,7 @@ static int radial_control_modal(bContext *C, wmOperator *op, const wmEvent *even
wmWindowManager *wm = CTX_wm_manager(C);
if (wm->op_undo_depth == 0) {
ID *id = rc->ptr.owner_id;
if (ED_undo_is_legacy_compatible_for_property(C, id)) {
if (ED_undo_is_legacy_compatible_for_property(C, id, rc->prop)) {
ED_undo_push(C, op->type->name);
}
}