Outliner: Double-click on item icon to select contents/hierarchy #110151

Merged
Harley Acheson merged 12 commits from mgradysaunders/blender:temp-outliner-selection into main 2024-01-17 00:45:52 +01:00
Contributor

This is the migrated PR for #99275, which I started working on back before Blender moved to the new git management setup.

The gist of the feature is to support double clicking on icons on the outliner to recursively select collections and objects, regardless of whether the layers are open or closed. This is meant to provide a snappier unified mechanism for the Select Objects and Select Hierarchy routines.

Overall the diff is relatively minimal and the core behavior is relatively easy to understand. However, there are edge cases that require more careful consideration, most notably what to do in the event that an object has a child which is part of a different collection. The current implementation is based on the condition that "recursive selections do not cross collection boundaries" which seems to be the consensus opinion of how it should work in the associated thread (#99275), but this is of course open for revision. Refer to the thread for more details.

This is the migrated PR for [#99275](https://projects.blender.org/blender/blender/issues/99275), which I started working on back before Blender moved to the new git management setup. The gist of the feature is to support double clicking on icons on the outliner to recursively select collections and objects, regardless of whether the layers are open or closed. This is meant to provide a snappier unified mechanism for the Select Objects and Select Hierarchy routines. Overall the diff is relatively minimal and the core behavior is relatively easy to understand. However, there are edge cases that require more careful consideration, most notably what to do in the event that an object has a child which is part of a different collection. The current implementation is based on the condition that "recursive selections do not cross collection boundaries" which seems to be the consensus opinion of how it should work in the associated thread (#99275), but this is of course open for revision. Refer to the thread for more details.
M. Grady Saunders added 1 commit 2023-07-16 01:16:06 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
1dab79ecba
Initialize changes from local diff.
Member

hi, guess PR is ready for the review? Could you remove WIP prefix in that case?

hi, guess PR is ready for the review? Could you remove `WIP` prefix in that case?
Author
Contributor

Right, yes. I had gone off and forgot about this again, not realizing that the WIP prefix might prevent people from inspecting the behavior here in the meantime. (Never contributed to Blender before, have another full-time job, excuses excuses)

I think the status of the feature is consistent and seemingly meeting all of the outlined requirements from the original post. Though deciding what the most sensible behavior should be turns out to be more of a judgment and less of a purely logical deliberation.

It would be nice to have some Blender power-users or devs with more authority to click around and use the feature and see whether it "feels right" and if not to have some concrete direction as to what is should do instead. Up until this point, it has been just me testing it out and recording demo videos.

Right, yes. I had gone off and forgot about this again, not realizing that the WIP prefix might prevent people from inspecting the behavior here in the meantime. (Never contributed to Blender before, have another full-time job, excuses excuses) I think the status of the feature is consistent and seemingly meeting all of the outlined requirements from the original post. Though deciding what the most sensible behavior should be turns out to be more of a judgment and less of a purely logical deliberation. It would be nice to have some Blender power-users or devs with more authority to click around and use the feature and see whether it "feels right" and if not to have some concrete direction as to what is should do instead. Up until this point, it has been just me testing it out and recording demo videos.
M. Grady Saunders changed title from WIP: Double click on icon to select contents/hierachy to Double click on icon to select contents/hierachy 2023-08-17 16:44:00 +02:00
Member
  • Though deciding what the most sensible behavior should be...
  • It would be nice to have some Blender power-users or devs with more authority to click around and use the feature and see whether it "feels right"

@pablovazquez , @JulienKaspar can you check?

> - Though deciding what the most sensible behavior should be... > - It would be nice to have some Blender power-users or devs with more authority to click around and use the feature and see whether it "feels right" @pablovazquez , @JulienKaspar can you check?
Pratik Borhade added the
Module
User Interface
label 2023-08-18 11:28:22 +02:00
Member

Happy to see this one back!

I've just compiled and tested it and all combinations of Ctrl, Shift, work as expected.
I may be missing something so I'll kick the buildbot (once it's back online) so others can test it.

Happy to see this one back! I've just compiled and tested it and all combinations of Ctrl, Shift, work as expected. I may be missing something so I'll kick the buildbot (once it's back online) so others can test it.
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110151) when ready.
Pablo Vazquez changed title from Double click on icon to select contents/hierachy to Outliner: Double-click on item icon to select contents/hierarchy 2023-08-18 14:31:17 +02:00
Harley Acheson reviewed 2023-08-19 18:24:51 +02:00
@ -404,3 +404,3 @@
BKE_view_layer_synced_ensure(scene, view_layer);
if (ob == nullptr || ob != BKE_view_layer_active_object_get(view_layer) ||
ob->matbits == nullptr) {
ob->matbits == nullptr)
Member

This appears to be an unintentional formatting-only change.

This appears to be an unintentional formatting-only change.
Harley marked this conversation as resolved
Harley Acheson reviewed 2023-08-19 18:25:03 +02:00
@ -1044,3 +1045,3 @@
BKE_view_layer_synced_ensure(scene, view_layer);
if (ob == nullptr || ob != BKE_view_layer_active_object_get(view_layer) ||
ob->matbits == nullptr) {
ob->matbits == nullptr)
Member

This appears to be an unintentional formatting-only change.

This appears to be an unintentional formatting-only change.
Harley marked this conversation as resolved
Harley Acheson added this to the User Interface project 2023-08-19 18:25:44 +02:00
Member

@mgradysaunders Thanks for working on this.

As of the merge of #104737 two days ago, "Select Hierarchy" should now be working correctly if multiple Outliner items are selected. Before this it would only work with a single hierarchy, just that of the active object/collection. But now it works if you shift- or ctrl-click multiple objects. Does this change allow a simplification of this PR? @PratikPB2123 do you mind taking a look?

@mgradysaunders Thanks for working on this. As of the merge of #104737 two days ago, "Select Hierarchy" should now be working correctly if multiple Outliner items are selected. Before this it would only work with a single hierarchy, just that of the active object/collection. But now it works if you shift- or ctrl-click multiple objects. Does this change allow a simplification of this PR? @PratikPB2123 do you mind taking a look?
Pratik Borhade reviewed 2023-08-20 09:13:36 +02:00
Pratik Borhade left a comment
Member

@Harley , Sure :)
@mgradysaunders , could you update the PR with main? I'm not able to apply the PR on current main.

Bots are failing too.


Also: did you check how this PR selects tree-element when sync-selection is disabled?


Some comments:

@Harley , Sure :) @mgradysaunders , could you [update the PR with main](https://wiki.blender.org/wiki/Tools/Pull_Requests#:~:text=Update%20a%20Pull%20Request)? I'm not able to apply the PR on current main. Bots are failing too. - - - Also: did you check how this PR selects tree-element when sync-selection is disabled? - - - Some comments:
@ -1524,2 +1559,3 @@
TreeElement *cursor,
bool selecting)
bool selecting,
bool recurse,
Member

make recurse const?

make recurse `const`?
Harley marked this conversation as resolved
@ -1584,2 +1640,3 @@
do_outliner_range_select_recursive(&space_outliner->tree, active, cursor, false);
Collection *in_collection = nullptr;
if (recurse) {
Member

There is code duplication.
in_collection is already found for active tree element in outliner_item_do_activate_from_cursor. So just pass it as argument to this function?

There is code duplication. `in_collection` is already found for active tree element in `outliner_item_do_activate_from_cursor`. So just pass it as argument to this function?
Harley marked this conversation as resolved
@ -1653,3 +1722,3 @@
}
else {
/* The row may also contain children, if one is hovered we want this instead of current te. */
/* The row may also contain children, if one is hovered we want this instead of current te.
Member

unintentional formatting?

unintentional formatting?
Harley marked this conversation as resolved
M. Grady Saunders added 1 commit 2023-08-20 19:06:55 +02:00
Author
Contributor

I was able to sync with upstream ok and my rebuild is good. Should be up to date now. I'm looking into implementing the suggested changes.

I do see there are some formatting discrepancies in a few places --- I do use clang-format, so all of that should be automated, but perhaps there is a version or something that may have caused some things to be shuffled around? I'll update LLVM build to see if that fixes it.

I was able to sync with upstream ok and my rebuild is good. Should be up to date now. I'm looking into implementing the suggested changes. I do see there are some formatting discrepancies in a few places --- I do use clang-format, so all of that should be automated, but perhaps there is a version or something that may have caused some things to be shuffled around? I'll update LLVM build to see if that fixes it.
Member

I've tested this for a bit and it all works like expected. Great job! 👍
If there are any issues I'm sure we can fix them in Bcon2 and 3.

The only odd edge edge case that I've noticed is that if a collection is disabled it will not have any of its objects selected on double click, even if that collection is linked and enabled somewhere else in the outliner.
But that seems more like a general design decision with disabling collections. I'd need more time to think about that one.

I've tested this for a bit and it all works like expected. Great job! 👍 If there are any issues I'm sure we can fix them in Bcon2 and 3. The only odd edge edge case that I've noticed is that if a collection is disabled it will not have any of its objects selected on double click, even if that collection is linked and enabled somewhere else in the outliner. But that seems more like a general design decision with disabling collections. I'd need more time to think about that one.
Pratik Borhade requested changes 2023-08-21 14:47:41 +02:00
Pratik Borhade left a comment
Member

A few more comments.
Apart from this. Code is working correctly :D
Code could be simplified further, more experienced UI dev knows better about it.

A few more comments. Apart from this. Code is working correctly :D Code could be simplified further, more experienced UI dev knows better about it.
@ -1572,0 +1591,4 @@
/* Only actually select the object if
1. We are not restricted to any collection, or
2. The object is in fact in the given collection. */
if (!in_collection || BKE_collection_has_object_recursive(
Member

for object ID, can we use do_outliner_object_select_recursive directly?

for object ID, can we use `do_outliner_object_select_recursive` directly?
PratikPB2123 marked this conversation as resolved
@ -1719,0 +1810,4 @@
select recursively. */
if (!recurse && (extend || use_range) &&
outliner_item_is_co_over_icon(activate_te, view_mval[0]))
return OPERATOR_CANCELLED;
Member

use brackets {}

use brackets {}
Harley marked this conversation as resolved
@ -1720,1 +1816,3 @@
do_outliner_range_select(C, space_outliner, activate_te, extend);
do_outliner_range_select(C, space_outliner, activate_te, extend, recurse);
if (recurse)
do_outliner_select_recursive(&activate_te->subtree, /*selecting=*/true, in_collection);
Member

use brackets {}

use brackets {}
Harley marked this conversation as resolved
@ -1728,0 +1827,4 @@
/* If we're CTRL+double-clicking and the element is aleady selected, skip the activation
straight to deselection */
if (extend && recurse && activate_tselem->flag & TSE_SELECTED)
Member

use brackets {}

use brackets {}
Harley marked this conversation as resolved
First-time contributor

Thank you so much @mgradysaunders for getting back at this!

I found a bug on the most recent downloadable build. If you do Shift dbl-click from top to bottom everything is fine, but if you do it from bottom to top, the objects of the first-selected collection get deselected (See "Selection Bug" video). This happens also with parents.

I wanted to ask something. Since I'd like this feature to be more immediate, I tried to change in the keymap the operator to be from dbl-click to single click. The problem is that this creates conflict with the other selection operators of the outliner which are also single click, resulting in hierarchy selection not working. One "solution" I found was to disable the select operator (See "Keymap Change" video), that way all the hierarchy selctions work, but now I lost the ability to select by clicking on names in the outliner.

Would be possible to make this feature so that if we want to change it from dbl-click to single click, doesn't create conflict with the other regular selections, having the best from both worlds?

Thank you so much @mgradysaunders for getting back at this! I found a bug on the most recent downloadable build. If you do Shift dbl-click from top to bottom everything is fine, but if you do it from bottom to top, the objects of the first-selected collection get deselected **(See "Selection Bug" video)**. This happens also with parents. I wanted to ask something. Since I'd like this feature to be more immediate, I tried to change in the keymap the operator to be from dbl-click to single click. The problem is that this creates conflict with the other selection operators of the outliner which are also single click, resulting in hierarchy selection not working. One "solution" I found was to disable the select operator **(See "Keymap Change" video)**, that way all the hierarchy selctions work, but now I lost the ability to select by clicking on names in the outliner. Would be possible to make this feature so that if we want to change it from dbl-click to single click, doesn't create conflict with the other regular selections, having the best from both worlds?
Harley Acheson added this to the 4.0 milestone 2023-08-22 19:25:31 +02:00
Harley Acheson added 2 commits 2023-09-11 19:49:40 +02:00
Harley Acheson added 1 commit 2023-09-11 19:54:25 +02:00
Member

@PratikPB2123

I updated this PR to the current state main. Removed the unintended formatting changes and addressed the review comments, except for your "for object ID, can we use do_outliner_object_select_recursive directly?" as I wasn't sure how to proceed there. There seems to be some difference in the functions based on in_collection, but not sure.

@PratikPB2123 I updated this PR to the current state main. Removed the unintended formatting changes and addressed the review comments, except for your "for object ID, can we use do_outliner_object_select_recursive directly?" as I wasn't sure how to proceed there. There _seems_ to be some difference in the functions based on `in_collection`, but not sure.
Member

@Harley , we can ignore that comment. With some modification we can use do_outliner_object_select_recursive but that's a topic for separate discussion.

@Harley , we can ignore that comment. With some modification we can use `do_outliner_object_select_recursive` but that's a topic for separate discussion.
Pratik Borhade approved these changes 2023-09-12 13:13:39 +02:00
Pratik Borhade left a comment
Member

I've checked again the code, did some tests and it's working as expected 😃
I think PR is ready to land?

I've checked again the code, did some tests and it's working as expected 😃 I think PR is ready to land?
Harley Acheson requested review from Julian Eisel 2023-09-12 17:27:05 +02:00
Julian Eisel requested changes 2023-09-22 18:51:01 +02:00
Julian Eisel left a comment
Member

I think this is looking good mostly, just some small tweaks to do. A bit unfortunate that this makes the operator even more complicated, but it's hard to avoid that.

I think this is looking good mostly, just some small tweaks to do. A bit unfortunate that this makes the operator even more complicated, but it's hard to avoid that.
@ -1571,30 +1571,83 @@ void outliner_item_select(bContext *C,
}
}
/* A simple but apparently effective solution for recursive selections. */
Member

This comment doesn't really add much, would remove it.

This comment doesn't really add much, would remove it.
Harley marked this conversation as resolved
@ -1574,0 +1577,4 @@
LISTBASE_FOREACH (TreeElement *, te, lb) {
TreeStoreElem *tselem = TREESTORE(te);
/* The desired behavior is only to select collections and object hierarchies
Member

Code-style: Every multi-line comment should have a * in new lines, see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2FC.2B.2B_Comments

Code-style: Every multi-line comment should have a `*` in new lines, see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2FC.2B.2B_Comments
Harley marked this conversation as resolved
@ -1585,0 +1620,4 @@
is kinda gross but necessary for consistent behavior in the recursive case. Essentially
we need to be sure that the object belongs to the right collection before setting the
SELECTED flag. */
bool can_select = !recurse || (tselem->type == TSE_LAYER_COLLECTION ||
Member

This logic is quite complicated. You could split this up into multiple bools.

However, I think it's good to move this into an own function, with a descriptive name. In there you can have multiple if statements, and explain each condition nicely.

This logic is quite complicated. You could split this up into multiple bools. However, I think it's good to move this into an own function, with a descriptive name. In there you can have multiple if statements, and explain each condition nicely.
Member

@JulianEisel - I find this area and this logic a bit too mystifying to simplify this.

@JulianEisel - I find this area and this logic a bit too mystifying to simplify this.
Member

This can be moved to one separate function (for example: can_select_recurse) where we can handle collection and object cases separately. I'll share the simplified version with the PR applied shortly

This can be moved to one separate function (for example: `can_select_recurse`) where we can handle collection and object cases separately. I'll share the simplified version with the PR applied shortly
Harley marked this conversation as resolved
@ -1598,0 +1642,4 @@
if (!(tselem->flag & TSE_CLOSED) || recurse) {
/* If this tree element is a collection, then it sets the precedent for inclusion
of its subobjects. */
Collection *in_collection2 = in_collection;
Member

Could use a better name, like nested_collection or child_collection.

Could use a better name, like `nested_collection` or `child_collection`.
Harley marked this conversation as resolved
@ -1721,0 +1779,4 @@
to prevent selecting across collection boundaries. (Object hierarchies might cross
collection boundaries, i.e., children may be in different collections from their
parents.) */
Collection *in_collection = nullptr;
Member

in_collection sounds like a boolean variable. Maybe parent_collection?

`in_collection` sounds like a boolean variable. Maybe `parent_collection`?
Harley marked this conversation as resolved
@ -1728,2 +1817,2 @@
const bool select = !extend || !(activate_tselem->flag & TSE_ACTIVE &&
activate_tselem->flag & TSE_SELECTED);
bool select = !extend ||
!(activate_tselem->flag & TSE_ACTIVE && activate_tselem->flag & TSE_SELECTED);
Member

I think this reads easier:

bool select = !extend || !(activate_tselem->flag & TSE_ACTIVE) || ! (activate_tselem->flag & TSE_SELECTED);
I think this reads easier: ``` bool select = !extend || !(activate_tselem->flag & TSE_ACTIVE) || ! (activate_tselem->flag & TSE_SELECTED); ```
Harley marked this conversation as resolved
Member

Hi @mgradysaunders, do you think you'd have time to address the review feedback? Even though it's late for this to get in Blender 4.0, 4.1 is already in bcon1 and such a functionality is great to have early in the Alpha stage for the community to test.

Hi @mgradysaunders, do you think you'd have time to address the review feedback? Even though it's late for this to get in Blender 4.0, 4.1 is already in `bcon1` and such a functionality is great to have early in the Alpha stage for the community to test.
First-time contributor

@mgradysaunders it would be great, if this extremely useful feature could finally land in main after all this time. Or @pablovazquez could this patch be discussed/posted in the UI Meeting to maybe trigger some awareness in the community, or finalize it directly?

@mgradysaunders it would be great, if this extremely useful feature could finally land in main after all this time. Or @pablovazquez could this patch be discussed/posted in the UI Meeting to maybe trigger some awareness in the community, or finalize it directly?
Brecht Van Lommel removed this from the 4.0 milestone 2024-01-02 11:13:43 +01:00
Harley Acheson added 2 commits 2024-01-11 18:56:46 +01:00
Pratik Borhade reviewed 2024-01-12 13:18:10 +01:00
@ -1596,0 +1643,4 @@
if (tselem->type == TSE_LAYER_COLLECTION) {
child_collection = static_cast<LayerCollection *>(te->directdata)->collection;
}
selecting = do_outliner_range_select_recursive(
Member

Maybe we can wrap this inside if(can_select). Otherwise unnecessarily we will iterate the children of object that are not present in current collection

Maybe we can wrap this inside `if(can_select)`. Otherwise unnecessarily we will iterate the children of object that are not present in current collection
Harley marked this conversation as resolved
Member

@Harley , something like (also removed the duplicate code and used can_select_recurse there)

Code Changes
diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc
index 7ea73d0324e..7b7198ae940 100644
--- a/source/blender/editors/space_outliner/outliner_select.cc
+++ b/source/blender/editors/space_outliner/outliner_select.cc
@@ -1569,6 +1569,27 @@ void outliner_item_select(bContext *C,
   }
 }

+bool can_select_recurse(TreeElement *te, Collection *in_collection)
+{
+  switch (te->store_elem->type) {
+    case TSE_LAYER_COLLECTION:
+      return true;
+    case TSE_SOME_ID: {
+      /* Only actually select the object if
+         1. We are not restricted to any collection, or
+         2. The object is in fact in the given collection. */
+      if (te->idcode == ID_OB) {
+        if (!in_collection || BKE_collection_has_object_recursive(
+                                  in_collection, reinterpret_cast<Object *>(te->store_elem->id)))
+        {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 static void do_outliner_select_recursive(ListBase *lb, bool selecting, Collection *in_collection)
 {
   LISTBASE_FOREACH (TreeElement *, te, lb) {
@@ -1576,24 +1597,17 @@ static void do_outliner_select_recursive(ListBase *lb, bool selecting, Collectio

     /* The desired behavior is only to select collections and object hierarchies
      *  recursively. So if this isn't a collection or an object, skip it. */
-    if (tselem->type == TSE_LAYER_COLLECTION) {
+    if (can_select_recurse(te, in_collection)) {
       tselem->flag = selecting ? (tselem->flag | TSE_SELECTED) : (tselem->flag & ~TSE_SELECTED);
       /* Restrict sub-tree selections to this collection. This prevents undesirable behavior in
          the edge-case where there is an object which is part of this collection, but which has
          children that are part of another collection. */
-      do_outliner_select_recursive(
-          &te->subtree, selecting, static_cast<LayerCollection *>(te->directdata)->collection);
-    }
-    else if (tselem->type == TSE_SOME_ID && te->idcode == ID_OB) {
-      /* Only actually select the object if
-         1. We are not restricted to any collection, or
-         2. The object is in fact in the given collection. */
-      if (!in_collection || BKE_collection_has_object_recursive(
-                                in_collection, reinterpret_cast<Object *>(tselem->id)))
-      {
-        tselem->flag = selecting ? (tselem->flag | TSE_SELECTED) : (tselem->flag & ~TSE_SELECTED);
-        do_outliner_select_recursive(&te->subtree, selecting, in_collection);
-      }
+      Collection *new_in_collection =
+          (tselem->type == TSE_LAYER_COLLECTION) ?
+              static_cast<LayerCollection *>(te->directdata)->collection :
+              in_collection;
+
+      do_outliner_select_recursive(&te->subtree, selecting, new_in_collection);
     }
     else {
       tselem->flag &= ~TSE_SELECTED;
@@ -1611,17 +1625,7 @@ static bool do_outliner_range_select_recursive(ListBase *lb,
   LISTBASE_FOREACH (TreeElement *, te, lb) {
     TreeStoreElem *tselem = TREESTORE(te);

-    /* If recurse is true, we are in recursive selection mode meaning we double clicked
-       on an icon. In this case, we want to be consistent with ordinary (non-range) recursive
-       selection mode by only selecting layer collections and objects. The object condition
-       is kinda gross but necessary for consistent behavior in the recursive case. Essentially
-       we need to be sure that the object belongs to the right collection before setting the
-       SELECTED flag. */
-    bool can_select = !recurse || (tselem->type == TSE_LAYER_COLLECTION ||
-                                   (tselem->type == TSE_SOME_ID && te->idcode == ID_OB &&
-                                    (!in_collection ||
-                                     BKE_collection_has_object_recursive(
-                                         in_collection, reinterpret_cast<Object *>(tselem->id)))));
+    bool can_select = !recurse || can_select_recurse(te, in_collection);

     /* Remember if we are selecting before we potentially change the selecting state. */
     bool selecting_before = selecting;
@Harley , something like (also removed the duplicate code and used `can_select_recurse` there) <details> <summary> Code Changes </summary> ``` diff --git a/source/blender/editors/space_outliner/outliner_select.cc b/source/blender/editors/space_outliner/outliner_select.cc index 7ea73d0324e..7b7198ae940 100644 --- a/source/blender/editors/space_outliner/outliner_select.cc +++ b/source/blender/editors/space_outliner/outliner_select.cc @@ -1569,6 +1569,27 @@ void outliner_item_select(bContext *C, } } +bool can_select_recurse(TreeElement *te, Collection *in_collection) +{ + switch (te->store_elem->type) { + case TSE_LAYER_COLLECTION: + return true; + case TSE_SOME_ID: { + /* Only actually select the object if + 1. We are not restricted to any collection, or + 2. The object is in fact in the given collection. */ + if (te->idcode == ID_OB) { + if (!in_collection || BKE_collection_has_object_recursive( + in_collection, reinterpret_cast<Object *>(te->store_elem->id))) + { + return true; + } + } + } + } + return false; +} + static void do_outliner_select_recursive(ListBase *lb, bool selecting, Collection *in_collection) { LISTBASE_FOREACH (TreeElement *, te, lb) { @@ -1576,24 +1597,17 @@ static void do_outliner_select_recursive(ListBase *lb, bool selecting, Collectio /* The desired behavior is only to select collections and object hierarchies * recursively. So if this isn't a collection or an object, skip it. */ - if (tselem->type == TSE_LAYER_COLLECTION) { + if (can_select_recurse(te, in_collection)) { tselem->flag = selecting ? (tselem->flag | TSE_SELECTED) : (tselem->flag & ~TSE_SELECTED); /* Restrict sub-tree selections to this collection. This prevents undesirable behavior in the edge-case where there is an object which is part of this collection, but which has children that are part of another collection. */ - do_outliner_select_recursive( - &te->subtree, selecting, static_cast<LayerCollection *>(te->directdata)->collection); - } - else if (tselem->type == TSE_SOME_ID && te->idcode == ID_OB) { - /* Only actually select the object if - 1. We are not restricted to any collection, or - 2. The object is in fact in the given collection. */ - if (!in_collection || BKE_collection_has_object_recursive( - in_collection, reinterpret_cast<Object *>(tselem->id))) - { - tselem->flag = selecting ? (tselem->flag | TSE_SELECTED) : (tselem->flag & ~TSE_SELECTED); - do_outliner_select_recursive(&te->subtree, selecting, in_collection); - } + Collection *new_in_collection = + (tselem->type == TSE_LAYER_COLLECTION) ? + static_cast<LayerCollection *>(te->directdata)->collection : + in_collection; + + do_outliner_select_recursive(&te->subtree, selecting, new_in_collection); } else { tselem->flag &= ~TSE_SELECTED; @@ -1611,17 +1625,7 @@ static bool do_outliner_range_select_recursive(ListBase *lb, LISTBASE_FOREACH (TreeElement *, te, lb) { TreeStoreElem *tselem = TREESTORE(te); - /* If recurse is true, we are in recursive selection mode meaning we double clicked - on an icon. In this case, we want to be consistent with ordinary (non-range) recursive - selection mode by only selecting layer collections and objects. The object condition - is kinda gross but necessary for consistent behavior in the recursive case. Essentially - we need to be sure that the object belongs to the right collection before setting the - SELECTED flag. */ - bool can_select = !recurse || (tselem->type == TSE_LAYER_COLLECTION || - (tselem->type == TSE_SOME_ID && te->idcode == ID_OB && - (!in_collection || - BKE_collection_has_object_recursive( - in_collection, reinterpret_cast<Object *>(tselem->id))))); + bool can_select = !recurse || can_select_recurse(te, in_collection); /* Remember if we are selecting before we potentially change the selecting state. */ bool selecting_before = selecting; ``` </details>
Harley Acheson added 2 commits 2024-01-12 19:32:26 +01:00
Member

@PratikPB2123

Awesome! Those changes make perfect sense. I updated this PR with those changes, altered only a bit to read a bit better and to fit nicer with existing comments. But please double-check.

Note that we are basically relying on you to test this thoroughly as you have been doing a good job of testing all the corner cases.

@PratikPB2123 Awesome! Those changes make perfect sense. I updated this PR with those changes, altered only a bit to read a bit better and to fit nicer with existing comments. But please double-check. Note that we are basically relying on you to test this thoroughly as you have been doing a good job of testing all the corner cases.
Harley Acheson added 1 commit 2024-01-12 19:45:00 +01:00
Harley Acheson added 1 commit 2024-01-12 19:50:33 +01:00
Pratik Borhade approved these changes 2024-01-14 07:15:33 +01:00
Pratik Borhade left a comment
Member

@Harley hi, changes looks correct now. And AFAICT feature is working as expected.
I think we can land the PR (If Julien has no concerns as well). If someone finds a bug later, I can help fixing them.

@Harley hi, changes looks correct now. And AFAICT feature is working as expected. I think we can land the PR (If Julien has no concerns as well). If someone finds a bug later, I can help fixing them.
@ -1765,10 +1875,11 @@ static int outliner_item_activate_invoke(bContext *C, wmOperator *op, const wmEv
const bool extend = RNA_boolean_get(op->ptr, "extend");
const bool use_range = RNA_boolean_get(op->ptr, "extend_range");
const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
const bool recurse = RNA_boolean_get(op->ptr, "recurse");
Member

If we do && outliner_item_is_co_over_icon() here then it would make more sense (since this is intended to work when double clicking on icon) but that's optional.

outliner_item_is_co_over_icon requires tree-element under cursor which is calculated in outliner_item_do_activate_from_cursor.

If we do `&& outliner_item_is_co_over_icon()` here then it would make more sense (since this is intended to work when double clicking on icon) but that's optional. `outliner_item_is_co_over_icon` requires tree-element under cursor which is calculated in `outliner_item_do_activate_from_cursor`.
Harley Acheson added 1 commit 2024-01-16 23:03:53 +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-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
80b9671caf
Merge branch 'main' of projects.blender.org:blender/blender into pr110151
Member

@blender-bot build

@blender-bot build
Member

@PratikPB2123

Let's leave it at this for now. Thank you so much for all your help with this one. And yes, we are going to lean on you for bug reports because of your familiarity here.

@PratikPB2123 Let's leave it at this for now. Thank you so much for all your help with this one. And yes, we are going to lean on you for bug reports because of your familiarity here.
Harley Acheson merged commit 796577d76e into main 2024-01-17 00:45:52 +01:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
9 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110151
No description provided.