blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 779 Projects 19 Wiki Activity

Fix #119384: Outliner crash extend mode toggle on objects sharing data #119416

Closed
Philipp Oeser wants to merge 3 commits from lichtwerk/blender:119384 into main
pull from: lichtwerk/blender:119384
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 4 Commits 3 Files Changed 1 +30 -3
1 changed files with 30 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
source/blender/editors/space_outliner/outliner_draw.cc
View File

@ -2142,6 +2142,33 @@ static void outliner_buttons(const bContext *C,
}
}
static bool outliner_is_object_data_shared_in_mode(TreeViewContext *tvc,
lichtwerk marked this conversation as resolved Outdated
Julian Eisel commented 2024-03-13 13:47:19 +01:00
Outdated
Review
Copy Link

This name is not clear to me. It reads like it's toggling some shared state of the object-data. How about outliner_is_object_data_shared_in_mode()?

This name is not clear to me. It reads like it's toggling some shared state of the object-data. How about `outliner_is_object_data_shared_in_mode()`?
const Object *ob,
const Object *ob_active)
lichtwerk marked this conversation as resolved Outdated
Julian Eisel commented 2024-03-13 13:55:38 +01:00
Outdated
Review
Copy Link

Both these can be const I think.

Both these can be `const` I think.
{
if (ob->data == ob_active->data) {
lichtwerk marked this conversation as resolved Outdated
Julian Eisel commented 2024-03-13 13:48:50 +01:00
Outdated
Review
Copy Link

This object_data_shared variable seems to just make things more confusing (and less efficient), I don't see a need for it. Just return once you have found an object sharing the data. Like

{
  if (...) {
    return true;
  }

  for (...) {
    if (...) {
      return true;
    }
  }
  return false;
}

This makes the function flow much easier to follow, there's no local "state" variable to keep track of.

This `object_data_shared` variable seems to just make things more confusing (and less efficient), I don't see a need for it. Just return once you have found an object sharing the data. Like ```C { if (...) { return true; } for (...) { if (...) { return true; } } return false; } ``` This makes the function flow much easier to follow, there's no local "state" variable to keep track of.
Hans Goudey commented 2024-03-13 13:57:04 +01:00
Outdated
Review
Copy Link

Looks like this can be changed to if (ob->data == ob_active->data) { return true; }

Looks like this can be changed to `if (ob->data == ob_active->data) { return true; }`
return true;
}
lichtwerk marked this conversation as resolved Outdated
Hans Goudey commented 2024-03-13 13:56:28 +01:00
Outdated
Review
Copy Link

Flip the condition and return early in this case

Flip the condition and return early in this case
if (static_cast<const ID *>(ob->data)->us <= 1) {
return false;
}
Julian Eisel commented 2024-03-13 13:54:26 +01:00
Outdated
Review
Copy Link

I'm not sure if it's a great idea to call this here, since it gets called for every object in every redraw. You could store this in TreeViewContext. What do you think @HooglyBoogly?

Also I guess this will contain both ob and ob_active, so the function will always return true?
I wonder if the original ob->data == tvc.obact->data check also had that issue, I guess it did.

I'm not sure if it's a great idea to call this here, since it gets called for every object in every redraw. You could store this in `TreeViewContext`. What do you think @HooglyBoogly? Also I guess this will contain both `ob` and `ob_active`, so the function will always return true? I wonder if the original `ob->data == tvc.obact->data` check also had that issue, I guess it did.
/* If the ID has multiple users, also check if any object using it is in the same mode as the
lichtwerk marked this conversation as resolved
Julian Eisel commented 2024-03-13 13:55:19 +01:00
Review
Copy Link

It's easy to confuse ob and object. Better name this iter_object or something clearer. Also, const.

It's easy to confuse `ob` and `object`. Better name this `iter_object` or something clearer. Also, `const`.
* active object. */
lichtwerk marked this conversation as resolved
Hans Goudey commented 2024-03-13 13:57:20 +01:00
Review
Copy Link

Return true in this case rather than changing a boolean, then return false at the end.

Return true in this case rather than changing a boolean, then return false at the end.
ObjectsInModeParams params = {0};
params.object_mode = ob_active->mode;
Vector<Object *> objects_in_mode = BKE_view_layer_array_from_objects_in_mode_params(
tvc->scene, tvc->view_layer, nullptr, &params);
for (const Object *object_iter : objects_in_mode) {
if (object_iter->data == ob->data) {
return true;
}
}
return false;
}
static void outliner_mode_toggle_fn(bContext *C, void *tselem_poin, void * /*arg2*/)
{
SpaceOutliner *space_outliner = CTX_wm_space_outliner(C);
@ -2157,8 +2184,8 @@ static void outliner_mode_toggle_fn(bContext *C, void *tselem_poin, void * /*arg
/* Check that the item is actually an object. */
BLI_assert(tselem->id != nullptr && GS(tselem->id->name) == ID_OB);
Object *ob = (Object *)tselem->id;
const bool object_data_shared = (ob->data == tvc.obact->data);
const bool object_data_shared = outliner_is_object_data_shared_in_mode(
&tvc, (Object *)tselem->id, tvc.obact);
wmWindow *win = CTX_wm_window(C);
const bool do_extend = (win->eventstate->modifier & KM_CTRL) && !object_data_shared;
@ -2199,7 +2226,7 @@ static void outliner_draw_mode_column_toggle(uiBlock *block,
draw_active_icon = false;
}
const bool object_data_shared = (ob->data == ob_active->data);
const bool object_data_shared = outliner_is_object_data_shared_in_mode(tvc, ob, ob_active);
draw_active_icon = draw_active_icon || object_data_shared;
int icon;