Fix #116734: Measure Tool Undo crash. #116751

Bastien Montagne · 2024-01-03T17:25:12+01:00

Bastien Montagne commented

2024-01-03 17:25:12 +01:00

From my understanding, existing ruler code would add new (GP) ID and
link it to current scene outside of any depsgraph tagging and/or undo
step writing?

From my understanding, existing ruler code would add new (GP) ID and link it to current scene outside of any depsgraph tagging and/or undo step writing?

👍 1

Bastien Montagne added 1 commit 2024-01-03 17:25:21 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

303b912960 Fix #116734 : Measure Tool Undo crash.

From my understanding, existing ruler code would add new (GP) ID and
link it to current scene outside of any depsgraph tagging and/or undo
step writing?

Bastien Montagne commented

2024-01-03 17:25:38 +01:00

@blender-bot build

Bastien Montagne referenced this pull request

2024-01-03 17:26:04 +01:00

Measure Tool Undo crash #116734

Germano Cavalcante commented

2024-01-04 15:09:42 +01:00

It appears that tweak the ruler ("GIZMOGROUP_OT_gizmo_tweak") is not expected to write Undo step. So just making sure Layer GP was added with "VIEW3D_GT_ruler_item" seems good enough imho.

Campbell Barton approved these changes 2024-01-11 11:48:17 +01:00

Campbell Barton left a comment

This works well, previously the new data-block wasn't account for by global-undo.

With the steps from #116734:

Before (also with Blender 3.6x which didn't crash):

Undo 3 Steps (*: active, #=applied, M=memfile-active, S=skip)
[  M ]   0 {0x60f000025ff8} type='Global Undo', name='Original'
[*   ]   1 {0x612000254bf8} type='Edit Mesh', name='Toggle Edit Mode'
[    ]   2 {0x612000447c78} type='Edit Mesh', name='Ruler Add'

With this PR applied:

Undo 4 Steps (*: active, #=applied, M=memfile-active, S=skip)
[*#M ]   0 {0x60f000179b98} type='Global Undo', name='Original'
[    ]   1 {0x6120007dadf8} type='Edit Mesh', name='Toggle Edit Mode'
[   S]   2 {0x60f00009b208} type='Global Undo', name='MemFile Internal (pre)'
[    ]   3 {0x612000647ef8} type='Edit Mesh', name='Ruler Add'

So this fix looks correct to me.

There is still the problem with measure tool undo steps not working in edit-mode but that's a more general issue.

This works well, previously the new data-block wasn't account for by global-undo. With the steps from #116734: Before (also with Blender 3.6x which didn't crash): ``` Undo 3 Steps (*: active, #=applied, M=memfile-active, S=skip) [ M ] 0 {0x60f000025ff8} type='Global Undo', name='Original' [* ] 1 {0x612000254bf8} type='Edit Mesh', name='Toggle Edit Mode' [ ] 2 {0x612000447c78} type='Edit Mesh', name='Ruler Add' ``` With this PR applied: ``` Undo 4 Steps (*: active, #=applied, M=memfile-active, S=skip) [*#M ] 0 {0x60f000179b98} type='Global Undo', name='Original' [ ] 1 {0x6120007dadf8} type='Edit Mesh', name='Toggle Edit Mode' [ S] 2 {0x60f00009b208} type='Global Undo', name='MemFile Internal (pre)' [ ] 3 {0x612000647ef8} type='Edit Mesh', name='Ruler Add' ``` So this fix looks correct to me. There is still the problem with measure tool undo steps not working in edit-mode but that's a more general issue.

source/blender/editors/space_view3d/view3d_gizmo_ruler.cc Outdated

						
				@ -517,2 +517,3 @@

				#define RULER_ID "RulerData3D"

				static bool view3d_ruler_to_gpencil(bContext *C, wmGizmoGroup *gzgroup)

				/* GP data creation has to happen before the undo step is stored. */

Campbell Barton commented

2024-01-11 11:47:59 +01:00

Worth referencing #116734.

mont29 marked this conversation as resolved

source/blender/editors/space_view3d/view3d_gizmo_ruler.cc

						
				@ -522,1 +524,4 @@

				  Scene *scene = CTX_data_scene(C);

				  if (scene->gpd == nullptr) {

				    scene->gpd = BKE_gpencil_data_addnew(bmain, "Annotations");

				    DEG_id_tag_update_ex(bmain, &scene->id, ID_RECALC_COPY_ON_WRITE);

Campbell Barton commented

2024-01-11 11:34:43 +01:00

Note why this is needed.

Bastien Montagne commented

2024-01-11 12:54:15 +01:00

Not sure what you mean? We are creating a new ID, and linking it to another ID, so we need to tag the modified ID for update... This is done in thousands of places all over our codebase, I would not expect to have to document this.

Actually, I also forgot to tag for relations update here.

Not sure what you mean? We are creating a new ID, and linking it to another ID, so we need to tag the modified ID for update... This is done in thousands of places all over our codebase, I would not expect to have to document this. Actually, I also forgot to tag for relations update here.

Campbell Barton commented

2024-01-11 22:47:00 +01:00

Ah, no need then, I thought this might relate to undo (as tagging wasn't done before).

👍 1

ideasman42 marked this conversation as resolved

Bastien Montagne added 2 commits 2024-01-11 12:55:05 +01:00

a9b608a089 Merge branch 'main' into tmp-116734

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details