Fix #89479: Unable to select hierarchies of multiple objects from outliner #104737

Pratik Borhade · 2023-02-14T12:52:11+01:00

Pratik Borhade commented

2023-02-14 12:52:11 +01:00

select hierarchy is treated in similar way as we're selecting single element from outliner.
When object_select_hierarchy_fn is called for the first time, we clear select flag for all elements
in outliner_item_select. This causes issue when select hierarchy is called for multiple elements.
So fix is to not touch select and activate flags when OL_ITEM_RECURSIVE flag is set.

This also fixes the hierarchy selection when "sync select" is OFF.

Old Differential Revision: https://archive.blender.org/developer/D16804

`select hierarchy` is treated in similar way as we're selecting single element from outliner. When `object_select_hierarchy_fn` is called for the first time, we clear select flag for all elements in `outliner_item_select`. This causes issue when `select hierarchy` is called for multiple elements. So fix is to not touch select and activate flags when `OL_ITEM_RECURSIVE` flag is set. This also fixes the hierarchy selection when "sync select" is OFF. Old Differential Revision: https://archive.blender.org/developer/D16804

Pratik Borhade added 1 commit 2023-02-14 12:52:13 +01:00

8c9308f87c Fix #89479 : Unable to select hierarchies of multiple objects from outliner

`select hierarchy` is treated in similar way as we're selecting single element from outliner.
When `object_select_hierarchy_fn` is called for the first time, we clear select flag for all elements
in `outliner_item_select`. This causes issue when `select hierarchy` is called for multiple elements.
So fix is to not touch select and activate flags when `OL_ITEM_RECURSIVE` flag is set.

Old Differential Revision: https://archive.blender.org/developer/D16804

Pratik Borhade requested review from Julian Eisel 2023-02-14 12:52:38 +01:00

Brecht Van Lommel added this to the User Interface project 2023-02-15 09:53:16 +01:00

Brecht Van Lommel requested review from Harley Acheson 2023-03-10 13:22:27 +01:00

Brecht Van Lommel commented

2023-03-10 13:22:31 +01:00

Poke, can we prioritize reviewing bug fixes like this a bit earlier?

Harley Acheson commented

2023-03-10 21:30:54 +01:00

I can definitely recreate and confirm the issue as reported in #89479. However, is this "object.select_hierarchy" (shift-]) operator exposed in a menu somewhere?

I can also confirm that this PR seems to solve this issue.

I can see why the the change to the clear_flag fixes the issue, but not sure that I like that it is doing so because of OL_ITEM_RECURSIVE. Could we not conceivably have a selection type that is recursive but might also need to clear the selected items? At the very least this would need a good comment.

Could we instead get a new TreeItemSelectAction flag for this behavior?

I can't tell what the second change does, the if (!recursive) around the addition of TSE_ACTIVE. It seems to work the same if I comment out that change.

I can definitely recreate and confirm the issue as reported in #89479. However, is this "object.select_hierarchy" (shift-]) operator exposed in a menu somewhere? I can also confirm that this PR seems to solve this issue. I can see why the the change to the clear_flag fixes the issue, but not sure that I like that it is doing so because of OL_ITEM_RECURSIVE. Could we not conceivably have a selection type that is recursive but might also need to clear the selected items? At the very least this would need a good comment. Could we instead get a new TreeItemSelectAction flag for this behavior? I can't tell what the second change does, the `if (!recursive)` around the addition of TSE_ACTIVE. It seems to work the same if I comment out that change.

Pratik Borhade commented

2023-03-13 12:02:53 +01:00

Hi, thanks for reviewing :)

I don't see any particular reason to introduce a new flag here.

I can't tell what the second change does, the if (!recursive) around the addition of TSE_ACTIVE. It seems to work the same if I comment out that change.

This change was done to keep the active tree-element same but looks like its not behaving as expected :/
Will fix that.

Hi, thanks for reviewing :) I don't see any particular reason to introduce a new flag here. > I can't tell what the second change does, the if (!recursive) around the addition of TSE_ACTIVE. It seems to work the same if I comment out that change. This change was done to keep the active tree-element same but looks like its not behaving as expected :/ Will fix that.

Pratik Borhade added 2 commits 2023-03-21 03:12:32 +01:00

bc478ddb9b Merge branch 'main' into T89479-select-multi-hierarchies

623ac68cf0 Avoid changing active tree element

`ED_object_base_activate` is changing the active base `view_layer->basact`.
This is exepcted for left click and ctrl click selection but we don't
expect to change the active element when selecting hierarchies. So to fix
this add extra `if() check`.

Pratik Borhade commented

2023-03-21 03:16:57 +01:00

@Harley hi, updated the PR. Can you check again?

Harley Acheson commented

2023-03-21 18:17:36 +01:00

A little thing: The comment (currently shown at line 1493) would need updating to include the inclusion of your recursive test. Something like?

/* Clear previous active when activating and clear selection when not extended or recursive selection. */

I'm still confused about that bit at line 1510:

if (!recursive) {
      tselem->flag |= TSE_ACTIVE;
}

In my testing it all seems to behave the same way without that change. I test it with a single branch or with multiple branches and I can't find a time when it is harmful to not set that element active. So I am probably missing some testing scenario? I'm sure you made that change for some good purpose, but I'm just not seeing it and would like to.

A little thing: The comment (currently shown at line 1493) would need updating to include the inclusion of your recursive test. Something like? ``` /* Clear previous active when activating and clear selection when not extended or recursive selection. */ ``` I'm still confused about that bit at line 1510: ``` if (!recursive) { tselem->flag |= TSE_ACTIVE; } ``` In my testing it all seems to behave the same way without that change. I test it with a single branch or with multiple branches and I can't find a time when it is harmful to not set that element active. So I am probably missing some testing scenario? I'm sure you made that change for some good purpose, but I'm just not seeing it and would like to.

Pratik Borhade added 1 commit 2023-03-23 02:56:34 +01:00

e0eea437d0 Small tweaks

- Comment added
- Added one if condition to not clear the both active and select flag
  (this was the mistake in earlier code)

Pratik Borhade commented

2023-03-23 03:03:01 +01:00

I'm still confused about that bit at line 1510:

@Harley , this is useful when sync selection option is OFF.
When sync selection is enabled, active element is correctly set from the reference of view_layer->basact in outliner_select_sync_from_object.

But when option is disabled and you comment out the if condition from line 1510, all the tree elements will be set as active.

> I'm still confused about that bit at line 1510: @Harley , this is useful when `sync selection` option is OFF. When `sync selection` is enabled, active element is correctly set from the reference of `view_layer->basact` in `outliner_select_sync_from_object`. But when option is disabled and you comment out the `if` condition from line 1510, all the tree elements will be set as `active`.

Pratik Borhade added 1 commit 2023-03-23 12:05:33 +01:00

79c38ac4a4 Merge branch 'main' into T89479-select-multi-hierarchies

Harley Acheson commented

2023-03-24 00:55:56 +01:00

@PratikPB2123 Sorry to be pain about this

I am testing with the file given for the original complaint: #89479.blend

Without your patch

I can see the complaint. In the 3D View I can select multiple parents, for example the cube and monkey, then press Shift=] and I will have selected not only those but their children too.

But in Outliner I can click a single parent, choose "Select Hierarchy" and it will correctly select the parent and children. But if I select multiple parents and select the same menu item I will only have selected one parent and its children, not multiples. That is the essence of the complaint.

But also note that if I turn off "Sync Selection" I can right-click on any item in Outliner, choose "Select" and it will become selected. And I can right-click on a second item, do the same and both items will then be selected.

But right-click on any item and select "Select Hierarchy" and it does nothing. Just left-click an item and try and it also does nothing. This seems like another bad issue.

With your patch

It is much better. I can select a single parent or multiples, select "Select Hierarchy" and things get selected as I expect.

But you mentioned that the code in question affected the behavior when "Sync Selection" is off. If I turn that off then "Select Hierarchy" still doesn't work. It does not select anything at all. And I don't see any change if I comment out that section of code.

I get much closer to what I want with the following:

diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc
index f273dc5d580..818ba9beb0d 100644
--- a/source/blender/editors/space_outliner/outliner_select.cc
+++ b/source/blender/editors/space_outliner/outliner_select.cc
@@ -357,6 +357,9 @@ static void tree_element_object_activate(bContext *C,
         }
       }
     }
+    else if (recursive) {
+      /* Pass */
+    }
     else {
       /* De-select all. */
 
@@ -382,7 +385,9 @@ static void tree_element_object_activate(bContext *C,
     }
 
     if (set != OL_SETSEL_NONE) {
-      ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      if (!recursive) {
+        ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      }
       DEG_id_tag_update(&scene->id, ID_RECALC_SELECT);
       WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene);
     }
@@ -1482,11 +1487,14 @@ void outliner_item_select(bContext *C,
   TreeStoreElem *tselem = TREESTORE(te);
   const bool activate = select_flag & OL_ITEM_ACTIVATE;
   const bool extend = select_flag & OL_ITEM_EXTEND;
-  const bool activate_data = select_flag & OL_ITEM_SELECT_DATA;
+  const bool recursive = select_flag & OL_ITEM_RECURSIVE;
+  const bool activate_data = select_flag & OL_ITEM_SELECT_DATA || recursive;
 
   /* Clear previous active when activating and clear selection when not extending selection */
   const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED);
-  if (clear_flag) {
+
+  /* Do not clear the active and select flag when selecting hierarchies. */
+  if (clear_flag && !recursive) {
     outliner_flag_set(*space_outliner, clear_flag, false);
   }
 
@@ -1508,7 +1516,7 @@ void outliner_item_select(bContext *C,
                                            te,
                                            tselem,
                                            extend,
-                                           select_flag & OL_ITEM_RECURSIVE,
+                                           recursive,
                                            activate_data || space_outliner->flag & SO_SYNC_SELECT);
   }
 }

It seems to select children of multiple parents correctly whether "sync selection" is on or off.

@PratikPB2123 Sorry to be pain about this I am testing with the file given for the [original complaint](https://projects.blender.org/blender/blender/issues/89479): [#89479.blend](https://archive.blender.org/developer/F14069397/T89479.blend) **Without your patch** I can see the complaint. In the 3D View I can select multiple parents, for example the cube and monkey, then press Shift=] and I will have selected not only those but their children too. But in Outliner I can click a *single* parent, choose "Select Hierarchy" and it will correctly select the parent and children. But if I select **multiple** parents and select the same menu item I will only have selected one parent and its children, not multiples. That is the essence of the complaint. But also note that if I turn off "Sync Selection" I can right-click on any item in Outliner, choose "Select" and it will become selected. And I can right-click on a second item, do the same and both items will then be selected. But right-click on any item and select "Select Hierarchy" and it does nothing. Just left-click an item and try and it also does nothing. This seems like another bad issue. **With your patch** It is much better. I can select a single parent or multiples, select "Select Hierarchy" and things get selected as I expect. But you mentioned that the code in question affected the behavior when "Sync Selection" is off. If I turn that off then "Select Hierarchy" still doesn't work. It does not select anything at all. And I don't see any change if I comment out that section of code. I get much closer to what I want with the following: ```Diff diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc index f273dc5d580..818ba9beb0d 100644 --- a/source/blender/editors/space_outliner/outliner_select.cc +++ b/source/blender/editors/space_outliner/outliner_select.cc @@ -357,6 +357,9 @@ static void tree_element_object_activate(bContext *C, } } } + else if (recursive) { + /* Pass */ + } else { /* De-select all. */ @@ -382,7 +385,9 @@ static void tree_element_object_activate(bContext *C, } if (set != OL_SETSEL_NONE) { - ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + if (!recursive) { + ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + } DEG_id_tag_update(&scene->id, ID_RECALC_SELECT); WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene); } @@ -1482,11 +1487,14 @@ void outliner_item_select(bContext *C, TreeStoreElem *tselem = TREESTORE(te); const bool activate = select_flag & OL_ITEM_ACTIVATE; const bool extend = select_flag & OL_ITEM_EXTEND; - const bool activate_data = select_flag & OL_ITEM_SELECT_DATA; + const bool recursive = select_flag & OL_ITEM_RECURSIVE; + const bool activate_data = select_flag & OL_ITEM_SELECT_DATA || recursive; /* Clear previous active when activating and clear selection when not extending selection */ const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED); - if (clear_flag) { + + /* Do not clear the active and select flag when selecting hierarchies. */ + if (clear_flag && !recursive) { outliner_flag_set(*space_outliner, clear_flag, false); } @@ -1508,7 +1516,7 @@ void outliner_item_select(bContext *C, te, tselem, extend, - select_flag & OL_ITEM_RECURSIVE, + recursive, activate_data || space_outliner->flag & SO_SYNC_SELECT); } } ``` It seems to select children of multiple parents correctly whether "sync selection" is on or off.

Pratik Borhade commented

2023-03-24 14:51:55 +01:00

@Harley, you're right. I did not expand the outliner tree for verifying the selection after the operation, sorry about that. I'll fix it.

BTW, the patch you've uploaded looks incomplete to me.

@Harley, you're right. I did not expand the outliner tree for verifying the selection after the operation, sorry about that. I'll fix it. BTW, the patch you've uploaded looks incomplete to me.

Harley Acheson commented

2023-03-24 15:50:24 +01:00

@PratikPB2123 - BTW, the patch you've uploaded looks incomplete to me.

Oh, sorry about that. I think the only significant change from yours was in setting that "activate_data" bool to select_flag & OL_ITEM_SELECT_DATA || recursive. This way when it is used as part of the "do_activate" it activates with a recursive operation if synch selection is off.

> @PratikPB2123 - BTW, the patch you've uploaded looks incomplete to me. Oh, sorry about that. I think the only significant change from yours was in setting that "activate_data" bool to `select_flag & OL_ITEM_SELECT_DATA || recursive`. This way when it is used as part of the "do_activate" it activates with a recursive operation if synch selection is off.

Pratik Borhade commented

2023-03-27 13:22:58 +02:00

@Harley , this change won't fix the issue. Because in do_outliner_object_select_recursive, we are adding selection flag to base. If we make the above change, objects in viewport are also selected (which is not expected)
When sync selection is off we only want to add selection flag to tree element.
I'll update the patch or share the workaround to fix this problem.

@Harley , this change won't fix the issue. Because in `do_outliner_object_select_recursive`, we are adding selection flag to base. If we make the above change, objects in viewport are also selected (which is not expected) When sync selection is off we only want to add selection flag to tree element. I'll update the patch or share the workaround to fix this problem.

Pratik Borhade added 1 commit 2023-04-01 13:10:30 +02:00

b84e0c72d2 Fix hierarchy selection when sync-selection is disabled

Pratik Borhade added 1 commit 2023-04-01 13:29:29 +02:00

dd840319ce Merge branch 'main' into T89479-select-multi-hierarchies

Pratik Borhade commented

2023-04-05 03:28:24 +02:00

@harley, BTW PR is ready again for the review. Can you check?

Harley Acheson commented

2023-04-05 21:28:15 +02:00

Hey, that seems to work really well!

Following might be picky (or wrong), but you've made a change to do_outliner_object_select_recursive, passing in a subtree and a sync_select bool. When sync_select is false then only the current existing code runs, while if true ONLY your new code runs. And that function is only called twice, once without and once with. Therefore you can pull your new code out of there and move it to that second calling site. Makes less changes and seems to be an easier read. What I mean:

diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc
index f77272f79f5..eb99fa7f8dd 100644
--- a/source/blender/editors/space_outliner/outliner_select.cc
+++ b/source/blender/editors/space_outliner/outliner_select.cc
@@ -355,10 +355,13 @@ static void tree_element_object_activate(bContext *C,
         if (parent_tselem) {
           parent_tselem->flag |= TSE_SELECTED;
         }
       }
     }
+    else if (recursive) {
+      /* Pass */
+    }
     else {
       /* De-select all. */
 
       /* Only in object mode so we can switch the active object,
        * keeping all objects in the current 'mode' selected, useful for multi-pose/edit mode.
@@ -380,11 +383,13 @@ static void tree_element_object_activate(bContext *C,
       do_outliner_object_select_recursive(
           scene, view_layer, ob, (base->flag & BASE_SELECTED) != 0);
     }
 
     if (set != OL_SETSEL_NONE) {
-      ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      if (!recursive) {
+        ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      }
       DEG_id_tag_update(&scene->id, ID_RECALC_SELECT);
       WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene);
     }
   }
 }
@@ -1411,10 +1416,19 @@ static void do_outliner_item_activate_tree_element(bContext *C,
                                  te,
                                  (extend && tselem->type == TSE_SOME_ID) ? OL_SETSEL_EXTEND :
                                                                            OL_SETSEL_NORMAL,
                                  recursive && tselem->type == TSE_SOME_ID);
   }
+  else if (recursive && !(space_outliner->flag & SO_SYNC_SELECT)) {
+    /* Selection of child objects in hierarchy when sync-selection is OFF. */
+    tree_iterator::all(te->subtree, [&](TreeElement *te) {
+      TreeStoreElem *tselem = TREESTORE(te);
+      if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) {
+        tselem->flag |= TSE_SELECTED;
+      }
+    });
+  }
 
   if (tselem->type == TSE_SOME_ID) { /* The lib blocks. */
     if (do_activate_data == false) {
       /* Only select in outliner. */
     }
@@ -1481,14 +1495,17 @@ void outliner_item_select(bContext *C,
 {
   TreeStoreElem *tselem = TREESTORE(te);
   const bool activate = select_flag & OL_ITEM_ACTIVATE;
   const bool extend = select_flag & OL_ITEM_EXTEND;
   const bool activate_data = select_flag & OL_ITEM_SELECT_DATA;
+  const bool recursive = select_flag & OL_ITEM_RECURSIVE;
 
   /* Clear previous active when activating and clear selection when not extending selection */
   const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED);
-  if (clear_flag) {
+
+  /* Do not clear the active and select flag when selecting hierarchies. */
+  if (clear_flag && !recursive) {
     outliner_flag_set(*space_outliner, clear_flag, false);
   }
 
   if (select_flag & OL_ITEM_SELECT) {
     tselem->flag |= TSE_SELECTED;
@@ -1499,11 +1516,14 @@ void outliner_item_select(bContext *C,
 
   if (activate) {
     TreeViewContext tvc;
     outliner_viewcontext_init(C, &tvc);
 
-    tselem->flag |= TSE_ACTIVE;
+    if (!recursive) {
+      tselem->flag |= TSE_ACTIVE;
+    }
+
     do_outliner_item_activate_tree_element(C,
                                            &tvc,
                                            space_outliner,
                                            te,
                                            tselem,

Hey, that seems to work really well! Following might be picky (or wrong), but you've made a change to `do_outliner_object_select_recursive`, passing in a subtree and a sync_select bool. When sync_select is false then *only the current existing code runs*, while if true **ONLY** your new code runs. And that function is only called twice, once without and once with. Therefore you can pull your new code out of there and move it to that second calling site. Makes less changes and seems to be an easier read. What I mean: ```Diff diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc index f77272f79f5..eb99fa7f8dd 100644 --- a/source/blender/editors/space_outliner/outliner_select.cc +++ b/source/blender/editors/space_outliner/outliner_select.cc @@ -355,10 +355,13 @@ static void tree_element_object_activate(bContext *C, if (parent_tselem) { parent_tselem->flag |= TSE_SELECTED; } } } + else if (recursive) { + /* Pass */ + } else { /* De-select all. */ /* Only in object mode so we can switch the active object, * keeping all objects in the current 'mode' selected, useful for multi-pose/edit mode. @@ -380,11 +383,13 @@ static void tree_element_object_activate(bContext *C, do_outliner_object_select_recursive( scene, view_layer, ob, (base->flag & BASE_SELECTED) != 0); } if (set != OL_SETSEL_NONE) { - ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + if (!recursive) { + ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + } DEG_id_tag_update(&scene->id, ID_RECALC_SELECT); WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene); } } } @@ -1411,10 +1416,19 @@ static void do_outliner_item_activate_tree_element(bContext *C, te, (extend && tselem->type == TSE_SOME_ID) ? OL_SETSEL_EXTEND : OL_SETSEL_NORMAL, recursive && tselem->type == TSE_SOME_ID); } + else if (recursive && !(space_outliner->flag & SO_SYNC_SELECT)) { + /* Selection of child objects in hierarchy when sync-selection is OFF. */ + tree_iterator::all(te->subtree, [&](TreeElement *te) { + TreeStoreElem *tselem = TREESTORE(te); + if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) { + tselem->flag |= TSE_SELECTED; + } + }); + } if (tselem->type == TSE_SOME_ID) { /* The lib blocks. */ if (do_activate_data == false) { /* Only select in outliner. */ } @@ -1481,14 +1495,17 @@ void outliner_item_select(bContext *C, { TreeStoreElem *tselem = TREESTORE(te); const bool activate = select_flag & OL_ITEM_ACTIVATE; const bool extend = select_flag & OL_ITEM_EXTEND; const bool activate_data = select_flag & OL_ITEM_SELECT_DATA; + const bool recursive = select_flag & OL_ITEM_RECURSIVE; /* Clear previous active when activating and clear selection when not extending selection */ const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED); - if (clear_flag) { + + /* Do not clear the active and select flag when selecting hierarchies. */ + if (clear_flag && !recursive) { outliner_flag_set(*space_outliner, clear_flag, false); } if (select_flag & OL_ITEM_SELECT) { tselem->flag |= TSE_SELECTED; @@ -1499,11 +1516,14 @@ void outliner_item_select(bContext *C, if (activate) { TreeViewContext tvc; outliner_viewcontext_init(C, &tvc); - tselem->flag |= TSE_ACTIVE; + if (!recursive) { + tselem->flag |= TSE_ACTIVE; + } + do_outliner_item_activate_tree_element(C, &tvc, space_outliner, te, tselem, ```

Pratik Borhade commented

2023-04-06 11:31:00 +02:00

you can pull your new code out of there and move it to that second calling site

I thought about that too. Before reverting the change I'd like @JulianEisel 's opinion on this.

> you can pull your new code out of there and move it to that second calling site I thought about that too. Before reverting the change I'd like @JulianEisel 's opinion on this.

Harley Acheson commented

2023-04-06 23:57:46 +02:00

I thought about that too. Before reverting the change I'd like @JulianEisel 's opinion on this.

No worries. Mine was only a suggestion. You know that area of code much better than me so leave it in a way that makes sense to you.

> I thought about that too. Before reverting the change I'd like @JulianEisel 's opinion on this. No worries. Mine was only a suggestion. You know that area of code much better than me so leave it in a way that makes sense to you.

Pratik Borhade added 1 commit 2023-04-07 11:30:53 +02:00

3e3d5c18fb Fix: Pass correct sync_select value

Pratik Borhade commented

2023-04-07 11:43:41 +02:00

Perhaps we can replace the current function logic (from master) with the below code.

 static void do_outliner_object_select_recursive(const Scene *scene,
                                                 ViewLayer *view_layer,
                                                 Object *ob_parent,
+                                                ListBase *lb,
+                                                const bool sync_select,
                                                 bool select)
 {
   BKE_view_layer_synced_ensure(scene, view_layer);
-  LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_get(view_layer)) {
-    Object *ob = base->object;
-    if (((base->flag & BASE_ENABLED_AND_MAYBE_VISIBLE_IN_VIEWPORT) != 0) &&
-        BKE_object_is_child_recursive(ob_parent, ob)) {
-      ED_object_base_select(base, select ? BA_SELECT : BA_DESELECT);
+
+  tree_iterator::all(*lb, [&](TreeElement *te) {
+    TreeStoreElem *tselem = TREESTORE(te);
+    if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) {
+      if (sync_select) {
+        Object *ob = (Object *)tselem->id;
+        Base *base = BKE_view_layer_base_find(view_layer, ob);
+        ED_object_base_select(base, select ? BA_SELECT : BA_DESELECT);
+      }
+      else {
+        tselem->flag |= TSE_SELECTED;
+      }
     }
-  }
+  });
 }

Perhaps we can replace the current function logic (from master) with the below code. ```Diff static void do_outliner_object_select_recursive(const Scene *scene, ViewLayer *view_layer, Object *ob_parent, + ListBase *lb, + const bool sync_select, bool select) { BKE_view_layer_synced_ensure(scene, view_layer); - LISTBASE_FOREACH (Base *, base, BKE_view_layer_object_bases_get(view_layer)) { - Object *ob = base->object; - if (((base->flag & BASE_ENABLED_AND_MAYBE_VISIBLE_IN_VIEWPORT) != 0) && - BKE_object_is_child_recursive(ob_parent, ob)) { - ED_object_base_select(base, select ? BA_SELECT : BA_DESELECT); + + tree_iterator::all(*lb, [&](TreeElement *te) { + TreeStoreElem *tselem = TREESTORE(te); + if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) { + if (sync_select) { + Object *ob = (Object *)tselem->id; + Base *base = BKE_view_layer_base_find(view_layer, ob); + ED_object_base_select(base, select ? BA_SELECT : BA_DESELECT); + } + else { + tselem->flag |= TSE_SELECTED; + } } - } + }); } ```

Harley Acheson commented

2023-04-20 23:42:57 +02:00

@PratikPB2123 - Sorry for the delay; I didn't notice you made a comment that asked me a question. This new system doesn't seem to notify as much, especially if not specifically at'ed.

Not tested but to my eye this doesn't look it is doing the same things. ob_parent would not be used?

@PratikPB2123 - Sorry for the delay; I didn't notice you made a comment that asked me a question. This new system doesn't seem to notify as much, especially if not specifically at'ed. Not tested but to my eye this doesn't look it is doing the same things. `ob_parent` would not be used?

Pratik Borhade commented

2023-04-21 14:34:49 +02:00

ob_parent would not be used?

No need of it AFAICS. ob_parent is same id data that is stored at te->tselem->id
Instead of using ob_parent, we could directly select all object that belongs to subtree of te

> `ob_parent` would not be used? No need of it AFAICS. `ob_parent` is same id data that is stored at `te->tselem->id` Instead of using ob_parent, we could directly select all object that belongs to subtree of `te`

Pratik Borhade referenced this pull request

2023-05-18 19:42:38 +02:00

`outliner.object_operation (select hierarchy)` does not change active object when directly called through a key event #107996

Pratik Borhade commented

2023-06-02 04:56:49 +02:00

Hi, I think we can close this PR or move this to WIP. Maybe fix is incorrect and that's why I'm not receiving code review?
Furthermore, structure of selection might change after outliner refactor.

Hi, I think we can close this PR or move this to WIP. Maybe fix is incorrect and that's why I'm not receiving code review? Furthermore, structure of selection might change after outliner refactor.

Harley Acheson commented

2023-06-06 23:51:15 +02:00

...and that's why I'm not receiving code review?

It seemed to be WIP since your comment on April 7th was a proposed change that is not in this PR.

I personally still have difficulties with your proposed changes to do_outliner_object_select_recursive, that I have mentioned earlier.

Your new section operates only if sync_select is false - an argument that you add - and then calls none of the existing code. This function is called only twice, once with your new arguments set to NULL and one other time with the all of the first three arguments set to NULL. If a function has two completely exclusive sections it is best that it just be two separate functions.

I think it would be much simpler to leave do_outliner_object_select_recursive as-is and move your new code to the single call site:

diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc
index 4bc503dc3e1..9f561c3d9e3 100644
--- a/source/blender/editors/space_outliner/outliner_select.cc
+++ b/source/blender/editors/space_outliner/outliner_select.cc
@@ -358,10 +358,13 @@ static void tree_element_object_activate(bContext *C,
         if (parent_tselem) {
           parent_tselem->flag |= TSE_SELECTED;
         }
       }
     }
+    else if (recursive) {
+      /* Pass */
+    }
     else {
       /* De-select all. */
 
       /* Only in object mode so we can switch the active object,
        * keeping all objects in the current 'mode' selected, useful for multi-pose/edit mode.
@@ -384,11 +387,13 @@ static void tree_element_object_activate(bContext *C,
       do_outliner_object_select_recursive(
           scene, view_layer, ob, (base->flag & BASE_SELECTED) != 0);
     }
 
     if (set != OL_SETSEL_NONE) {
-      ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      if (!recursive) {
+        ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */
+      }
       DEG_id_tag_update(&scene->id, ID_RECALC_SELECT);
       WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene);
     }
   }
 }
@@ -1417,10 +1422,19 @@ static void do_outliner_item_activate_tree_element(bContext *C,
                                  te,
                                  (extend && tselem->type == TSE_SOME_ID) ? OL_SETSEL_EXTEND :
                                                                            OL_SETSEL_NORMAL,
                                  recursive && tselem->type == TSE_SOME_ID);
   }
+  else if (recursive && !(space_outliner->flag & SO_SYNC_SELECT)) {
+    /* Selection of child objects in hierarchy when sync-selection is OFF. */
+    tree_iterator::all(te->subtree, [&](TreeElement *te) {
+      TreeStoreElem *tselem = TREESTORE(te);
+      if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) {
+        tselem->flag |= TSE_SELECTED;
+      }
+    });
+  }
 
   if (tselem->type == TSE_SOME_ID) { /* The lib blocks. */
     if (do_activate_data == false) {
       /* Only select in outliner. */
     }
@@ -1487,14 +1501,17 @@ void outliner_item_select(bContext *C,
 {
   TreeStoreElem *tselem = TREESTORE(te);
   const bool activate = select_flag & OL_ITEM_ACTIVATE;
   const bool extend = select_flag & OL_ITEM_EXTEND;
   const bool activate_data = select_flag & OL_ITEM_SELECT_DATA;
+  const bool recursive = select_flag & OL_ITEM_RECURSIVE;
 
   /* Clear previous active when activating and clear selection when not extending selection */
   const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED);
-  if (clear_flag) {
+
+  /* Do not clear the active and select flag when selecting hierarchies. */
+  if (clear_flag && !recursive) {
     outliner_flag_set(*space_outliner, clear_flag, false);
   }
 
   if (select_flag & OL_ITEM_SELECT) {
     tselem->flag |= TSE_SELECTED;
@@ -1505,11 +1522,14 @@ void outliner_item_select(bContext *C,
 
   if (activate) {
     TreeViewContext tvc;
     outliner_viewcontext_init(C, &tvc);
 
-    tselem->flag |= TSE_ACTIVE;
+    if (!recursive) {
+      tselem->flag |= TSE_ACTIVE;
+    }
+
     do_outliner_item_activate_tree_element(C,
                                            &tvc,
                                            space_outliner,
                                            te,
                                            tselem,

> ...and that's why I'm not receiving code review? It seemed to be WIP since your comment on April 7th was a proposed change that is not in this PR. I personally still have difficulties with your proposed changes to `do_outliner_object_select_recursive`, that I have mentioned earlier. Your new section operates only if `sync_select` is false - an argument that you add - and then calls none of the existing code. This function is called only twice, once with your new arguments set to NULL and one other time with the all of the first three arguments set to NULL. If a function has two completely **exclusive** sections it is best that it just be two separate functions. I think it would be much simpler to leave `do_outliner_object_select_recursive` as-is and move your new code to the single call site: ```Diff diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc index 4bc503dc3e1..9f561c3d9e3 100644 --- a/source/blender/editors/space_outliner/outliner_select.cc +++ b/source/blender/editors/space_outliner/outliner_select.cc @@ -358,10 +358,13 @@ static void tree_element_object_activate(bContext *C, if (parent_tselem) { parent_tselem->flag |= TSE_SELECTED; } } } + else if (recursive) { + /* Pass */ + } else { /* De-select all. */ /* Only in object mode so we can switch the active object, * keeping all objects in the current 'mode' selected, useful for multi-pose/edit mode. @@ -384,11 +387,13 @@ static void tree_element_object_activate(bContext *C, do_outliner_object_select_recursive( scene, view_layer, ob, (base->flag & BASE_SELECTED) != 0); } if (set != OL_SETSEL_NONE) { - ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + if (!recursive) { + ED_object_base_activate_with_mode_exit_if_needed(C, base); /* adds notifier */ + } DEG_id_tag_update(&scene->id, ID_RECALC_SELECT); WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene); } } } @@ -1417,10 +1422,19 @@ static void do_outliner_item_activate_tree_element(bContext *C, te, (extend && tselem->type == TSE_SOME_ID) ? OL_SETSEL_EXTEND : OL_SETSEL_NORMAL, recursive && tselem->type == TSE_SOME_ID); } + else if (recursive && !(space_outliner->flag & SO_SYNC_SELECT)) { + /* Selection of child objects in hierarchy when sync-selection is OFF. */ + tree_iterator::all(te->subtree, [&](TreeElement *te) { + TreeStoreElem *tselem = TREESTORE(te); + if ((tselem->type == TSE_SOME_ID) && (te->idcode == ID_OB)) { + tselem->flag |= TSE_SELECTED; + } + }); + } if (tselem->type == TSE_SOME_ID) { /* The lib blocks. */ if (do_activate_data == false) { /* Only select in outliner. */ } @@ -1487,14 +1501,17 @@ void outliner_item_select(bContext *C, { TreeStoreElem *tselem = TREESTORE(te); const bool activate = select_flag & OL_ITEM_ACTIVATE; const bool extend = select_flag & OL_ITEM_EXTEND; const bool activate_data = select_flag & OL_ITEM_SELECT_DATA; + const bool recursive = select_flag & OL_ITEM_RECURSIVE; /* Clear previous active when activating and clear selection when not extending selection */ const short clear_flag = (activate ? TSE_ACTIVE : 0) | (extend ? 0 : TSE_SELECTED); - if (clear_flag) { + + /* Do not clear the active and select flag when selecting hierarchies. */ + if (clear_flag && !recursive) { outliner_flag_set(*space_outliner, clear_flag, false); } if (select_flag & OL_ITEM_SELECT) { tselem->flag |= TSE_SELECTED; @@ -1505,11 +1522,14 @@ void outliner_item_select(bContext *C, if (activate) { TreeViewContext tvc; outliner_viewcontext_init(C, &tvc); - tselem->flag |= TSE_ACTIVE; + if (!recursive) { + tselem->flag |= TSE_ACTIVE; + } + do_outliner_item_activate_tree_element(C, &tvc, space_outliner, te, tselem, ```

Pratik Borhade added 2 commits 2023-06-07 11:37:43 +02:00

8c6e5519f8 Merge branch 'main' into T89479-select-multi-hierarchies

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

9b272a44e2 Revert changes in do_outliner_object_select_recursive

- Revert b84e0c72d2
- Handle heirarchy selection in do_outliner_item_activate_tree_element
  when sync-selection is disabled

Pratik Borhade commented

2023-06-07 11:39:54 +02:00

@Harley , thanks. updated the PR :)
Guess I created confusion by adding alternative fix. sorry for that :/

@Harley , thanks. updated the PR :) Guess I created confusion by adding alternative fix. sorry for that :/

Harley Acheson approved these changes 2023-06-20 19:36:42 +02:00

Harley Acheson commented

2023-06-20 19:38:38 +02:00

This seems to work great and the code looks fine. @JulianEisel can you take a look?

Pratik Borhade referenced this pull request

2023-07-20 04:37:17 +02:00

'Select Hierarchy' in outliner does nothing if Sync Selection is turned off #110286

Pratik Borhade commented

2023-07-20 04:38:41 +02:00

PR also fixes: #110286 :)

PR also fixes: https://projects.blender.org/blender/blender/issues/110286 :)

Brecht Van Lommel approved these changes 2023-08-15 19:48:41 +02:00

Harley Acheson commented

2023-08-15 20:36:24 +02:00

@blender-bot build

Harley Acheson commented

2023-08-16 01:33:05 +02:00

@blender-bot build

🎉 1

Harley Acheson commented

2023-08-16 20:39:16 +02:00

@blender-bot build

Harley Acheson commented

2023-08-16 21:20:35 +02:00

@PratikPB2123 - This has enough approval that it be merged now.

This applies to main and works just fine, and I just tested it again.

However , the PR is out of date enough that it won't run on the build bots (against 3.6 libs I think). So ideally you would update it, test on the bots, and commit.

@PratikPB2123 - This has enough approval that it be merged now. This applies to main and works just fine, and I just tested it again. However , the PR is out of date enough that it won't run on the build bots (against 3.6 libs I think). So ideally you would update it, test on the bots, and commit.

Pratik Borhade commented

2023-08-17 04:48:02 +02:00

Sure, I'll do that. Thanks :)

Pratik Borhade added 1 commit 2023-08-17 10:30:29 +02:00