GPv3: Copy/Paste Keyframes #117388

Merged
Falk David merged 36 commits from Chao-Li/blender:113110 into main 2024-04-22 12:06:34 +02:00
3 changed files with 250 additions and 4 deletions

View File

@ -21,6 +21,7 @@
#include "ANIM_keyframing.hh"
#include "ED_anim_api.hh"
#include "ED_grease_pencil.hh"
#include "ED_keyframes_edit.hh"
#include "ED_markers.hh"
@ -431,6 +432,200 @@ static void GREASE_PENCIL_OT_insert_blank_frame(wmOperatorType *ot)
RNA_def_int(ot->srna, "duration", 0, 0, MAXFRAME, "Duration", "", 0, 100);
}
bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard)
{
using namespace bke::greasepencil;
/* Clear buffer first. */
clipboard.clear();
/* Filter data. */
const int filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE | ANIMFILTER_NODUPLIS);
ListBase anim_data = {nullptr, nullptr};
ANIM_animdata_filter(
ac, &anim_data, eAnimFilter_Flags(filter), ac->data, eAnimCont_Types(ac->datatype));
Chao-Li marked this conversation as resolved Outdated

Would be a bit better if these were grouped in a struct.

Would be a bit better if these were grouped in a `struct`.
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
Chao-Li marked this conversation as resolved Outdated

I get crashes quite easily in debug mode, looks like the copy_buffer ends up with uninitialized memory.

I get crashes quite easily in debug mode, looks like the `copy_buffer` ends up with uninitialized memory.

Sorry I couldn't produce the error on my machine. But the added commit should resolve the issue. Please let me know.

Sorry I couldn't produce the error on my machine. But the added commit should resolve the issue. Please let me know.

I was wrong, it's not simply initialization: the static clipboard variable must be defined in the same file it's used, i.e. in action_edit.cc. I tried giving the copy/paste functions an explicit Clipboard &clipboard argument and move the static variable to action_edit.cc, which seems to solve the issue.

show diff
diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc
index 9fad77e621f..b7d7b209c16 100644
--- a/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc
+++ b/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc
@@ -432,46 +432,10 @@ static void GREASE_PENCIL_OT_insert_blank_frame(wmOperatorType *ot)
   RNA_def_int(ot->srna, "duration", 0, 0, MAXFRAME, "Duration", "", 0, 100);
 }
 
-/* Datatype for use in copy/paste buffer. */
-struct DrawingBufferItem {
-  blender::bke::greasepencil::FramesMapKey frame_number;
-  bke::greasepencil::Drawing drawing;
-  int duration;
-};
-
-struct LayerBufferItem {
-  Vector<DrawingBufferItem> drawing_buffers;
-  blender::bke::greasepencil::FramesMapKey first_frame;
-  blender::bke::greasepencil::FramesMapKey last_frame;
-};
-
-struct Clipboard {
-  Map<std::string, LayerBufferItem> copy_buffer{};
-  int first_frame{std::numeric_limits<int>::max()};
-  int last_frame{std::numeric_limits<int>::min()};
-  int cfra{0};
-
-  void clear()
-  {
-    copy_buffer.clear();
-    first_frame = std::numeric_limits<int>::max();
-    last_frame = std::numeric_limits<int>::min();
-    cfra = 0;
-  }
-};
-
-static Clipboard &get_grease_pencil_keyframe_clipboard()
-{
-  static Clipboard clipboard;
-  return clipboard;
-}
-
-bool grease_pencil_copy_keyframes(bAnimContext *ac)
+bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard)
 {
   using namespace bke::greasepencil;
 
-  Clipboard &clipboard = get_grease_pencil_keyframe_clipboard();
-
   /* Clear buffer first. */
   clipboard.clear();
 
@@ -491,7 +455,7 @@ bool grease_pencil_copy_keyframes(bAnimContext *ac)
 
     GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
     Layer *layer = reinterpret_cast<Layer *>(ale->data);
-    Vector<DrawingBufferItem> buf;
+    Vector<KeyframeClipboard::DrawingBufferItem> buf;
     FramesMapKey layer_first_frame = std::numeric_limits<int>::max();
     FramesMapKey layer_last_frame = std::numeric_limits<int>::min();
     for (auto [frame_number, frame] : layer->frames().items()) {
@@ -533,7 +497,9 @@ bool grease_pencil_copy_keyframes(bAnimContext *ac)
   return !clipboard.copy_buffer.is_empty();
 }
 
-int calculate_offset(const eKeyPasteOffset offset_mode, const int cfra, const Clipboard &clipboard)
+static int calculate_offset(const eKeyPasteOffset offset_mode,
+                            const int cfra,
+                            const KeyframeClipboard &clipboard)
 {
   int offset = 0;
   switch (offset_mode) {
@@ -555,12 +521,11 @@ int calculate_offset(const eKeyPasteOffset offset_mode, const int cfra, const Cl
 
 bool grease_pencil_paste_keyframes(bAnimContext *ac,
                                    const eKeyPasteOffset offset_mode,
-                                   const eKeyMergeMode merge_mode)
+                                   const eKeyMergeMode merge_mode,
+                                   const KeyframeClipboard &clipboard)
 {
   using namespace bke::greasepencil;
 
-  const Clipboard &clipboard = get_grease_pencil_keyframe_clipboard();
-
   /* Check if buffer is empty. */
   if (clipboard.copy_buffer.is_empty()) {
     return false;
@@ -589,8 +554,9 @@ bool grease_pencil_paste_keyframes(bAnimContext *ac,
     if (!from_single_channel && !clipboard.copy_buffer.contains(layer_name)) {
       continue;
     }
-    LayerBufferItem layer_buffer = from_single_channel ? *clipboard.copy_buffer.values().begin() :
-                                                         clipboard.copy_buffer.lookup(layer_name);
+    KeyframeClipboard::LayerBufferItem layer_buffer = from_single_channel ?
+                                                          *clipboard.copy_buffer.values().begin() :
+                                                          clipboard.copy_buffer.lookup(layer_name);
     bool change = false;
 
     /* Mix mode with existing data. */
@@ -638,7 +604,7 @@ bool grease_pencil_paste_keyframes(bAnimContext *ac,
         break;
       }
     }
-    for (DrawingBufferItem drawing_buffer : layer_buffer.drawing_buffers) {
+    for (KeyframeClipboard::DrawingBufferItem drawing_buffer : layer_buffer.drawing_buffers) {
       const int target_frame_number = drawing_buffer.frame_number + offset;
       if (layer->frames().contains(target_frame_number)) {
         layer->remove_frame(target_frame_number);
diff --git a/source/blender/editors/include/ED_grease_pencil.hh b/source/blender/editors/include/ED_grease_pencil.hh
index f7518131d75..06a3a5fd954 100644
--- a/source/blender/editors/include/ED_grease_pencil.hh
+++ b/source/blender/editors/include/ED_grease_pencil.hh
@@ -131,11 +131,40 @@ bool remove_all_selected_frames(GreasePencil &grease_pencil, bke::greasepencil::
 
 void select_layer_channel(GreasePencil &grease_pencil, bke::greasepencil::Layer *layer);
 
-bool grease_pencil_copy_keyframes(bAnimContext *ac);
+struct KeyframeClipboard {
+  /* Datatype for use in copy/paste buffer. */
+  struct DrawingBufferItem {
+    blender::bke::greasepencil::FramesMapKey frame_number;
+    bke::greasepencil::Drawing drawing;
+    int duration;
+  };
+
+  struct LayerBufferItem {
+    Vector<DrawingBufferItem> drawing_buffers;
+    blender::bke::greasepencil::FramesMapKey first_frame;
+    blender::bke::greasepencil::FramesMapKey last_frame;
+  };
+
+  Map<std::string, LayerBufferItem> copy_buffer;
+  int first_frame = std::numeric_limits<int>::max();
+  int last_frame = std::numeric_limits<int>::min();
+  int cfra{0};
+
+  void clear()
+  {
+    copy_buffer.clear();
+    first_frame = std::numeric_limits<int>::max();
+    last_frame = std::numeric_limits<int>::min();
+    cfra = 0;
+  }
+};
+
+bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard);
 
 bool grease_pencil_paste_keyframes(bAnimContext *ac,
                                    const eKeyPasteOffset offset_mode,
-                                   const eKeyMergeMode merge_mode);
+                                   const eKeyMergeMode merge_mode,
+                                   const KeyframeClipboard &clipboard);
 
 /**
  * Sets the selection flag, according to \a selection_mode to the frame at \a frame_number in the
diff --git a/source/blender/editors/space_action/action_edit.cc b/source/blender/editors/space_action/action_edit.cc
index 1c23b280da6..2892c953f83 100644
--- a/source/blender/editors/space_action/action_edit.cc
+++ b/source/blender/editors/space_action/action_edit.cc
@@ -576,6 +576,12 @@ static eKeyPasteError paste_action_keys(bAnimContext *ac,
 
 /* ------------------- */
 
+static blender::ed::greasepencil::KeyframeClipboard &get_grease_pencil_keyframe_clipboard()
+{
+  static blender::ed::greasepencil::KeyframeClipboard clipboard;
+  return clipboard;
+}
+
 static int actkeys_copy_exec(bContext *C, wmOperator *op)
 {
   bAnimContext ac;
@@ -588,7 +594,8 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op)
   /* copy keyframes */
   if (ac.datatype == ANIMCONT_GPENCIL) {
     if (ED_gpencil_anim_copybuf_copy(&ac) == false &&
-        blender::ed::greasepencil::grease_pencil_copy_keyframes(&ac) == false)
+        blender::ed::greasepencil::grease_pencil_copy_keyframes(
+            &ac, get_grease_pencil_keyframe_clipboard()) == false)
     {
       /* check if anything ended up in the buffer */
       BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard");
@@ -604,7 +611,8 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op)
     /* Both copy function needs to be evaluated to account for mixed selection */
     const short kf_empty = copy_action_keys(&ac);
     const bool gpf_ok = ED_gpencil_anim_copybuf_copy(&ac) ||
-                        blender::ed::greasepencil::grease_pencil_copy_keyframes(&ac);
+                        blender::ed::greasepencil::grease_pencil_copy_keyframes(
+                            &ac, get_grease_pencil_keyframe_clipboard());
 
     if (kf_empty && !gpf_ok) {
       BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard");
@@ -651,8 +659,8 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op)
   /* paste keyframes */
   if (ac.datatype == ANIMCONT_GPENCIL) {
     if (ED_gpencil_anim_copybuf_paste(&ac, offset_mode) == false &&
-        blender::ed::greasepencil::grease_pencil_paste_keyframes(&ac, offset_mode, merge_mode) ==
-            false)
+        blender::ed::greasepencil::grease_pencil_paste_keyframes(
+            &ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard()) == false)
     {
       BKE_report(op->reports, RPT_ERROR, "No data in the internal clipboard to paste");
       return OPERATOR_CANCELLED;
@@ -671,7 +679,7 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op)
     /* non-zero return means an error occurred while trying to paste */
     gpframes_inbuf = ED_gpencil_anim_copybuf_paste(&ac, offset_mode) ||
                      blender::ed::greasepencil::grease_pencil_paste_keyframes(
-                         &ac, offset_mode, merge_mode);
+                         &ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard());
 
     /* Only report an error if nothing was pasted, i.e. when both FCurve and GPencil failed. */
     if (!gpframes_inbuf) {
I was wrong, it's not simply initialization: the static clipboard variable must be defined in the same file it's used, i.e. in `action_edit.cc`. I tried giving the copy/paste functions an explicit `Clipboard &clipboard` argument and move the static variable to `action_edit.cc`, which seems to solve the issue. <details> <summary>show diff</summary> ```diff diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc index 9fad77e621f..b7d7b209c16 100644 --- a/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc +++ b/source/blender/editors/grease_pencil/intern/grease_pencil_frames.cc @@ -432,46 +432,10 @@ static void GREASE_PENCIL_OT_insert_blank_frame(wmOperatorType *ot) RNA_def_int(ot->srna, "duration", 0, 0, MAXFRAME, "Duration", "", 0, 100); } -/* Datatype for use in copy/paste buffer. */ -struct DrawingBufferItem { - blender::bke::greasepencil::FramesMapKey frame_number; - bke::greasepencil::Drawing drawing; - int duration; -}; - -struct LayerBufferItem { - Vector<DrawingBufferItem> drawing_buffers; - blender::bke::greasepencil::FramesMapKey first_frame; - blender::bke::greasepencil::FramesMapKey last_frame; -}; - -struct Clipboard { - Map<std::string, LayerBufferItem> copy_buffer{}; - int first_frame{std::numeric_limits<int>::max()}; - int last_frame{std::numeric_limits<int>::min()}; - int cfra{0}; - - void clear() - { - copy_buffer.clear(); - first_frame = std::numeric_limits<int>::max(); - last_frame = std::numeric_limits<int>::min(); - cfra = 0; - } -}; - -static Clipboard &get_grease_pencil_keyframe_clipboard() -{ - static Clipboard clipboard; - return clipboard; -} - -bool grease_pencil_copy_keyframes(bAnimContext *ac) +bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard) { using namespace bke::greasepencil; - Clipboard &clipboard = get_grease_pencil_keyframe_clipboard(); - /* Clear buffer first. */ clipboard.clear(); @@ -491,7 +455,7 @@ bool grease_pencil_copy_keyframes(bAnimContext *ac) GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id); Layer *layer = reinterpret_cast<Layer *>(ale->data); - Vector<DrawingBufferItem> buf; + Vector<KeyframeClipboard::DrawingBufferItem> buf; FramesMapKey layer_first_frame = std::numeric_limits<int>::max(); FramesMapKey layer_last_frame = std::numeric_limits<int>::min(); for (auto [frame_number, frame] : layer->frames().items()) { @@ -533,7 +497,9 @@ bool grease_pencil_copy_keyframes(bAnimContext *ac) return !clipboard.copy_buffer.is_empty(); } -int calculate_offset(const eKeyPasteOffset offset_mode, const int cfra, const Clipboard &clipboard) +static int calculate_offset(const eKeyPasteOffset offset_mode, + const int cfra, + const KeyframeClipboard &clipboard) { int offset = 0; switch (offset_mode) { @@ -555,12 +521,11 @@ int calculate_offset(const eKeyPasteOffset offset_mode, const int cfra, const Cl bool grease_pencil_paste_keyframes(bAnimContext *ac, const eKeyPasteOffset offset_mode, - const eKeyMergeMode merge_mode) + const eKeyMergeMode merge_mode, + const KeyframeClipboard &clipboard) { using namespace bke::greasepencil; - const Clipboard &clipboard = get_grease_pencil_keyframe_clipboard(); - /* Check if buffer is empty. */ if (clipboard.copy_buffer.is_empty()) { return false; @@ -589,8 +554,9 @@ bool grease_pencil_paste_keyframes(bAnimContext *ac, if (!from_single_channel && !clipboard.copy_buffer.contains(layer_name)) { continue; } - LayerBufferItem layer_buffer = from_single_channel ? *clipboard.copy_buffer.values().begin() : - clipboard.copy_buffer.lookup(layer_name); + KeyframeClipboard::LayerBufferItem layer_buffer = from_single_channel ? + *clipboard.copy_buffer.values().begin() : + clipboard.copy_buffer.lookup(layer_name); bool change = false; /* Mix mode with existing data. */ @@ -638,7 +604,7 @@ bool grease_pencil_paste_keyframes(bAnimContext *ac, break; } } - for (DrawingBufferItem drawing_buffer : layer_buffer.drawing_buffers) { + for (KeyframeClipboard::DrawingBufferItem drawing_buffer : layer_buffer.drawing_buffers) { const int target_frame_number = drawing_buffer.frame_number + offset; if (layer->frames().contains(target_frame_number)) { layer->remove_frame(target_frame_number); diff --git a/source/blender/editors/include/ED_grease_pencil.hh b/source/blender/editors/include/ED_grease_pencil.hh index f7518131d75..06a3a5fd954 100644 --- a/source/blender/editors/include/ED_grease_pencil.hh +++ b/source/blender/editors/include/ED_grease_pencil.hh @@ -131,11 +131,40 @@ bool remove_all_selected_frames(GreasePencil &grease_pencil, bke::greasepencil:: void select_layer_channel(GreasePencil &grease_pencil, bke::greasepencil::Layer *layer); -bool grease_pencil_copy_keyframes(bAnimContext *ac); +struct KeyframeClipboard { + /* Datatype for use in copy/paste buffer. */ + struct DrawingBufferItem { + blender::bke::greasepencil::FramesMapKey frame_number; + bke::greasepencil::Drawing drawing; + int duration; + }; + + struct LayerBufferItem { + Vector<DrawingBufferItem> drawing_buffers; + blender::bke::greasepencil::FramesMapKey first_frame; + blender::bke::greasepencil::FramesMapKey last_frame; + }; + + Map<std::string, LayerBufferItem> copy_buffer; + int first_frame = std::numeric_limits<int>::max(); + int last_frame = std::numeric_limits<int>::min(); + int cfra{0}; + + void clear() + { + copy_buffer.clear(); + first_frame = std::numeric_limits<int>::max(); + last_frame = std::numeric_limits<int>::min(); + cfra = 0; + } +}; + +bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard); bool grease_pencil_paste_keyframes(bAnimContext *ac, const eKeyPasteOffset offset_mode, - const eKeyMergeMode merge_mode); + const eKeyMergeMode merge_mode, + const KeyframeClipboard &clipboard); /** * Sets the selection flag, according to \a selection_mode to the frame at \a frame_number in the diff --git a/source/blender/editors/space_action/action_edit.cc b/source/blender/editors/space_action/action_edit.cc index 1c23b280da6..2892c953f83 100644 --- a/source/blender/editors/space_action/action_edit.cc +++ b/source/blender/editors/space_action/action_edit.cc @@ -576,6 +576,12 @@ static eKeyPasteError paste_action_keys(bAnimContext *ac, /* ------------------- */ +static blender::ed::greasepencil::KeyframeClipboard &get_grease_pencil_keyframe_clipboard() +{ + static blender::ed::greasepencil::KeyframeClipboard clipboard; + return clipboard; +} + static int actkeys_copy_exec(bContext *C, wmOperator *op) { bAnimContext ac; @@ -588,7 +594,8 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op) /* copy keyframes */ if (ac.datatype == ANIMCONT_GPENCIL) { if (ED_gpencil_anim_copybuf_copy(&ac) == false && - blender::ed::greasepencil::grease_pencil_copy_keyframes(&ac) == false) + blender::ed::greasepencil::grease_pencil_copy_keyframes( + &ac, get_grease_pencil_keyframe_clipboard()) == false) { /* check if anything ended up in the buffer */ BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard"); @@ -604,7 +611,8 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op) /* Both copy function needs to be evaluated to account for mixed selection */ const short kf_empty = copy_action_keys(&ac); const bool gpf_ok = ED_gpencil_anim_copybuf_copy(&ac) || - blender::ed::greasepencil::grease_pencil_copy_keyframes(&ac); + blender::ed::greasepencil::grease_pencil_copy_keyframes( + &ac, get_grease_pencil_keyframe_clipboard()); if (kf_empty && !gpf_ok) { BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard"); @@ -651,8 +659,8 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op) /* paste keyframes */ if (ac.datatype == ANIMCONT_GPENCIL) { if (ED_gpencil_anim_copybuf_paste(&ac, offset_mode) == false && - blender::ed::greasepencil::grease_pencil_paste_keyframes(&ac, offset_mode, merge_mode) == - false) + blender::ed::greasepencil::grease_pencil_paste_keyframes( + &ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard()) == false) { BKE_report(op->reports, RPT_ERROR, "No data in the internal clipboard to paste"); return OPERATOR_CANCELLED; @@ -671,7 +679,7 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op) /* non-zero return means an error occurred while trying to paste */ gpframes_inbuf = ED_gpencil_anim_copybuf_paste(&ac, offset_mode) || blender::ed::greasepencil::grease_pencil_paste_keyframes( - &ac, offset_mode, merge_mode); + &ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard()); /* Only report an error if nothing was pasted, i.e. when both FCurve and GPencil failed. */ if (!gpframes_inbuf) { ``` </details>

Thank you for helping me debug the issue!

Thank you for helping me debug the issue!
/* This function only deals with grease pencil layer frames.
Chao-Li marked this conversation as resolved Outdated

std::numeric_limits<int>::max()

`std::numeric_limits<int>::max()`
* This check is needed in the case of a call from the main dopesheet. */
if (ale->type != ANIMTYPE_GREASE_PENCIL_LAYER) {
continue;
}
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
Layer *layer = reinterpret_cast<Layer *>(ale->data);
Vector<KeyframeClipboard::DrawingBufferItem> buf;
FramesMapKey layer_first_frame = std::numeric_limits<int>::max();
FramesMapKey layer_last_frame = std::numeric_limits<int>::min();
for (auto [frame_number, frame] : layer->frames().items()) {
if (frame.is_selected()) {
const Drawing *drawing = grease_pencil->get_drawing_at(*layer, frame_number);
const int duration = frame.is_implicit_hold() ? 0 :
layer->get_frame_duration_at(frame_number);
buf.append({frame_number, Drawing(*drawing), duration});
/* Check the range of this layer only. */
if (frame_number < layer_first_frame) {
layer_first_frame = frame_number;
}
if (frame_number > layer_last_frame) {
layer_last_frame = frame_number;
}
}
}
if (!buf.is_empty()) {
BLI_assert(!clipboard.copy_buffer.contains(layer->name()));
clipboard.copy_buffer.add_new(layer->name(), {buf, layer_first_frame, layer_last_frame});
/* Update the range of entire copy buffer. */
if (layer_first_frame < clipboard.first_frame) {
clipboard.first_frame = layer_first_frame;
}
if (layer_last_frame > clipboard.last_frame) {
clipboard.last_frame = layer_last_frame;
}
}
Chao-Li marked this conversation as resolved Outdated

same as said above :)

same as said above :)
}
/* In case 'relative' paste method is used. */
clipboard.cfra = ac->scene->r.cfra;
/* Clean up. */
ANIM_animdata_freelist(&anim_data);
/* If nothing ended up in the buffer, copy failed. */
return !clipboard.copy_buffer.is_empty();
}
static int calculate_offset(const eKeyPasteOffset offset_mode,
const int cfra,
Chao-Li marked this conversation as resolved Outdated

Here it may be better to go with something like :
if (layer->name() != buffer.layer_name) { continue; } ...

Here it may be better to go with something like : `if (layer->name() != buffer.layer_name) { continue; } ... `
const KeyframeClipboard &clipboard)
{
int offset = 0;
switch (offset_mode) {
case KEYFRAME_PASTE_OFFSET_CFRA_START:
offset = (cfra - clipboard.first_frame);
break;
Chao-Li marked this conversation as resolved Outdated

I think we need to recalculate the grease pencil geometry after this loop. You can do so by calling
DEG_id_tag_update(&grease_pencil->id, ID_RECALC_GEOMETRY)

We should also probably add a change flag to only recalculate the geometry if the insertion actually occurred.

I think we need to recalculate the grease pencil geometry after this loop. You can do so by calling `DEG_id_tag_update(&grease_pencil->id, ID_RECALC_GEOMETRY)` We should also probably add a `change` flag to only recalculate the geometry if the insertion actually occurred.

Yes, I also noticed the missing update after pasting keys.

Yes, I also noticed the missing update after pasting keys.
case KEYFRAME_PASTE_OFFSET_CFRA_END:
offset = (cfra - clipboard.last_frame);
break;
case KEYFRAME_PASTE_OFFSET_CFRA_RELATIVE:
offset = (cfra - clipboard.cfra);
break;
case KEYFRAME_PASTE_OFFSET_NONE:
offset = 0;
break;
}
return offset;
}
bool grease_pencil_paste_keyframes(bAnimContext *ac,
const eKeyPasteOffset offset_mode,
const eKeyMergeMode merge_mode,
const KeyframeClipboard &clipboard)
{
using namespace bke::greasepencil;
/* Check if buffer is empty. */
if (clipboard.copy_buffer.is_empty()) {
return false;
}
const int offset = calculate_offset(offset_mode, ac->scene->r.cfra, clipboard);
const int filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE | ANIMFILTER_NODUPLIS |
ANIMFILTER_FOREDIT | ANIMFILTER_SEL);
ListBase anim_data = {nullptr, nullptr};
ANIM_animdata_filter(
Chao-Li marked this conversation as resolved Outdated

Bit of a nitpick, but I would but this into a function. And then retrieve the offset directly: const int offset = ...;.

Bit of a nitpick, but I would but this into a function. And then retrieve the offset directly: `const int offset = ...;`.
ac, &anim_data, eAnimFilter_Flags(filter), ac->data, eAnimCont_Types(ac->datatype));
/* Check if single channel in buffer (disregard names if so). */
const bool from_single_channel = clipboard.copy_buffer.size() == 1;
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
/* Only deal with GPlayers (case of calls from general dopesheet). */
if (ale->type != ANIMTYPE_GREASE_PENCIL_LAYER) {
continue;
}
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
Layer *layer = reinterpret_cast<Layer *>(ale->data);
const std::string layer_name = layer->name();
if (!from_single_channel && !clipboard.copy_buffer.contains(layer_name)) {
continue;
}
KeyframeClipboard::LayerBufferItem layer_buffer = from_single_channel ?
*clipboard.copy_buffer.values().begin() :
clipboard.copy_buffer.lookup(layer_name);
Chao-Li marked this conversation as resolved Outdated

Hi, now PR looks mostly good to me. I'm thinking how to simplify this further. Too many loops 🙂

Hi, now PR looks mostly good to me. I'm thinking how to simplify this further. Too many loops 🙂

Instead of BufferItem , you can create key-pair: Map<string, Vector<DrawingBufferItem>>

So you will be able to remove this "layer" loop.

Instead of BufferItem , you can create key-pair: `Map<string, Vector<DrawingBufferItem>>` So you will be able to remove this "layer" loop.

Thanks for reviewing! I've made changes accordingly.

Thanks for reviewing! I've made changes accordingly.
bool change = false;
/* Mix mode with existing data. */
switch (merge_mode) {
case KEYFRAME_PASTE_MERGE_MIX:
/* Do nothing. */
break;
Chao-Li marked this conversation as resolved Outdated

I don't think we require this loop anymore. We can get layer/layer name from ale->data. Then get keyframe buffers of that layer: copy_buffer.lookup("layer name of current layer")

I don't think we require this loop anymore. We can get layer/layer name from `ale->data`. Then get keyframe buffers of that layer: `copy_buffer.lookup("layer name of current layer")`
case KEYFRAME_PASTE_MERGE_OVER: {
Chao-Li marked this conversation as resolved Outdated

only keep the selected channels.
If no channels are selected, we can simply skip the paste operation like gpv2.

only keep the selected channels. If no channels are selected, we can simply skip the paste operation like gpv2.

Hello. I can't upload video here so I uploaded them in the comment below. This change here ensures the same behavior with gpv2. Please check.

Hello. I can't upload video here so I uploaded them in the comment below. This change here ensures the same behavior with gpv2. Please check.

I mean when no channel is selected, simply don't paste any keys. To elaborate further, revert the changes done in a6befa9108 and include ANIMFILTER_SEL flag in filters
Same as GPv2 editaction_gpencil.cc#L421-L424

I mean when no channel is selected, simply don't paste any keys. To elaborate further, revert the changes done in a6befa9108d6546938230f535b28a33af61a65ec and include `ANIMFILTER_SEL` flag in filters Same as GPv2 [editaction_gpencil.cc#L421-L424](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/gpencil_legacy/editaction_gpencil.cc#L421-L424)

Got it. I made the changes in that commit because I was looking at behaviors of other keyframe pasting, dropsheet, action edit etc. But it makes sense that we want the behavior the same as GPv2.

Got it. I made the changes in that commit because I was looking at behaviors of other keyframe pasting, dropsheet, action edit etc. But it makes sense that we want the behavior the same as GPv2.
/* Remove all keys. */
Vector<int> frames_to_remove;
for (auto frame_number : layer->frames().keys()) {
frames_to_remove.append(frame_number);
LukasTonne marked this conversation as resolved Outdated

The ANIMFILTER_SEL probably shouldn't be here. It makes it so keyframes cannot be pasted when the channel view in the dopesheet is collapsed.

The `ANIMFILTER_SEL` probably shouldn't be here. It makes it so keyframes cannot be pasted when the channel view in the dopesheet is collapsed.

There was a comment by @PratikPB2123. This is the same behavior as gpv2.

After that this is added to the filter: 2b499de5c7

There was a comment by @PratikPB2123. This is the same behavior as gpv2. After that this is added to the filter: 2b499de5c7

Right. This is how GPv2 is working ¯_(ツ)_/¯

See: #117388 (comment)

Right. This is how GPv2 is working ¯\_(ツ)_/¯ See: https://projects.blender.org/blender/blender/pulls/117388#issuecomment-1158831

Ok, fair enough.

Ok, fair enough.
}
grease_pencil->remove_frames(*layer, frames_to_remove);
change = true;
break;
}
case KEYFRAME_PASTE_MERGE_OVER_RANGE:
case KEYFRAME_PASTE_MERGE_OVER_RANGE_ALL: {
int frame_min, frame_max;
if (merge_mode == KEYFRAME_PASTE_MERGE_OVER_RANGE) {
/* Entire range of this layer. */
frame_min = layer_buffer.first_frame + offset;
Chao-Li marked this conversation as resolved Outdated
          grease_pencil->remove_frames(layer, layer.sorted_keys());

is not working properly so I had to copy out all the keys. The Span<FramesMapKey> returned by sorted_keys() might be invalidated/changed when the frames are removed?

``` grease_pencil->remove_frames(layer, layer.sorted_keys()); ``` is not working properly so I had to copy out all the keys. The `Span<FramesMapKey>` returned by `sorted_keys()` might be invalidated/changed when the frames are removed?

@filedescriptor ^
Not sure but I saw remove_frames() not working in one more PR :/

Also GPv2 don't handle any "merge_mode" so we can also skip this for GPv3. Let's wait for Falk's reply

@filedescriptor ^ Not sure but I saw `remove_frames()` not working in one more PR :/ Also GPv2 don't handle any "merge_mode" so we can also skip this for GPv3. Let's wait for Falk's reply

The merge mode is working now. It's just that the keys need to be copied out when "overwrite all". Maybe there can be an API to remove all frames? It's not blocking this feature.

The merge mode is working now. It's just that the keys need to be copied out when "overwrite all". Maybe there can be an API to remove all frames? It's not blocking this feature.
frame_max = layer_buffer.last_frame + offset;
Chao-Li marked this conversation as resolved Outdated

*copy_buffer.values().begin().
Just values() did not work for me

`*copy_buffer.values().begin()`. Just values() did not work for me
}
else {
/* Entire range of all copied keys. */
frame_min = clipboard.first_frame + offset;
frame_max = clipboard.last_frame + offset;
}
/* Remove keys in range. */
if (frame_min < frame_max) {
Vector<int> frames_to_remove;
for (auto frame_number : layer->frames().keys()) {
if (frame_min < frame_number && frame_number < frame_max) {
frames_to_remove.append(frame_number);
}
}
grease_pencil->remove_frames(*layer, frames_to_remove);
change = true;
}
break;
}
}
for (KeyframeClipboard::DrawingBufferItem drawing_buffer : layer_buffer.drawing_buffers) {
const int target_frame_number = drawing_buffer.frame_number + offset;
if (layer->frames().contains(target_frame_number)) {
layer->remove_frame(target_frame_number);
}
layer->add_frame(
target_frame_number, grease_pencil->drawings().size(), drawing_buffer.duration);
grease_pencil->add_duplicate_drawings(1, drawing_buffer.drawing);
change = true;
}
if (change) {
DEG_id_tag_update(&grease_pencil->id, ID_RECALC_GEOMETRY);
Chao-Li marked this conversation as resolved Outdated

can be const

can be `const`
}
}
/* Clean up. */
Chao-Li marked this conversation as resolved Outdated

instead of 0, we need to consider the is_implicit_hold() here
So when copying frame, we would need to do:

        const int duration = frame.is_implicit_hold() ? 0 :
                                                        layer->get_frame_duration_at(frame_number);

and also include a new member duration in DrawingBufferItem

instead of 0, we need to consider the `is_implicit_hold()` here So when copying frame, we would need to do: ``` const int duration = frame.is_implicit_hold() ? 0 : layer->get_frame_duration_at(frame_number); ``` and also include a new member `duration` in `DrawingBufferItem`
ANIM_animdata_freelist(&anim_data);
Chao-Li marked this conversation as resolved Outdated

Would prefer DrawingBufferItem item here instead of auto drawing_buffer

Would prefer `DrawingBufferItem item` here instead of `auto drawing_buffer`
return true;
}

Just a note that this API will be replaced at some point. The reason being that adding or removing keyframes affects the GreasePencil data, so the API shouldn't be on the Layer since it leads to bugs like the drawing user counts being wrong. This is on my TODO list and I will refactor this too.

Just a note that this API will be replaced at some point. The reason being that adding or removing keyframes affects the `GreasePencil` data, so the API shouldn't be on the `Layer` since it leads to bugs like the drawing user counts being wrong. This is on my TODO list and I will refactor this too.
} // namespace blender::ed::greasepencil
void ED_operatortypes_grease_pencil_frames()

View File

@ -131,6 +131,41 @@ bool remove_all_selected_frames(GreasePencil &grease_pencil, bke::greasepencil::
void select_layer_channel(GreasePencil &grease_pencil, bke::greasepencil::Layer *layer);
struct KeyframeClipboard {
/* Datatype for use in copy/paste buffer. */
struct DrawingBufferItem {
blender::bke::greasepencil::FramesMapKey frame_number;
bke::greasepencil::Drawing drawing;
int duration;
};
struct LayerBufferItem {
Vector<DrawingBufferItem> drawing_buffers;
blender::bke::greasepencil::FramesMapKey first_frame;
blender::bke::greasepencil::FramesMapKey last_frame;
};
Map<std::string, LayerBufferItem> copy_buffer{};
int first_frame{std::numeric_limits<int>::max()};
int last_frame{std::numeric_limits<int>::min()};
int cfra{0};
void clear()
{
copy_buffer.clear();
first_frame = std::numeric_limits<int>::max();
last_frame = std::numeric_limits<int>::min();
cfra = 0;
}
};
bool grease_pencil_copy_keyframes(bAnimContext *ac, KeyframeClipboard &clipboard);
bool grease_pencil_paste_keyframes(bAnimContext *ac,
const eKeyPasteOffset offset_mode,
const eKeyMergeMode merge_mode,
const KeyframeClipboard &clipboard);
/**
* Sets the selection flag, according to \a selection_mode to the frame at \a frame_number in the
* \a layer if such frame exists. Returns false if no such frame exists.

View File

@ -576,6 +576,12 @@ static eKeyPasteError paste_action_keys(bAnimContext *ac,
/* ------------------- */
static blender::ed::greasepencil::KeyframeClipboard &get_grease_pencil_keyframe_clipboard()
{
static blender::ed::greasepencil::KeyframeClipboard clipboard;
return clipboard;
}
static int actkeys_copy_exec(bContext *C, wmOperator *op)
{
bAnimContext ac;
@ -587,7 +593,10 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op)
/* copy keyframes */
if (ac.datatype == ANIMCONT_GPENCIL) {
if (ED_gpencil_anim_copybuf_copy(&ac) == false) {
if (ED_gpencil_anim_copybuf_copy(&ac) == false &&
blender::ed::greasepencil::grease_pencil_copy_keyframes(
&ac, get_grease_pencil_keyframe_clipboard()) == false)
{
/* check if anything ended up in the buffer */
BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard");
return OPERATOR_CANCELLED;
@ -601,7 +610,9 @@ static int actkeys_copy_exec(bContext *C, wmOperator *op)
else {
/* Both copy function needs to be evaluated to account for mixed selection */
const short kf_empty = copy_action_keys(&ac);
const bool gpf_ok = ED_gpencil_anim_copybuf_copy(&ac);
const bool gpf_ok = ED_gpencil_anim_copybuf_copy(&ac) ||
blender::ed::greasepencil::grease_pencil_copy_keyframes(
&ac, get_grease_pencil_keyframe_clipboard());
if (kf_empty && !gpf_ok) {
BKE_report(op->reports, RPT_ERROR, "No keyframes copied to the internal clipboard");
@ -647,7 +658,10 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op)
/* paste keyframes */
if (ac.datatype == ANIMCONT_GPENCIL) {
if (ED_gpencil_anim_copybuf_paste(&ac, offset_mode) == false) {
if (ED_gpencil_anim_copybuf_paste(&ac, offset_mode) == false &&
blender::ed::greasepencil::grease_pencil_paste_keyframes(
&ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard()) == false)
{
BKE_report(op->reports, RPT_ERROR, "No data in the internal clipboard to paste");
return OPERATOR_CANCELLED;
}
@ -663,7 +677,9 @@ static int actkeys_paste_exec(bContext *C, wmOperator *op)
/* Both paste function needs to be evaluated to account for mixed selection */
const eKeyPasteError kf_empty = paste_action_keys(&ac, offset_mode, merge_mode, flipped);
/* non-zero return means an error occurred while trying to paste */
gpframes_inbuf = ED_gpencil_anim_copybuf_paste(&ac, offset_mode);
gpframes_inbuf = ED_gpencil_anim_copybuf_paste(&ac, offset_mode) ||
blender::ed::greasepencil::grease_pencil_paste_keyframes(
&ac, offset_mode, merge_mode, get_grease_pencil_keyframe_clipboard());
/* Only report an error if nothing was pasted, i.e. when both FCurve and GPencil failed. */
if (!gpframes_inbuf) {