GPv3: Move channels #111009

Pratik Borhade · 2023-08-10T16:08:10+02:00

Pratik Borhade commented

2023-08-10 16:08:10 +02:00

This will allow to move selected channels to Top/Bottom/Up/Down in the list.
Existing "island" approch can not be used here since layers are now
stored in tree.

This will allow to move selected channels to Top/Bottom/Up/Down in the list. Existing "island" approch can not be used here since layers are now stored in tree.

Pratik Borhade added 1 commit 2023-08-10 16:08:21 +02:00

a7d0778228 GPv3: Move channels

This will allow to move selected channels to Top/Bottom/Up/Down in the list.
Existing "island" approch can not be used here since layers are now
stored in tree.

Pratik Borhade added the

Module

Grease Pencil

label 2023-08-10 16:08:40 +02:00

Pratik Borhade added this to the Grease Pencil project 2023-08-10 16:08:52 +02:00

Pratik Borhade requested review from Falk David 2023-08-10 16:09:04 +02:00

Pratik Borhade requested review from Amélie Fondevilla 2023-08-10 16:09:05 +02:00

Amélie Fondevilla referenced this pull request

2023-08-10 17:52:50 +02:00

GPv3: Integration in the Dopesheets #110056

Amélie Fondevilla commented

2023-08-10 18:04:28 +02:00

Hey, thanks for the patch.

I think there is a problem with how the layer move with layer groups.
Here is an example in the following video, when I'm moving up the layer called "Hair", I would expect it to go outside of its layer group and be placed right after the layer called "Clouds".

Instead, it is moving the layer "Clouds" inside of the layer group, which is weird.

Maybe this would be easier to test once #111015 is merged, because then we would actually see the groups in the dopesheet.

Hey, thanks for the patch. I think there is a problem with how the layer move with layer groups. Here is an example in the following video, when I'm moving up the layer called "Hair", I would expect it to go outside of its layer group and be placed right after the layer called "Clouds". <video src="/attachments/e19f77f4-0e09-45f2-bc34-40a64dad049d" title="rearrange_channels_groups.mp4" controls></video> Instead, it is moving the layer "Clouds" inside of the layer group, which is weird. Maybe this would be easier to test once #111015 is merged, because then we would actually see the groups in the dopesheet.

rearrange_channels_groups.mp4

778 KiB

Pratik Borhade commented

2023-08-11 11:06:47 +02:00

I see, thanks. Will rewrite the logic 😅

Pratik Borhade changed title from ~~GPv3: Move channels~~ to WIP: GPv3: Move channels

2023-08-11 11:06:58 +02:00

Pratik Borhade added 1 commit 2023-08-11 12:30:25 +02:00

653b41afa9 Fix: Channel move up

Falk David commented

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

@PratikPB2123 hey, thanks for working on this. I think there are two things that we need to work on before this can be finalized.

The add_group/layer_before/after functions should be rewritten to something like insert_node_before/after. This is a refactor that I have on my list, but haven't gotten to yet. Moving the selection up and down should also work on groups.
Right now the PR #111015 is still in review, but I think it should be merged before this one.

It might also be worth it to write the move up/down as a core function.

@PratikPB2123 hey, thanks for working on this. I think there are two things that we need to work on before this can be finalized. 1) The `add_group/layer_before/after` functions should be rewritten to something like `insert_node_before/after`. This is a refactor that I have on my list, but haven't gotten to yet. Moving the selection up and down should also work on groups. 2) Right now the PR #111015 is still in review, but I think it should be merged before this one. It might also be worth it to write the move up/down as a core function.

Pratik Borhade commented

2023-08-14 11:43:38 +02:00

Hi, thanks for mentioning this.

functions should be rewritten to something like insert_node_before/after. This is a refactor that I have on my list, but haven't gotten to yet

Can do that myself in a new PR :)

Moving the selection up and down should also work on groups.

So far, this PR moves all "selected" channels correctly in Up/Top/Bottom direction to the correct group. (only "down" is remaining)

Right now the PR #111015 is still in review, but I think it should be merged before this one.

👍

It might also be worth it to write the move up/down as a core function.

In GreasePencil class? or in anim_channels_edit.cc

Hi, thanks for mentioning this. > functions should be rewritten to something like insert_node_before/after. This is a refactor that I have on my list, but haven't gotten to yet Can do that myself in a new PR :) > Moving the selection up and down should also work on groups. So far, this PR moves all "selected" channels correctly in Up/Top/Bottom direction to the correct group. (only "down" is remaining) > Right now the PR #111015 is still in review, but I think it should be merged before this one. 👍 > It might also be worth it to write the move up/down as a core function. In GreasePencil class? or in `anim_channels_edit.cc`

Falk David commented

2023-08-14 12:03:52 +02:00

In GreasePencil class? or in anim_channels_edit.cc

In the GreasePencil struct, yea. It could be a function like so: GreasePencil::move_nodes_up(Span<blender::bke::greasepencil::TreeNode *> nodes)

> In GreasePencil class? or in anim_channels_edit.cc In the `GreasePencil` struct, yea. It could be a function like so: `GreasePencil::move_nodes_up(Span<blender::bke::greasepencil::TreeNode *> nodes)`

👍 1

Pratik Borhade added 1 commit 2023-08-15 07:06:21 +02:00

c4cee27c40 Fix: Move layer down with new struct function

Pratik Borhade added 1 commit 2023-08-15 07:35:17 +02:00

d343e14d5d Cleanup: Create and use move_layer_up

Pratik Borhade commented

2023-08-15 07:37:31 +02:00

@filedescriptor / @amelief hi, all cases are handled now (also created new functions in GreasePencil struct).

Pratik Borhade changed title from ~~WIP: GPv3: Move channels~~ to GPv3: Move channels

2023-08-15 12:03:11 +02:00

Falk David commented

2023-08-18 15:04:30 +02:00

After some more thought, I think it's best if we simplify this a bit more.
For now it's best if we don't change the hierarchy and move layers in and out of their groups. We can add more operators for that later.

Here are my suggested changes (also needs a rebase on main):

diff --git a/source/blender/blenkernel/BKE_grease_pencil.hh b/source/blender/blenkernel/BKE_grease_pencil.hh
index 42e33439b29..428720a95bb 100644
--- a/source/blender/blenkernel/BKE_grease_pencil.hh
+++ b/source/blender/blenkernel/BKE_grease_pencil.hh
@@ -481,6 +481,11 @@ class LayerGroup : public ::GreasePencilLayerTreeGroup {
   Layer &add_layer_after(Layer *layer, TreeNode *link);
   Layer &add_layer_after(StringRefNull name, TreeNode *link);
 
+  void move_node_up(TreeNode *node, int step);
+  void move_node_down(TreeNode *node, int step);
+  void move_node_top(TreeNode *node);
+  void move_node_bottom(TreeNode *node);
+
   /**
    * Returns the number of direct nodes in this group.
    */
diff --git a/source/blender/blenkernel/intern/grease_pencil.cc b/source/blender/blenkernel/intern/grease_pencil.cc
index e96d43032d0..a2741d5c4f1 100644
--- a/source/blender/blenkernel/intern/grease_pencil.cc
+++ b/source/blender/blenkernel/intern/grease_pencil.cc
@@ -900,6 +900,29 @@ Layer &LayerGroup::add_layer_after(StringRefNull name, TreeNode *link)
   return this->add_layer_after(new_layer, link);
 }
 
+void LayerGroup::move_node_up(TreeNode *node, const int step)
+{
+  BLI_listbase_link_move(&this->children, node, step);
+  this->tag_nodes_cache_dirty();
+}
+void LayerGroup::move_node_down(TreeNode *node, const int step)
+{
+  BLI_listbase_link_move(&this->children, node, -step);
+  this->tag_nodes_cache_dirty();
+}
+void LayerGroup::move_node_top(TreeNode *node)
+{
+  BLI_remlink(&this->children, node);
+  BLI_insertlinkafter(&this->children, this->children.last, node);
+  this->tag_nodes_cache_dirty();
+}
+void LayerGroup::move_node_bottom(TreeNode *node)
+{
+  BLI_remlink(&this->children, node);
+  BLI_insertlinkbefore(&this->children, this->children.first, node);
+  this->tag_nodes_cache_dirty();
+}
+
 int64_t LayerGroup::num_direct_nodes() const
 {
   return BLI_listbase_count(&this->children);
diff --git a/source/blender/editors/animation/anim_channels_edit.cc b/source/blender/editors/animation/anim_channels_edit.cc
index 3ef22753dce..89708b6196e 100644
--- a/source/blender/editors/animation/anim_channels_edit.cc
+++ b/source/blender/editors/animation/anim_channels_edit.cc
@@ -1660,57 +1660,38 @@ static void rearrange_grease_pencil_channels(bAnimContext *ac, eRearrangeAnimCha
   ANIM_animdata_filter(
       ac, &anim_data, eAnimFilter_Flags(filter), ac->data, eAnimCont_Types(ac->datatype));
 
-  /* Layer to reorder with. */
-  Layer *temp_layer = nullptr;
-
   LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
-    GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
     Layer *layer = static_cast<Layer *>(ale->data);
 
-    if (layer == grease_pencil->layers().last()) {
-      temp_layer = (mode == REARRANGE_ANIMCHAN_TOP) ? grease_pencil->layers_for_write().last() :
-                                                      nullptr;
-      layer_list.clear();
-    }
-
     switch (mode) {
       case REARRANGE_ANIMCHAN_TOP: {
         if (layer->is_selected()) {
-          grease_pencil->move_layer_up(layer, temp_layer);
+          layer->parent_group().move_node_top(&layer->as_node());
         }
         break;
       }
       case REARRANGE_ANIMCHAN_UP: {
-        /* Store last unselected element in temp_layer and move selected channels above it. */
-        if (!layer->is_selected()) {
-          temp_layer = layer;
-        }
-        else if (temp_layer) {
-          grease_pencil->move_layer_up(layer, temp_layer);
+        if (layer->is_selected()) {
+          layer->parent_group().move_node_up(&layer->as_node(), 1);
         }
         break;
       }
       case REARRANGE_ANIMCHAN_DOWN: {
         if (layer->is_selected()) {
-          layer_list.append(layer);
-          continue;
-        }
-        if (layer_list.size() != 0) {
-          grease_pencil->move_layer_down(layer_list, layer);
-          layer_list.clear();
+          layer->parent_group().move_node_down(&layer->as_node(), 1);
         }
         break;
       }
       case REARRANGE_ANIMCHAN_BOTTOM: {
-        temp_layer = grease_pencil->layers_for_write().first();
-        if (layer->is_selected() && layer != temp_layer) {
-          layer->parent_group().unlink_node(&layer->as_node());
-          temp_layer->parent_group().add_layer_before(layer, &temp_layer->as_node());
+        if (layer->is_selected()) {
+          layer->parent_group().move_node_bottom(&layer->as_node());
         }
         break;
       }
     }
   }
+
+  BLI_freelistN(&anim_data);
 }
 
 static void rearrange_gpencil_channels(bAnimContext *ac, eRearrangeAnimChan_Mode mode)

After some more thought, I think it's best if we simplify this a bit more. For now it's best if we don't change the hierarchy and move layers in and out of their groups. We can add more operators for that later. Here are my suggested changes (also needs a rebase on main): ```diff diff --git a/source/blender/blenkernel/BKE_grease_pencil.hh b/source/blender/blenkernel/BKE_grease_pencil.hh index 42e33439b29..428720a95bb 100644 --- a/source/blender/blenkernel/BKE_grease_pencil.hh +++ b/source/blender/blenkernel/BKE_grease_pencil.hh @@ -481,6 +481,11 @@ class LayerGroup : public ::GreasePencilLayerTreeGroup { Layer &add_layer_after(Layer *layer, TreeNode *link); Layer &add_layer_after(StringRefNull name, TreeNode *link); + void move_node_up(TreeNode *node, int step); + void move_node_down(TreeNode *node, int step); + void move_node_top(TreeNode *node); + void move_node_bottom(TreeNode *node); + /** * Returns the number of direct nodes in this group. */ diff --git a/source/blender/blenkernel/intern/grease_pencil.cc b/source/blender/blenkernel/intern/grease_pencil.cc index e96d43032d0..a2741d5c4f1 100644 --- a/source/blender/blenkernel/intern/grease_pencil.cc +++ b/source/blender/blenkernel/intern/grease_pencil.cc @@ -900,6 +900,29 @@ Layer &LayerGroup::add_layer_after(StringRefNull name, TreeNode *link) return this->add_layer_after(new_layer, link); } +void LayerGroup::move_node_up(TreeNode *node, const int step) +{ + BLI_listbase_link_move(&this->children, node, step); + this->tag_nodes_cache_dirty(); +} +void LayerGroup::move_node_down(TreeNode *node, const int step) +{ + BLI_listbase_link_move(&this->children, node, -step); + this->tag_nodes_cache_dirty(); +} +void LayerGroup::move_node_top(TreeNode *node) +{ + BLI_remlink(&this->children, node); + BLI_insertlinkafter(&this->children, this->children.last, node); + this->tag_nodes_cache_dirty(); +} +void LayerGroup::move_node_bottom(TreeNode *node) +{ + BLI_remlink(&this->children, node); + BLI_insertlinkbefore(&this->children, this->children.first, node); + this->tag_nodes_cache_dirty(); +} + int64_t LayerGroup::num_direct_nodes() const { return BLI_listbase_count(&this->children); diff --git a/source/blender/editors/animation/anim_channels_edit.cc b/source/blender/editors/animation/anim_channels_edit.cc index 3ef22753dce..89708b6196e 100644 --- a/source/blender/editors/animation/anim_channels_edit.cc +++ b/source/blender/editors/animation/anim_channels_edit.cc @@ -1660,57 +1660,38 @@ static void rearrange_grease_pencil_channels(bAnimContext *ac, eRearrangeAnimCha ANIM_animdata_filter( ac, &anim_data, eAnimFilter_Flags(filter), ac->data, eAnimCont_Types(ac->datatype)); - /* Layer to reorder with. */ - Layer *temp_layer = nullptr; - LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) { - GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id); Layer *layer = static_cast<Layer *>(ale->data); - if (layer == grease_pencil->layers().last()) { - temp_layer = (mode == REARRANGE_ANIMCHAN_TOP) ? grease_pencil->layers_for_write().last() : - nullptr; - layer_list.clear(); - } - switch (mode) { case REARRANGE_ANIMCHAN_TOP: { if (layer->is_selected()) { - grease_pencil->move_layer_up(layer, temp_layer); + layer->parent_group().move_node_top(&layer->as_node()); } break; } case REARRANGE_ANIMCHAN_UP: { - /* Store last unselected element in temp_layer and move selected channels above it. */ - if (!layer->is_selected()) { - temp_layer = layer; - } - else if (temp_layer) { - grease_pencil->move_layer_up(layer, temp_layer); + if (layer->is_selected()) { + layer->parent_group().move_node_up(&layer->as_node(), 1); } break; } case REARRANGE_ANIMCHAN_DOWN: { if (layer->is_selected()) { - layer_list.append(layer); - continue; - } - if (layer_list.size() != 0) { - grease_pencil->move_layer_down(layer_list, layer); - layer_list.clear(); + layer->parent_group().move_node_down(&layer->as_node(), 1); } break; } case REARRANGE_ANIMCHAN_BOTTOM: { - temp_layer = grease_pencil->layers_for_write().first(); - if (layer->is_selected() && layer != temp_layer) { - layer->parent_group().unlink_node(&layer->as_node()); - temp_layer->parent_group().add_layer_before(layer, &temp_layer->as_node()); + if (layer->is_selected()) { + layer->parent_group().move_node_bottom(&layer->as_node()); } break; } } } + + BLI_freelistN(&anim_data); } static void rearrange_gpencil_channels(bAnimContext *ac, eRearrangeAnimChan_Mode mode) ```

Pratik Borhade commented

2023-08-23 13:05:24 +02:00

Thanks. All changes are yours now 😅
Will update the PR

Thanks. All changes are yours now 😅 Will update the PR

Falk David commented

2023-09-11 10:46:29 +02:00

Hey @PratikPB2123 Sorry for changing this a bunch... Do you want me to open a new PR for this? Or would you still want to do the changes yourself? Either works for me :)