Fixing undo overreach after RNA property changes done through UI. #104582

New Issue

Closed

opened 2023-02-10 18:30:13 +01:00 by Alexander Gavrilov · 8 comments

Alexander Gavrilov commented

2023-02-10 18:30:13 +01:00

Member

Currently changing RNA properties of undo-able datablocks through the UI rather than operators does not create undo stack entries. As a result, undoing the first operator done after such changes will also revert the values.

This can be extremely annoying in some workflows and render the use of the redo panel impossible without extra steps to push an anchoring undo entry via harmless but unnecessary actions like selection.

Since I have only very vague understanding of the undo system, my only idea is some brute force solution like any RNA property changes to 'undo-able' datablocks done outside an operator setting a global flag, which would cause the next undo-able operator to make an extra undo push at the start of execution.

@brecht @mont29 @BClark

Some major cases of this is changing the active vertex group or shape key and then doing painting or editing actions on it.

Currently changing RNA properties of undo-able datablocks through the UI rather than operators does not create undo stack entries. As a result, undoing the first operator done after such changes will also revert the values. This can be extremely annoying in some workflows and render the use of the redo panel impossible without extra steps to push an anchoring undo entry via harmless but unnecessary actions like selection. Since I have only very vague understanding of the undo system, my only idea is some brute force solution like any RNA property changes to 'undo-able' datablocks done outside an operator setting a global flag, which would cause the next undo-able operator to make an extra undo push at the start of execution. @brecht @mont29 @BClark ------- Some major cases of this is changing the active vertex group or shape key and then doing painting or editing actions on it.

Alexander Gavrilov added the

label 2023-02-10 18:30:13 +01:00

Brecht Van Lommel added the

label 2023-02-10 18:35:15 +01:00

Bastien Montagne commented

2023-02-10 18:41:08 +01:00

Owner

Am not sure I follow you here... In the UI, undoable data changes do generate an undo step (e.g. changing location of an object in the property editor)?

Non-UI RNA editing (e.g. through python script) indeed do not generate undo steps, but this is on purpose, python code can create these undo steps when it needs to.

Am not sure I follow you here... In the UI, undoable data changes do generate an undo step (e.g. changing location of an object in the property editor)? Non-UI RNA editing (e.g. through python script) indeed do not generate undo steps, but this is on purpose, python code can create these undo steps when it needs to.

Bastien Montagne added the

label 2023-02-10 18:41:20 +01:00

Alexander Gavrilov commented

2023-02-10 18:45:39 +01:00

Author

Member

@mont29 Is the active vertex group or shape key 'undoable data'? The most recent case of this that I remember is related to them. I also have vague recollection about the issue happening to maybe brush settings in paint/sculpt mode?..

If they are not 'undoable data', then either they should not be undone, or they should be undoable.

If they are marked 'not undoable' to avoid unnecessary undo pushes, I think my solution could be the correct approach - since in reality the value is undoable, and the goal is to coalesce all changes to such weakly undoable properties into the smallest number of pushes.

@mont29 Is the active vertex group or shape key 'undoable data'? The most recent case of this that I remember is related to them. I also have vague recollection about the issue happening to maybe brush settings in paint/sculpt mode?.. If they are not 'undoable data', then either they should not be undone, or they should be undoable. If they are marked 'not undoable' to avoid unnecessary undo pushes, I think my solution could be the correct approach - since in reality the value is undoable, and the goal is to coalesce all changes to such weakly undoable properties into the smallest number of pushes.

Bastien Montagne commented

2023-02-10 18:47:25 +01:00

Owner

Reacting to your edit: This is totally unrelated issue to RNA, it's due to the fact that Undo systems for non-Object modes are different, and are not always cooperating well with Object mode undo (aka memfile undo). We already have many reports about this, see e.g #97700, #90291, and so on.

Reacting to your edit: This is totally unrelated issue to RNA, it's due to the fact that Undo systems for non-Object modes are different, and are not always cooperating well with Object mode undo (aka memfile undo). We already have many reports about this, see e.g #97700, #90291, and so on.

Bastien Montagne commented

2023-02-10 18:48:44 +01:00

Owner

Think @ideasman42 will be your best source of info on these topics.

Think @ideasman42 will be your best source of info on these topics.

Julian Eisel commented

2023-02-10 19:13:42 +01:00

Member

Code that changes scene data should always do an undo push when done. If the undo doesn't behave as expected, that is bug or limitation. It needs to be investigated what happens in the specific case. I know that there were indeed cases with brush settings getting reset wrongly, but this again is a bug then. Typically they are caused by either forgetting to enable undo or because of a fuzzy distinction between scene (undoable) and UI (not undoable) data. This needs special treatment then.
A hack like the proposed global flag would just hide the symptoms without looking at the root of the issue.

For when many undo pushes could be an issue, there's now "undo groups", so for example changing the active vertex group multiple times could result in a single merged undo push.

Code that changes scene data should always do an undo push when done. If the undo doesn't behave as expected, that is bug or limitation. It needs to be investigated what happens in the specific case. I know that there were indeed cases with brush settings getting reset wrongly, but this again is a bug then. Typically they are caused by either forgetting to enable undo or because of a fuzzy distinction between scene (undoable) and UI (not undoable) data. This needs special treatment then. A hack like the proposed global flag would just hide the symptoms without looking at the root of the issue. For when many undo pushes could be an issue, there's now "undo groups", so for example changing the active vertex group multiple times *could* result in a single merged undo push.

Alexander Gavrilov commented

2023-02-10 19:57:16 +01:00

Author

Member

@JulianEisel Apparently in some cases undo is disabled deliberately?.. #101728 (comment)

@JulianEisel Apparently in some cases undo is disabled deliberately?.. https://projects.blender.org/blender/blender/issues/101728#issuecomment-98614

Alexander Gavrilov commented

2023-02-11 18:50:18 +01:00

Author

Member

So today I tried debugging property undo (specifically targeting various active layer selections), and came up with a bunch of proposed fixes for the paint modes e8805a1a3ff.

In particular, I found that for some reason some specific code by @JosephEagar is directly doing over-undo for the active color layer selection in sculpt undo - I wonder what was the logic for that.

Edit: There also seems to be a major conceptual issue that if a sculpt step follows a memfile step for whatever reason, and a property change is done in between, the interface code won't save a step for the change because it's in sculpt mode, but since the preceeding step is memfile the change will actually be undone. This should either be fixed somehow, or the interface code should push property steps anyway if the current step on the stack is memfile.

So today I tried debugging property undo (specifically targeting various active layer selections), and came up with a bunch of proposed fixes for the paint modes e8805a1a3ff. In particular, I found that for some reason some [specific code](https://projects.blender.org/blender/blender/commit/e8805a1a3ff977018882fc37b083c4d5f1f96a6e#diff-ba6c72638d638cdf50f74fcf4590a2b08b51886c) by @JosephEagar is directly doing over-undo for the active color layer selection in sculpt undo - I wonder what was the logic for that. Edit: There also seems to be a major conceptual issue that if a sculpt step follows a memfile step for whatever reason, and a property change is done in between, the interface code won't save a step for the change because it's in sculpt mode, but since the preceeding step is memfile the change will actually be undone. This should either be fixed somehow, or the interface code should push property steps anyway if the current step on the stack is memfile.

Bastien Montagne referenced this issue from infrastructure/blender-projects-platform

2023-02-13 11:37:45 +01:00

inconsistencies in git-hash-based links between organizations #25

Alexander Gavrilov referenced this issue

2023-02-17 14:20:47 +01:00

Shape Keys: support locking to protect from accidental editing. #104463

Bastien Montagne commented

2023-02-17 18:22:39 +01:00

Owner

We already have many (many) bug reports about that generic issue (transitions between undo steps of different types), and this is not a design task at all. So will archive it for now.

Note that a proper design task regarding how to fix this issue, and how to improve readability and maintainability of the undo system would be much welcome. Would suggest you talk with the current owner of this area first, though (@ideasman42).

We already have many (many) bug reports about that generic issue (transitions between undo steps of different types), and this is not a design task at all. So will archive it for now. Note that a proper design task regarding how to fix this issue, and how to improve readability and maintainability of the undo system would be much welcome. Would suggest you talk with the current owner of this area first, though (@ideasman42).

Bastien Montagne closed this issue

2023-02-17 18:22:40 +01:00

Blender Bot added the

label 2023-02-17 18:22:41 +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#104582

No description provided.