Fix #112767: Outliner unlink operation fails to unlink from scene #112955

Pratik Borhade · 2023-09-27T15:15:39+02:00

Pratik Borhade commented

2023-09-27 15:15:39 +02:00

When in scene mode, operation searches the object in master collection
to unlink. But object can be in any collection. So iterate through all
collections present in scene to unlink the object

When in scene mode, operation searches the object in master collection to unlink. But object can be in any collection. So iterate through all collections present in scene to unlink the object

Pratik Borhade added 1 commit 2023-09-27 15:15:52 +02:00

f468105a90 Fix #112767 : Outliner unlink operation fails to unlink from scene

When in scene mode, operation searches the object in master collection
to unlink. but object can be in any collection. So iterate thorugh all
collections present in scene to unlink the object

Pratik Borhade requested review from Harley Acheson 2023-09-27 15:16:08 +02:00

Pratik Borhade requested review from Julian Eisel 2023-09-27 15:16:08 +02:00

Pratik Borhade added the

Module

User Interface

label 2023-09-27 15:16:18 +02:00

Pratik Borhade referenced this pull request

2023-09-27 15:16:32 +02:00

Unlink Not Work in Outliner in "Scenes" or "Blener File" #112767

Brecht Van Lommel changed title from ~~Fix #112767: Outliner unlink operation fails to unlink from scene~~ to Fix #112767: Outliner unlink operation fails to unlink from scene

2023-09-27 16:01:23 +02:00

brecht changed target branch from main to blender-v4.0-release

2023-09-27 16:01:24 +02:00

Brecht Van Lommel changed title from ~~Fix #112767: Outliner unlink operation fails to unlink from scene~~ to Fix #112767: Outliner unlink operation fails to unlink from scene

2023-09-27 16:06:44 +02:00

brecht changed target branch from blender-v4.0-release to main

2023-09-27 16:06:45 +02:00

Brecht Van Lommel commented

2023-09-27 16:08:42 +02:00

Reminder, bugfixes like this should go to the 4.0 branch. But just switching in Gitea isn't enough as the commit needs to be rebased also.

Pratik Borhade commented

2023-09-28 12:49:38 +02:00

@brecht , sure. I'll rebase the branch once PR is approved :)

Maintaining working branch with "main" seems handy

@brecht , sure. I'll rebase the branch once PR is approved :) Maintaining working branch with "main" seems handy

Julian Eisel requested review from Bastien Montagne 2023-11-10 11:55:15 +01:00

Julian Eisel approved these changes 2023-11-10 11:59:35 +01:00

Julian Eisel left a comment

Seems correct, but would like somebody like Bastien or Brecht to double check.

Bastien Montagne requested changes 2023-11-10 13:04:02 +01:00

source/blender/editors/space_outliner/outliner_tools.cc Outdated

						
				@ -467,3 +467,2 @@

				        Scene *scene = (Scene *)tsep->id;

				        Collection *parent = scene->master_collection;

				        BKE_collection_object_remove(bmain, parent, ob, true);

				        FOREACH_SCENE_COLLECTION_BEGIN (scene, collection) {

Bastien Montagne commented

2023-11-10 13:03:58 +01:00

Would need a comment here, since this only works because the only case when the parent item is a Scene is in the Scene view of the Outliner. in ViewLayer view, the parent of an object item is always a collection (normal one, or the Scene's master collection).

In fact, would also be good to have an assert that this branch is only executed from an Outliner in Scene view...

Would need a comment here, since this only works because the only case when the parent item is a Scene is in the `Scene` view of the Outliner. in `ViewLayer` view, the parent of an object item is always a collection (normal one, or the Scene's master collection). In fact, would also be good to have an assert that this branch is only executed from an Outliner in `Scene` view...

Pratik Borhade commented

2023-11-22 14:53:06 +01:00

@mont29 , I think the condition else if (GS(tsep->id->name) == ID_SCE) { itself suggests we're in outliner scene view.
Because in scene view, parent of selected data references to scene itself.
If object is child of some object then we're finding parent scene/collection to unlink the object from: see

@mont29 , I think the condition `else if (GS(tsep->id->name) == ID_SCE) {` itself suggests we're in outliner `scene` view. Because in scene view, parent of selected data references to `scene` itself. If object is child of some object then we're finding parent scene/collection to unlink the object from: [see](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/space_outliner/outliner_tools.cc#L443)

Bastien Montagne commented

2023-11-22 15:46:29 +01:00

What I am saying is that this code makes certain assumptions about the context in which it is executed, and that these assumptions should be both documented as a comment, and preferably validated with some assert

I think the condition else if (GS(tsep->id->name) == ID_SCE) { itself suggests we're in outliner Scene view.

That is exactly what I am asking you to explain in the comment. Because there is nothing 'obvious' about that assumption, and while correct in current code, it may very well become wrong in the future, causing the code to have an unexpected/buggy behavior.

What I am saying is that this code makes certain assumptions about the context in which it is executed, and that these assumptions should be both documented as a comment, and preferably validated with some `assert` > I think the condition `else if (GS(tsep->id->name) == ID_SCE) {` itself suggests we're in outliner `Scene` view. That is exactly what I am asking you to explain in the comment. Because there is nothing 'obvious' about that assumption, and while correct in current code, it may very well become wrong in the future, causing the code to have an unexpected/buggy behavior.

Pratik Borhade commented

2023-11-23 05:04:05 +01:00

Ok, thanks for clarifying 😅
Updated, I hope this one liner comment is sufficient :)

Ok, thanks for clarifying 😅 Updated, I hope this one liner comment is sufficient :)

Pratik Borhade added 2 commits 2023-11-23 05:02:36 +01:00

bf87dc01f4 Merge branch 'main' into 112767-outliner-scene-unlink

405cd45366 Add comment and assert

Bastien Montagne approved these changes 2023-11-23 10:38:51 +01:00

Bastien Montagne left a comment

Thanks, yes it's essentially good to go now.

Following points can be addressed before final commit, no need for extra review for them.

Thanks, yes it's essentially good to go now. Following points can be addressed before final commit, no need for extra review for them.

source/blender/editors/space_outliner/outliner_tools.cc Outdated

						
				@ -473,9 +473,17 @@ static void unlink_object_fn(bContext *C,

				        DEG_relations_tag_update(bmain);

				      }

				      else if (GS(tsep->id->name) == ID_SCE) {

				        /* Following execution is expected to happen in outliner scene view. */

Bastien Montagne commented

2023-11-23 10:37:48 +01:00

to happen exclusively in the Outliner Scene view

`to happen exclusively in the Outliner Scene view`

source/blender/editors/space_outliner/outliner_tools.cc Outdated

						
				@ -474,2 +474,4 @@

				      }

				      else if (GS(tsep->id->name) == ID_SCE) {

				        /* Following execution is expected to happen in outliner scene view. */

				        SpaceOutliner *space_outliner = CTX_wm_space_outliner(C);