Outliner: Port bone elements to new tree-element code design #109684

Almaz-Shinbay · 2023-07-04T08:32:09+02:00

Almaz-Shinbay commented

2023-07-04 08:32:09 +02:00

No user visible changes expected.

Part of #96713, continuation of work started in 249e4df110 and 2e221de4ce.
Refer to these for a motivation and design overview.

Adds a new class for bone elements.

No user visible changes expected. Part of #96713, continuation of work started in 249e4df110 and 2e221de4ce. Refer to these for a motivation and design overview. Adds a new class for bone elements.

Almaz-Shinbay added 2 commits 2023-07-04 08:32:20 +02:00

Outliner: Port bone elements to new tree-element code design 828cbff504

Include new files in CMakeLists.txt 1b85b3a077

Almaz-Shinbay requested review from Julian Eisel 2023-07-04 08:33:00 +02:00

Julian Eisel requested changes 2023-07-04 15:01:41 +02:00

Julian Eisel left a comment

There's a bug here, noted inline.

source/blender/editors/space_outliner/tree/tree_element.cc Outdated

						
				@ -103,2 +104,4 @@

				      return std::make_unique<TreeElementSequenceStripDuplicate>(legacy_te,

				                                                                 *static_cast<Sequence *>(idv));

				    case TSE_BONE:

				      return std::make_unique<TreeElementBone>(legacy_te, *static_cast<Bone *>(idv));

Julian Eisel commented

2023-07-04 15:01:09 +02:00

In the outliner_add_element() call, the armature is passed as idv, but here you are casting it to a Bone *. This is a bug.

Since for now there's only one custom argument you can pass, you can create a wrapper struct like this:

struct BoneElementCreateData {
  ID *armature_id;
  Bone *bone;
};

The TreeElementBone constructor can take both a ID & and a Bone &.

In the `outliner_add_element()` call, the armature is passed as `idv`, but here you are casting it to a `Bone *`. This is a bug. Since for now there's only one custom argument you can pass, you can create a wrapper struct like this: ```Cpp struct BoneElementCreateData { ID *armature_id; Bone *bone; }; ``` The `TreeElementBone` constructor can take both a `ID &` and a `Bone &`.

Almaz-Shinbay marked this conversation as resolved

source/blender/editors/space_outliner/tree/tree_element_id_armature.cc

						
				@ -89,4 +89,2 @@

				  te->name = curBone->name;

				  te->directdata = curBone;

				  LISTBASE_FOREACH (Bone *, child_bone, &curBone->childbase) {

Julian Eisel commented

2023-07-04 15:01:30 +02:00

Any reason for not doing this in the expand function?

Almaz-Shinbay commented

2023-07-04 15:44:55 +02:00

I didn't find a way how to pass the index, but now I have some idea in mind

Almaz-Shinbay commented

2023-07-05 15:00:30 +02:00

Do you have an idea how to do it?

Julian Eisel commented

2023-07-05 15:16:47 +02:00

Actually, it's not that important to move this to the expand() function, so you can leave it as is now. (I do see solutions for the index but it's not really worth it).

Actually, it's not that important to move this to the `expand()` function, so you can leave it as is now. (I do see solutions for the index but it's not really worth it).

Julian Eisel added the

Module

User Interface

label 2023-07-04 15:02:13 +02:00

Almaz-Shinbay added 2 commits 2023-07-05 09:42:31 +02:00

Merge remote-tracking branch 'origin/main' into temp-bone-refactor 6464dbf87e

Modify TreeElementBone class and its constructor c992f22e25

Almaz-Shinbay commented

2023-07-06 13:40:09 +02:00

Is it better to include TSE_EBONE to this PR or create new PR for it?

Is it better to include `TSE_EBONE` to this PR or create new PR for it?

Julian Eisel reviewed 2023-07-06 18:29:15 +02:00

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

						
				@ -240,2 +240,4 @@

				    id = nullptr;

				  }

				  else if (type == TSE_BONE) {

				    id = ((BoneElementCreateData *)idv)->armature_id;