GPv3: Layergroup color tag #122873

Pratik Borhade · 2024-06-07T12:36:08+02:00

Pratik Borhade commented

2024-06-07 12:36:08 +02:00

Reused the collection color theme option for the layer group.
Introduced new operator change the icon color it can be accessed from
layer group context menu.

Part of #121390

Reused the collection color theme option for the layer group. Introduced new operator change the icon color it can be accessed from layer group context menu. Part of #121390 <video src="/attachments/2cce335f-f334-4fa5-800a-0e46b93ad27f" title="gpv3-layergroup-colortag.mp4" controls></video>

gpv3-layergroup-colortag.mp4

9.8 MiB

Pratik Borhade added 1 commit 2024-06-07 12:36:18 +02:00

GPv3: Layergroup color tag b9a91b4c73

Part of #121390

Nika Kutsniashvili commented

2024-06-07 12:44:02 +02:00

Is that just changing color? I see layers don't have custom channel color property itself.
I strongly advise using these collection colors in Dope Sheet instead of them. Managing channel colors per-layer is absolute pain. (or at least add ability to override with collection color)

Is that just changing color? I see layers don't have custom channel color property itself. I strongly advise using these collection colors in Dope Sheet instead of them. Managing channel colors per-layer is absolute pain. (or at least add ability to override with collection color)

Pratik Borhade added 1 commit 2024-06-07 12:48:00 +02:00

Cleanup c0273c013b

Pratik Borhade referenced this pull request

2024-06-07 12:49:18 +02:00

GPv3: Layer Group operations #121390

Pratik Borhade commented

2024-06-07 12:52:19 +02:00

@nickberckley hi, not sure what you're expecting here 🙂
As discussed with Falk in #121390 , idea was to change group icon color just like "collection icon in outliner"

I strongly advise using these collection colors in Dope Sheet instead of them

why? 🤔

@nickberckley hi, not sure what you're expecting here 🙂 As discussed with Falk in #121390 , idea was to change group icon color just like "collection icon in outliner" > I strongly advise using these collection colors in Dope Sheet instead of them why? 🤔

Pratik Borhade added this to the Grease Pencil project 2024-06-07 12:57:50 +02:00

Pratik Borhade requested review from Falk David 2024-06-07 12:57:57 +02:00

Nika Kutsniashvili commented

2024-06-07 12:58:33 +02:00

When you have multiple layers and want to change color for all of them its a hustle. Collection-centric workflow like rest of Blender would be much more convimient

Falk David commented

2024-06-07 13:45:51 +02:00

@nickberckley Layers don't have color tags. This is just for layer groups.

Nika Kutsniashvili commented

2024-06-07 13:47:02 +02:00

Yes what I'm proposing is using this color tags for dope sheet channel colors as well

Falk David commented

2024-06-07 13:50:10 +02:00

I see, yea that would make sense. I don't think this PR needs to change all of that though. For now, just introducing a way to set a color for a layer group is fine. There are more ideas on where to use those colors, but we'll have to discuss this with the module.

👍 1

Falk David requested changes 2024-06-07 14:07:33 +02:00

Dismissed

Falk David left a comment

Did a quick pass on the code. Still need to test this locally.

source/blender/blenkernel/BKE_grease_pencil.hh Outdated

						
				@ -612,6 +612,7 @@ class LayerGroup : public ::GreasePencilLayerTreeGroup {

				  LayerGroup &operator=(const LayerGroup &other);

				 public:

				  int color_icon;

Falk David commented

2024-06-07 14:06:21 +02:00

These classes can't store data, they can only contain functions.

You need to add this to DNA:

diff --git a/source/blender/makesdna/DNA_grease_pencil_types.h b/source/blender/makesdna/DNA_grease_pencil_types.h
index ea7083a52dd..7492d58023c 100644
--- a/source/blender/makesdna/DNA_grease_pencil_types.h
+++ b/source/blender/makesdna/DNA_grease_pencil_types.h
@@ -331,6 +331,11 @@ typedef struct GreasePencilLayerTreeGroup {
    * List of `GreasePencilLayerTreeNode`.
    */
   ListBase children;
+  /**
+   * Icon color tag.
+   */
+  int8_t color_tag;
+  char _pad[7];
   /**
    * Runtime struct pointer.
    */

(I should add some static asserts to the wrapper classes of the DNA structs to ensure this doesn't compile)

These classes can't store data, they can only contain functions. You need to add this to DNA: ```diff diff --git a/source/blender/makesdna/DNA_grease_pencil_types.h b/source/blender/makesdna/DNA_grease_pencil_types.h index ea7083a52dd..7492d58023c 100644 --- a/source/blender/makesdna/DNA_grease_pencil_types.h +++ b/source/blender/makesdna/DNA_grease_pencil_types.h @@ -331,6 +331,11 @@ typedef struct GreasePencilLayerTreeGroup { * List of `GreasePencilLayerTreeNode`. */ ListBase children; + /** + * Icon color tag. + */ + int8_t color_tag; + char _pad[7]; /** * Runtime struct pointer. */ ``` (I should add some static asserts to the wrapper classes of the DNA structs to ensure this doesn't compile)

Falk David commented

2024-06-07 14:25:05 +02:00

I should add some static asserts to the wrapper classes of the DNA structs to ensure this doesn't compile

Done. e65981b999

> I should add some static asserts to the wrapper classes of the DNA structs to ensure this doesn't compile Done. https://projects.blender.org/blender/blender/commit/e65981b999b63c3db5e6796b00aaf02d111110cd

Pratik Borhade commented

2024-06-08 12:50:06 +02:00

Thanks, will update.
Actually, I added new member to struct but wasn't compiling after that. Didn't know _pad is required there 😅

Thanks, will update. Actually, I added new member to struct but wasn't compiling after that. Didn't know `_pad` is required there 😅

Pratik Borhade added 1 commit 2024-06-08 12:59:52 +02:00

Store color tag and move it to dna struct 4b07217e03

Pratik Borhade added 1 commit 2024-06-08 13:02:50 +02:00

Cleanup: format 0d63ffd302

Falk David requested changes 2024-06-11 11:48:02 +02:00

Dismissed

source/blender/editors/grease_pencil/intern/grease_pencil_layers.cc Outdated

						
				@ -259,6 +260,7 @@ static int grease_pencil_layer_group_add_exec(bContext *C, wmOperator *op)

				  }();

				  LayerGroup &new_group = grease_pencil.add_layer_group(parent_group, new_layer_group_name);

				  new_group.color_tag = -1;

Falk David commented

2024-06-11 11:46:08 +02:00

This can be moved into LayerGroup::LayerGroup() I think since it should be the starting value always. Also use LAYERGROUP_COLOR_NONE.

LayerGroup::LayerGroup()
{
  ...
  BLI_listbase_clear(&this->children);
  this->color_tag = LAYERGROUP_COLOR_NONE;

This can be moved into `LayerGroup::LayerGroup()` I think since it should be the starting value always. Also use `LAYERGROUP_COLOR_NONE`. ``` LayerGroup::LayerGroup() { ... BLI_listbase_clear(&this->children); this->color_tag = LAYERGROUP_COLOR_NONE; ```

source/blender/editors/interface/templates/interface_template_grease_pencil_layer_tree.cc Outdated

						
				@ -386,1 +386,3 @@

				    uiBut *but = uiItemL_ex(&row, group_.name().c_str(), ICON_FILE_FOLDER, false, false);

				    short icon = ICON_FILE_FOLDER;

				    if (group_.color_tag != -1) {