Undo: Always fallback to memfile undo #110022

Falk David · 2023-07-12T19:24:31+02:00

Falk David commented

2023-07-12 19:24:31 +02:00

Previously, memfile undo would not work for objects in edit mode.

But we should allow to fallback to memfile for e.g. objects like
point clouds, curves, or grease pencil 3, where no special edit mode
undo system is needed because they do not have any runtime edit data.

The memfile undo system poll function only runs after all the other
undo systems have been polled. Any object that uses a special undo
system for a particular mode already implements this undo system.

Previously, memfile undo would not work for objects in edit mode. But we should allow to fallback to memfile for e.g. objects like point clouds, curves, or grease pencil 3, where no special edit mode undo system is needed because they do not have any runtime edit data. The memfile undo system poll function only runs after all the other undo systems have been polled. Any object that uses a special undo system for a particular mode already implements this undo system.

Falk David requested review from Campbell Barton 2023-07-12 19:28:45 +02:00

Falk David requested review from Bastien Montagne 2023-07-12 19:28:45 +02:00

Hans Goudey commented

2023-07-12 19:41:52 +02:00

Does this make the curves undo system ED_curves_undosys_type unnecessary?

Does this make the curves undo system `ED_curves_undosys_type` unnecessary?

Falk David commented

2023-07-13 07:50:22 +02:00

@HooglyBoogly In theory, yes. In practice, it might be a lot slower in some cases. But it's not like the curves undo system is really optimized for anything.

Falk David force-pushed undo-memfile-default-fallback-all-modes from 6fa9b8d109 to 70cc6f1520

2023-07-14 12:25:14 +02:00

Compare

Bastien Montagne commented

2023-07-17 11:49:38 +02:00

Am not sure about these changes, but this is also not really my area of expertise. Would rather let @ideasman42 check on this first.

Falk David added 1 commit 2023-10-13 13:52:14 +02:00

b5590aaa7a Merge branch 'main' into undo-memfile-default-fallback-all-modes

Falk David added 2 commits 2023-10-13 14:39:19 +02:00

20e4a0e495 Remove comment

3a6692ba7c Don't change object mode when reading object data

Campbell Barton referenced this pull request

2023-10-26 08:36:35 +02:00

Tests: support running graphical tests as part of CTests #114164

Campbell Barton requested changes 2023-10-27 23:18:57 +02:00

Campbell Barton left a comment

To check if this patch causes problems I updated the undo tests which weren't working after recent menu search changes.

While all tests passed, I was testing this patch and managed to get a crash which I've now added a test for.

The test can be run with the following command:

python3 ../lib/tests/ui_simulate/run.py \
  --blender='./bin/blender --factory-startup -w -d --log "ed.undo.*,wm.*" --log-level 1 --log-show-basename' \
  --tests "test_undo.view3d_edit_mode_multi_window"

A while ago I looked into loading memfile undo steps in edit-mode and recall it was overly complicated to properly support (although it would be good to support, since it would allow for changing scene settings for e.g. to be tracked while in edit-mode).

Either this crash needs to be investigated, or we could differentiate between object types that support MEMFILE undo or not.

To run all undo tests replace --tests "test_undo.view3d_edit_mode_multi_window" with --tests "*".
There's a PR to make these tests part of CTest #114164.

To check if this patch causes problems I updated the undo tests which weren't working after recent menu search changes. While all tests passed, I was testing this patch and managed to get a crash which I've now added a test for. The test can be run with the following command: ``` python3 ../lib/tests/ui_simulate/run.py \ --blender='./bin/blender --factory-startup -w -d --log "ed.undo.*,wm.*" --log-level 1 --log-show-basename' \ --tests "test_undo.view3d_edit_mode_multi_window" ``` A while ago I looked into loading memfile undo steps in edit-mode and recall it was overly complicated to properly support (although it would be good to support, since it would allow for changing scene settings for e.g. to be tracked while in edit-mode). Either this crash needs to be investigated, or we could differentiate between object types that support MEMFILE undo or not. ---- To run all undo tests replace `--tests "test_undo.view3d_edit_mode_multi_window"` with `--tests "*"`. There's a PR to make these tests part of CTest #114164.

Falk David commented

2023-10-30 10:59:17 +01:00

Thanks for fixing the tests and running them on the patch.

Either this crash needs to be investigated, or we could differentiate between object types that support MEMFILE undo or not.

I'm not sure I follow here. Don't we already do this? If we look at ED_undosys_type_init and BKE_undosys_type_from_context we can see that the memfile undo system is pushed last so that it acts like the fallback. Any object type that doesn't support memfile will have pushed it's own undo system before that.

Thanks for fixing the tests and running them on the patch. > Either this crash needs to be investigated, or we could differentiate between object types that support MEMFILE undo or not. I'm not sure I follow here. Don't we already do this? If we look at `ED_undosys_type_init` and `BKE_undosys_type_from_context` we can see that the memfile undo system is pushed last so that it acts like the fallback. Any object type that doesn't support memfile will have pushed it's own undo system before that.

Campbell Barton commented

2023-10-30 11:02:08 +01:00

From the main PR text:

But we should allow to fallback to memfile for e.g. objects like
point clouds, curves, or grease pencil 3, where no special edit mode
undo system is needed because they do not have any runtime edit data.

So this patch could be modified only to change behavior for those object types and keep the current behavior for edit-mesh, edit-text ... etc.

From the main PR text: ``` But we should allow to fallback to memfile for e.g. objects like point clouds, curves, or grease pencil 3, where no special edit mode undo system is needed because they do not have any runtime edit data. ``` So this patch could be modified only to change behavior for those object types and keep the current behavior for edit-mesh, edit-text ... etc.

Falk David commented

2023-10-30 11:10:28 +01:00

Hm well the idea for this patch is that we wouldn't have to do this anymore. Basically, any object type that needs a special undo system can define their own, but any new object type that doesn't need a special undo system shouldn't have to define their own if it's not needed and just use memfile by default.

I'll try and see what the issue is with the test and try and fix it.

Hm well the idea for this patch is that we wouldn't have to do this anymore. Basically, any object type that needs a special undo system can define their own, but any new object type that doesn't need a special undo system shouldn't have to define their own if it's not needed and just use memfile by default. I'll try and see what the issue is with the test and try and fix it.

👍 1

Falk David commented

2023-10-30 12:57:36 +01:00

@ideasman42 This patch below fixes the crash for me. Maybe this just happend to work before because of some assumptions made?

diff --git a/source/blender/editors/space_view3d/view3d_header.cc b/source/blender/editors/space_view3d/view3d_header.cc
index 5acc1e205d9..ca2cd82d285 100644
--- a/source/blender/editors/space_view3d/view3d_header.cc
+++ b/source/blender/editors/space_view3d/view3d_header.cc
@@ -87,6 +87,9 @@ void uiTemplateEditModeSelection(uiLayout *layout, bContext *C)
   }
 
   BMEditMesh *em = BKE_editmesh_from_object(obedit);
+  if (!em) {
+    return;
+  }
   uiLayout *row = uiLayoutRow(layout, true);
 
   PointerRNA op_ptr;

@ideasman42 This patch below fixes the crash for me. Maybe this just happend to work before because of some assumptions made? ```diff diff --git a/source/blender/editors/space_view3d/view3d_header.cc b/source/blender/editors/space_view3d/view3d_header.cc index 5acc1e205d9..ca2cd82d285 100644 --- a/source/blender/editors/space_view3d/view3d_header.cc +++ b/source/blender/editors/space_view3d/view3d_header.cc @@ -87,6 +87,9 @@ void uiTemplateEditModeSelection(uiLayout *layout, bContext *C) } BMEditMesh *em = BKE_editmesh_from_object(obedit); + if (!em) { + return; + } uiLayout *row = uiLayoutRow(layout, true); PointerRNA op_ptr; ```

Campbell Barton commented

2023-10-30 23:22:51 +01:00

@filedescriptor the problem with this change is it means there is the state where the object-data is in edit-mode but there is no run-time edit-mode data.

Even after the fix you mention withing ~2min I was able to cause another crash, just testing undo/redo with multiple windows & different object types.

There was also some new & strange edit-mode glitches (flickering edit-mode with multiple windows), and I ran into cases where objects where shown in edit-mode (in the header) but drawn in object-mode in the 3D viewport.
It's possible to get stuck in edit mode (pressing Tab did nothing).
Ran into an error Error: Unable to execute 'Toggle Edit Mode', error changing modes with armatures.

I'd like to double check none none of these issues exist without this patch & improve on existing tests, I can't be sure as I'm just testing various actions after applying the patch, nevertheless I'm quite sure some of them are introduced by this patch (issues caused by missing run-time data at least).

If we add null check results the crash, the result will still be invalid/buggy from a user perspective - further it moves the problem to become the responsibility of each operator/add-on. After this PR, it's not enough to check if the object is in edit-mode, it must also be checked for the mode's run-time data exists. These kinds of states are worth avoiding where possible as they're often overlooked during development and can easily cause crashes or strange bugs that make blender feel glitchy/broken from a user perspective - even if the worst crashes are avoided.

It should be possible to gain most of the benefits of this patch while ensuring we never have objects in edit-mode without their run-time edit-mode data.

@filedescriptor the problem with this change is it means there is the state where the object-data is in edit-mode but there is no run-time edit-mode data. Even after the fix you mention withing ~2min I was able to cause another crash, just testing undo/redo with multiple windows & different object types. - There was also some new & strange edit-mode glitches (flickering edit-mode with multiple windows), and I ran into cases where objects where shown in edit-mode (in the header) but drawn in object-mode in the 3D viewport. - It's possible to get stuck in edit mode (pressing Tab did nothing). - Ran into an error `Error: Unable to execute 'Toggle Edit Mode', error changing modes` with armatures. I'd like to double check none none of these issues exist without this patch & improve on existing tests, I can't be sure as I'm just testing various actions after applying the patch, nevertheless I'm quite sure some of them are introduced by this patch (issues caused by missing run-time data at least). --- If we add null check results the crash, the result will still be invalid/buggy from a user perspective - further it moves the problem to become the responsibility of each operator/add-on. After this PR, it's not enough to check if the object is in edit-mode, it must also be checked for the mode's run-time data exists. These kinds of states are worth avoiding where possible as they're often overlooked during development and can easily cause crashes or strange bugs that make blender feel glitchy/broken from a user perspective - even if the worst crashes are avoided. It should be possible to gain most of the benefits of this patch while ensuring we never have objects in edit-mode without their run-time edit-mode data.

Falk David added 3 commits 2023-11-08 15:13:35 +01:00

29fcaf2f36 Merge branch 'main' into undo-memfile-default-fallback-all-modes

6b22357773 Fix crash

3e7a61534b Merge branch 'main' into undo-memfile-default-fallback-all-modes

Falk David added 1 commit 2023-11-08 15:15:10 +01:00

62aaf0bdd0 Revert change

Bastien Montagne requested changes 2024-02-12 10:38:13 +01:00

Bastien Montagne left a comment

@filedescriptor not sure if you want to keep working on this one?

Falk David commented

2024-02-12 10:48:34 +01:00

I didn't have the time to come back to this one. I think it would be good to look into this in the future though.

Falk David added 1 commit 2024-02-27 13:55:14 +01:00

57a99be7ee Merge branch 'main' into undo-memfile-default-fallback-all-modes

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u undo-memfile-default-fallback-all-modes:filedescriptor-undo-memfile-default-fallback-all-modes

git checkout filedescriptor-undo-memfile-default-fallback-all-modes

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Undo: Always fallback to memfile undo #110022

Checkout