GPv3: Outliner support #111105

Falk David · 2023-08-14T10:46:42+02:00

Falk David commented

2023-08-14 10:46:42 +02:00

This PR adds support for displaying the content of the GPv3 object in the outliner.

This PR adds support for displaying the content of the GPv3 object in the outliner. ![image](/attachments/0f125e01-e98e-4e7c-be19-fb5ff1d66302)

image.png

44 KiB

Falk David added this to the Grease Pencil project 2023-08-14 10:47:01 +02:00

Falk David requested review from Julian Eisel 2023-08-14 10:47:25 +02:00

Falk David force-pushed gpv3-outliner-support from 05e95fad66 to fe6f8e81ce

2023-08-14 10:48:52 +02:00

Compare

Falk David commented

2023-08-14 10:49:00 +02:00

@blender-bot build

Julian Eisel requested changes 2023-08-14 11:25:59 +02:00

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

						
				@ -0,0 +22,4 @@

				{

				  BLI_assert(legacy_te.store_elem->type == TSE_GREASE_PENCIL_NODE);

				  legacy_te.name = node.name;

				  legacy_te.directdata = &node;

Julian Eisel commented

2023-08-14 10:59:32 +02:00

Let's avoid usages of directdata. All necessary data can be stored directly within TreeElementGreasePencilNode. Code that needs access to it can use tree_element_cast() to get this from the TreeElement, and then use accessors to type-safely get the data.

Let's avoid usages of `directdata`. All necessary data can be stored directly within `TreeElementGreasePencilNode`. Code that needs access to it can use `tree_element_cast()` to get this from the `TreeElement`, and then use accessors to type-safely get the data.

filedescriptor marked this conversation as resolved

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

						
				@ -0,0 +37,4 @@

				                         &legacy_te_,

				                         TSE_GREASE_PENCIL_NODE,

				                         0,

				                         child->wrap().is_group());

Julian Eisel commented

2023-08-14 11:06:52 +02:00

I don't see a reason to use the expand option here. It exists because sometimes we just want to display some objects or collections, without expanding their entire hierarchy. It's mostly a performance workaround. Here there just isn't anything to expand, so the expand() function won't add elements anyway.

I don't see a reason to use the `expand` option here. It exists because sometimes we just want to display some objects or collections, without expanding their entire hierarchy. It's mostly a performance workaround. Here there just isn't anything to expand, so the `expand()` function won't add elements anyway.

Falk David commented

2023-08-14 11:57:02 +02:00

I tried setting this to false but then I cannot expand groups with a single child. They show up as if they had no child at all.

I tried setting this to `false` but then I cannot expand groups with a single child. They show up as if they had no child at all.

Julian Eisel commented

2023-08-14 12:08:44 +02:00

You don't need to set it at all, it has a default parameter value of true.

You don't need to set it at all, it has a default parameter value of `true`.

Falk David commented

2023-08-14 12:12:25 +02:00

Ah that explains it, ok.

filedescriptor marked this conversation as resolved

source/blender/makesdna/DNA_outliner_types.h Outdated

						
				@ -117,6 +117,7 @@ typedef enum eTreeStoreElemType {

				  TSE_LIBRARY_OVERRIDE = 45,

				  TSE_LIBRARY_OVERRIDE_OPERATION = 46,

				  TSE_GENERIC_LABEL = 47, /* No ID */

				  TSE_GREASE_PENCIL_NODE = 48, /* No ID */

Julian Eisel commented

2023-08-14 11:03:27 +02:00

The comment says this is not an ID, but TreeStoreElem.id actually points to a valid ID. Checking above, it seems like the ID is needed, so the comment can be removed. Otherwise (if it wouldn't actually point to an ID), TSE_IS_REAL_ID() would have to be updated.

The comment says this is not an ID, but `TreeStoreElem.id` actually points to a valid ID. Checking above, it seems like the ID is needed, so the comment can be removed. Otherwise (if it wouldn't actually point to an ID), `TSE_IS_REAL_ID()` would have to be updated.

filedescriptor marked this conversation as resolved

Falk David requested review from Julian Eisel 2023-08-14 12:00:51 +02:00

Falk David commented

2023-08-14 12:05:50 +02:00

@JulianEisel Seems like the windows bot is failing, but I don't understand why.

Falk David commented

2023-08-14 12:14:41 +02:00

I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them.

@pablovazquez what do you think?

I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them. @pablovazquez what do you think?

Julian Eisel requested changes 2023-08-14 12:17:02 +02:00

Julian Eisel left a comment

@JulianEisel Seems like the windows bot is failing, but I don't understand why.

Noted the likely fix inline.

> @JulianEisel Seems like the windows bot is failing, but I don't understand why. Noted the likely fix inline.

source/blender/editors/space_outliner/tree/tree_element_grease_pencil_node.hh Outdated

						
				@ -0,0 +11,4 @@

				#include "tree_element.hh"

				namespace blender::bke::greasepencil {

				struct TreeNode;

Julian Eisel commented

2023-08-14 12:16:35 +02:00

Should be class, otherwise MSVC won't build.

Should be `class`, otherwise MSVC won't build.

filedescriptor marked this conversation as resolved

source/blender/makesdna/DNA_outliner_types.h Outdated

						
				@ -133,6 +134,7 @@ typedef enum eTreeStoreElemType {

				         TSE_RNA_ARRAY_ELEM, \

				         TSE_ID_BASE, \

				         TSE_GP_LAYER, \

				         TSE_GREASE_PENCIL_NODE, \

Julian Eisel commented

2023-08-14 12:15:13 +02:00

Nope, this is still pointing to an ID, so this shouldn't be included here :) (Note the negation: #define TSE_IS_REAL_ID(_tse) (!ELEM(...)))

Nope, this is still pointing to an ID, so this shouldn't be included here :) (Note the negation: `#define TSE_IS_REAL_ID(_tse) (!ELEM(...))`)

filedescriptor marked this conversation as resolved

Julian Eisel commented

2023-08-14 12:20:54 +02:00

I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them.

I actually don't think we should use the folder icon like that. A folder is a metaphor used for file hierarchies on disk, we are not dealing with files here. Does this need an icon even? Or we just use the regular layer icon, I still wonder if we should even expose layers and layer groups as separate concepts to the user. As far as they are concerned a layer may simply support nesting.

> I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them. I actually don't think we should use the folder icon like that. A folder is a metaphor used for file hierarchies on disk, we are not dealing with files here. Does this need an icon even? Or we just use the regular layer icon, I still wonder if we should even expose layers and layer groups as separate concepts to the user. As far as they are concerned a layer may simply support nesting.

Falk David commented

2023-08-14 12:26:46 +02:00

As far as they are concerned a layer may simply support nesting.

The difference is that a layer group cannot be drawn on, they don't store any stroke or animation data. They are simply a way to organize layers together.

So, to me, it wouldn't make sense to have them use the same icon as layers.

> As far as they are concerned a layer may simply support nesting. The difference is that a layer group cannot be drawn on, they don't store any stroke or animation data. They are simply a way to organize layers together. So, to me, it wouldn't make sense to have them use the same icon as layers.

Falk David commented

2023-08-14 12:28:58 +02:00

@blender-bot build

hamza.SMA commented

2023-08-14 12:44:45 +02:00

First-time contributor

I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them.

@pablovazquez what do you think?

IIRC, wasn't planned to support coloring groups, so i anticipate that it will be reflected in the outliner ?

> I'm also wondering about the current use of the folder icon. I think it fits well but the yellowish color in the outliner doesn't make much sense. It should be light grey like the collections. We also plan to add color tags to them. > > @pablovazquez what do you think? IIRC, wasn't planned to support coloring groups, so i anticipate that it will be reflected in the outliner ?

Falk David commented

2023-08-14 12:46:59 +02:00

We also plan to add color tags to them.

@hamza-el-barmaki That's what I said here, yes.

> We also plan to add color tags to them. @hamza-el-barmaki That's what I said here, yes.

👍 1

Julian Eisel reviewed 2023-08-14 12:49:07 +02:00

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

						
				@ -0,0 +40,4 @@

				                         &legacy_te_,

				                         TSE_GREASE_PENCIL_NODE,

				                         0,

				                         child->wrap().is_group());

Julian Eisel commented

2023-08-14 12:48:24 +02:00

This should still be unnecessary.

filedescriptor marked this conversation as resolved

Falk David force-pushed gpv3-outliner-support from ee40b144c9 to 006f44859c

2023-08-14 12:58:19 +02:00

Compare

Falk David requested review from Julian Eisel 2023-08-14 13:43:19 +02:00

Falk David commented

2023-08-14 14:24:59 +02:00

@JulianEisel Also let me know if the changes I've made to deal with showing the number of layers in a layer group or in the object-data (using MergedIconRow) are fine. The code is a bit messy there so I'm not too sure of this.

@JulianEisel Also let me know if the changes I've made to deal with showing the number of layers in a layer group or in the object-data (using `MergedIconRow`) are fine. The code is a bit messy there so I'm not too sure of this.

Julian Eisel commented

2023-08-14 15:11:26 +02:00

@JulianEisel Also let me know if the changes I've made to deal with showing the number of layers in a layer group or in the object-data (using MergedIconRow) are fine. The code is a bit messy there so I'm not too sure of this.

I wouldn't bother much with it, this code needs refactoring anyway. The recent changes/refactors in the Outliner code should help quite a bit.

> @JulianEisel Also let me know if the changes I've made to deal with showing the number of layers in a layer group or in the object-data (using `MergedIconRow`) are fine. The code is a bit messy there so I'm not too sure of this. I wouldn't bother much with it, this code needs refactoring anyway. The recent changes/refactors in the Outliner code should help quite a bit.

Julian Eisel approved these changes 2023-08-14 15:12:16 +02:00

Julian Eisel left a comment

Accepting, but I still have objections against the folder icon. Maybe commit without this for now?

Falk David commented

2023-08-14 15:19:33 +02:00

@JulianEisel For the folder icon: https://devtalk.blender.org/t/ui-discussion-grease-pencil-layer-and-layer-group-icons/30693
Would it be okay to commit as is and wait for the devtalk thread to resolve the questions about the icons? Then create another PR that fixes the icons properly?

@JulianEisel For the folder icon: https://devtalk.blender.org/t/ui-discussion-grease-pencil-layer-and-layer-group-icons/30693 Would it be okay to commit as is and wait for the devtalk thread to resolve the questions about the icons? Then create another PR that fixes the icons properly?

Julian Eisel commented

2023-08-14 15:29:27 +02:00

Problem is that such temporary icons often end up in a release and being not so temporary anymore. I would rather remove it entirely for now.

Falk David commented

2023-08-14 15:36:41 +02:00

@JulianEisel I would have to do that in the properties panel and dopesheet as well then. I agree that this should not end up in a release, but since GPv3 is experimental, that is unlikely to happen. It's only really there for testing purposes.
But I can create a PR removing it from everywhere if this is really needed.

@JulianEisel I would have to do that in the properties panel and dopesheet as well then. I agree that this should not end up in a release, but since GPv3 is experimental, that is unlikely to happen. It's only really there for testing purposes. But I can create a PR removing it from everywhere if this is really needed.

Julian Eisel approved these changes 2023-08-14 15:39:39 +02:00

Julian Eisel left a comment

Okay, given that this icon is used in other places already I'm fine with keeping it.

👍 1

Falk David referenced this issue from a commit

2023-08-14 17:40:27 +02:00

GPv3: Initial outliner support

Falk David merged commit 310ccbdc33 into main

2023-08-14 17:40:27 +02:00

Falk David deleted branch gpv3-outliner-support

2023-08-14 17:40:29 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Outliner support #111105