Fix #119384: Outliner mode column crash with shared object data #119745

Hans Goudey · 2024-03-21T14:47:11+01:00

Hans Goudey commented

2024-03-21 14:47:11 +01:00

The outliner is meant to check whether each object's data is currently
in the active mode when choosing whether to use "extend" behavior for
the mode switching column. That check worked for when the object data
is shared with the active object, but not any other object currently
in the active mode. The incorrect check caused the "extend" behavior
to turn off, making it exit edit mode for the object data without
handing the other objects using the data.

The simplest way of checking whether an object's data is in the active
mode is putting all the object data in a Set. Storing the set once
when building the outliner view context also avoids quadratic cost,
since the state needs to be checked for every object.

This does raise some questions about whether the mode is a property of
the object or the object data in practice. Also, I find the way the
outliner's callback is always "toggle" rather than "enter" or "exit" is
a bit weak. And the fact that the mode switching API allows this is
unfortunate as well. But this is the most clearly correct non-invasive
change to avoid the crash.

The outliner is meant to check whether each object's data is currently in the active mode when choosing whether to use "extend" behavior for the mode switching column. That check worked for when the object data is shared with the active object, but not any other object currently in the active mode. The incorrect check caused the "extend" behavior to turn off, making it exit edit mode for the object data without handing the other objects using the data. The simplest way of checking whether an object's data is in the active mode is putting all the object data in a Set. Storing the set once when building the outliner view context also avoids quadratic cost, since the state needs to be checked for every object. This does raise some questions about whether the mode is a property of the object or the object data in practice. Also, I find the way the outliner's callback is always "toggle" rather than "enter" or "exit" is a bit weak. And the fact that the mode switching API allows this is unfortunate as well. But this is the most clearly correct non-invasive change to avoid the crash.

Hans Goudey added 1 commit 2024-03-21 14:47:19 +01:00

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

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

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

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

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

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

a91b5ab8ad Fix #119384 : Outliner mode column crash with shared object data

The outliner is meant to check whether each object's data is currently
in the active mode when choosing whether to use "extend" behavior for
the mode switching column. That check worked for when the object data
is shared with the active object, but not any other object currently
in the active mode. The incorrect check caused the "extend" behavior
to turn off, making it exit edit mode for the object data without
handing the other objects using the data.

The simplest way of checking whether an object's data is in the active
mode is putting all the object data in a Set. Storing the set once
when building the outliner view context also avoids quadratic cost,
since the state needs to be checked for every object.

This does raise some questions about whether the mode is a property of
the object or the object data in practice. Also, I find the way the
outliner's callback is always "toggle" rather than "enter" or "exit" is
a bit weak. And the fact that the mode switching API allows this is
unfortunate as well. But this is the most clearly correct non-invasive
change to avoid the crash.

Hans Goudey requested review from Philipp Oeser 2024-03-21 14:47:39 +01:00

Hans Goudey requested review from Julian Eisel 2024-03-21 14:47:39 +01:00

Hans Goudey added this to the 4.1 milestone 2024-03-21 14:47:45 +01:00

Hans Goudey added this to the User Interface project 2024-03-21 14:47:51 +01:00

Hans Goudey commented

2024-03-21 14:47:57 +01:00

@blender-bot build

Hans Goudey referenced this pull request

2024-03-21 14:49:16 +01:00

Fix #119384: Outliner Edit Mode Toggle When Data is Shared #119704

Philipp Oeser commented

2024-03-21 15:26:41 +01:00

Seems fine functionality wise.

I was a bit confused why it uses BKE_view_layer_array_from_objects_in_mode_unique_data (instead of something like BKE_view_layer_array_from_objects_in_mode_params), since unique data is actually not what we want here?

OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector (but just adjusts what is tagged LIB_TAG_DOIT -- and we are not relying on that here...). My confusion might not be justified, this is just from first glance.

Seems fine functionality wise. I was a bit confused why it uses `BKE_view_layer_array_from_objects_in_mode_unique_data` (instead of something like `BKE_view_layer_array_from_objects_in_mode_params`), since **unique data** is actually not what we want here? OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector (but just adjusts what is tagged `LIB_TAG_DOIT` -- and we are not relying on that here...). My confusion might not be justified, this is just from first glance.

Hans Goudey commented

2024-03-21 15:38:54 +01:00

since unique data is actually not what we want here?

Are you sure about that? I am putting the object data IDs in a Set anyway, which means they will be unique in the end.

OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector

I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender.

>since unique data is actually not what we want here? Are you sure about that? I am putting the object data IDs in a `Set` anyway, which means they will be unique in the end. >OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender.

Philipp Oeser commented

2024-03-21 15:52:14 +01:00

since unique data is actually not what we want here?

Are you sure about that? I am putting the object data IDs in a Set anyway, which means they will be unique in the end.

Oops, OK the set contains object data, not objects, should be fine then, sorry for the noise

OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector

I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender.

Need to double-check

> >since unique data is actually not what we want here? > > Are you sure about that? I am putting the object data IDs in a `Set` anyway, which means they will be unique in the end. Oops, OK the set contains object **data**, not **objects**, should be fine then, sorry for the noise > >OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector > > I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender. Need to double-check

Philipp Oeser commented

2024-03-21 16:17:22 +01:00

OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector

I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender.

Yeah, only one object per object data. Probably cant read code well enough, might be missing something in the (iter) macros.
Would have been surprising indeed.

So only noise from my side so far (sorry for this), except I can say for this PR it is working as expected :) So +1 from me

> >OTOH, that function does not seem to actually exclude objects sharing the same data from the returned Vector > > I was pretty sure if only have a single object with each object data ID. If it's not working that way, I'm fairly sure it would cause bugs with other areas in Blender. Yeah, only one object per object data. Probably cant read code well enough, might be missing something in the (iter) macros. Would have been surprising indeed. So only noise from my side so far (sorry for this), except I can say for this PR it is working as expected :) So +1 from me

Philipp Oeser approved these changes 2024-03-21 16:18:01 +01:00

Harley Acheson commented

2024-03-21 16:31:41 +01:00

First, I should say this is a way better approach than mine.

But seeing some change in behavior in normal cases.

Starting with default scene, add a monkey, so no sharing of data. Put monkey into edit mode. The outliner now properly shows the monkey with the "edit" icon and Cube shows a dot. Ctrl-click on cube's dot and they are both in the edit set and both properly show the edit icon. But ctrl-click again on cube and IT will then be only thing in edit mode (and is now the active object).

Actually with multiple objects in edit mode you can't take any single item out of the edit set.

First, I should say this is a way better approach than mine. But seeing some change in behavior in normal cases. Starting with default scene, add a monkey, so no sharing of data. Put monkey into edit mode. The outliner now properly shows the monkey with the "edit" icon and Cube shows a dot. Ctrl-click on cube's dot and they are both in the edit set and both properly show the edit icon. But ctrl-click again on cube and IT will then be only thing in edit mode (and is now the active object). Actually with multiple objects in edit mode you can't take any single item out of the edit set.

Hans Goudey added 2 commits 2024-03-21 18:36:31 +01:00

e5e00f68ee Merge branch 'blender-v4.1-release' into fix-outliner-mode-switch-crash

fd9a7c6317 Fix crash with no active object

Hans Goudey commented

2024-03-21 18:56:01 +01:00

Thanks for the testing. Good point about the changed behavior. I can't think of a good way to fix it without splitting up the "toggle" into "enter" and "exit" each with separate "extend" behavior that properly handles shared object data.

I'm still inclined to go with this PR as-is for 4.1 so we can get the crash out of the way. The "reverse extend" behavior with the mode icon isn't documented in the tooltip, while it is for the dot.

If that sounds okay to you, we could move forward with this and I can refactor this area and fix the change in behavior for main.

Thanks for the testing. Good point about the changed behavior. I can't think of a good way to fix it without splitting up the "toggle" into "enter" and "exit" each with separate "extend" behavior that properly handles shared object data. I'm still inclined to go with this PR as-is for 4.1 so we can get the crash out of the way. The "reverse extend" behavior with the mode icon isn't documented in the tooltip, while it is for the dot. If that sounds okay to you, we could move forward with this and I can refactor this area and fix the change in behavior for `main`.

Harley Acheson commented

2024-03-21 19:18:52 +01:00

Digging deeper, this is what I see now...

We load that sample file, and see that "Suzanne.001" has "dot" icon but it is sharing a mesh with "Suzanne" which is shown as already in edit mode.

If we expand both objects to show their meshes, we see that the display of the meshes is correct, both showing as being in edit mode. It is really just a very simple bug that "Suzanne.01" is showing the wrong icon. Let's imagine simply changing that icon so it shows the edit icon.

Ctrl-click on "Suzanne.001"'s icon and it does the correct thing, which is take it OUT of edit mode. do_outliner_item_editmode_toggle works as it is supposed to, and only crashes when we get to ED_undo_push. But that crash is not related to "Suzanne.001" but to ""Suzanne".

What happens is we are changing the edit mode of "Suzanne.001", and afterward that object is no longer in mode OB_MODE_EDIT and it's data->edit_mesh becomes null.

But "Suzanne" is still in OB_MODE_EDIT, but its edit_mesh is now null. Functions like ED_undo_editmode_objects_from_view_layer just check that mode. So in ED_undo_push it ends up with a list containing an object that should be editable but has no mesh.

I can make this work as we expect if in do_outliner_item_editmode_toggle and exiting edit mode we go through all the objects, and for any that have the same data we remove OB_MODE_EDIT. And add that if we are entering edit mode.

Digging deeper, this is what I see now... We load that sample file, and see that "Suzanne.001" has "dot" icon but it is sharing a mesh with "Suzanne" which is shown as already in edit mode. If we expand both objects to show their meshes, we see that the display of the meshes is correct, both showing as being in edit mode. It is really just a very simple bug that "Suzanne.01" is showing the wrong icon. Let's imagine simply changing that icon so it shows the edit icon. Ctrl-click on "Suzanne.001"'s icon and it does the correct thing, which is take it OUT of edit mode. do_outliner_item_editmode_toggle works as it is supposed to, and only crashes when we get to `ED_undo_push`. But that crash is not related to "Suzanne.001" but to ""Suzanne". What happens is we are changing the edit mode of "Suzanne.001", and afterward that object is no longer in mode OB_MODE_EDIT and it's data->edit_mesh becomes null. But "Suzanne" is still in OB_MODE_EDIT, but its edit_mesh is now null. Functions like ED_undo_editmode_objects_from_view_layer just check that mode. So in ED_undo_push it ends up with a list containing an object that should be editable but has no mesh. I can make this work as we expect if in do_outliner_item_editmode_toggle and exiting edit mode we go through all the objects, and for any that have the same data we remove OB_MODE_EDIT. And add that if we are entering edit mode.

Hans Goudey commented

2024-03-21 20:28:36 +01:00

Looks like #119759 is better than this one.

Hans Goudey closed this pull request

2024-03-21 20:28:38 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

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

Fix #119384: Outliner mode column crash with shared object data #119745

Pull request closed