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
Contributor

This commit implements Copy and Past Keyframes for GPv3. The operation is available in Animation workspace. The copy operator copies the selected GPv3 Keyframes and the paste operator paste the Keyframe from the clipboard. See #113110.

Technical notes
grease_pencil_copy_keyframes() iterates through every selected frame for every channel/layer, then stores layer name, selected frame number and its corresponding drawing in a vector of BufferItem. The buffer is valid even if the source frame is deleted because the drawings are stored. Please note that layer can be moved so storing layer pointer is not viable.
grease_pencil_paste_keyframes() finds the layer copy buffer. If exists, 1. the existing frames might be removed depending on different merge mode, 2. the target frame number for pasting is calculated, 3. the original frames at the target frame number will be removed, 4. the drawings stored in Vector<DrawingBufferItem> will be added and new frames created at the right place.

This commit implements Copy and Past Keyframes for GPv3. The operation is available in Animation workspace. The copy operator copies the selected GPv3 Keyframes and the paste operator paste the Keyframe from the clipboard. See #113110. **Technical notes** `grease_pencil_copy_keyframes()` iterates through every selected frame for every channel/layer, then stores layer name, selected frame number and its corresponding drawing in a vector of `BufferItem`. The buffer is valid even if the source frame is deleted because the drawings are stored. Please note that layer can be moved so storing layer pointer is not viable. `grease_pencil_paste_keyframes()` finds the layer copy buffer. If exists, 1. the existing frames might be removed depending on different merge mode, 2. the target frame number for pasting is calculated, 3. the original frames at the target frame number will be removed, 4. the drawings stored in `Vector<DrawingBufferItem>` will be added and new frames created at the right place.
Chao Li added 1 commit 2024-01-21 21:04:33 +01:00
f384321f04 GPv3: Copy/Paste Keyframes
This commit implements Copy and Past Keyframes for GPv3. The operation
is available in Animation workspace. The copy operator copies the
selected GPv3 Keyframes and the paste operator paste the Keyframe from
the clipboard.

**Technical notes**
`grease_pencil_copy_keyframes()` iterates through every selected frame
for every channel/layer, then stores frame_number and layer name in a
vector of `BufferItem`. Please note that layer can be moved so storing
layer pointer is not viable.
`grease_pencil_paste_keyframes()` iterates through all `BufferItem` and
uses `GreasePencil::insert_duplicate_frame()` to duplicate the keyframe
and drawing. Please note that there's a issue with the function that may
lead to crash, for which I opened #114352.
Pratik Borhade requested changes 2024-01-25 11:31:04 +01:00
Dismissed
Pratik Borhade left a comment
Member

Hi, thanks for the PR. Appears to be working correctly. Found few points on first look :)
I'll add Falk and Amelie as reviewer

Hi, thanks for the PR. Appears to be working correctly. Found few points on first look :) I'll add Falk and Amelie as reviewer
@ -382,0 +408,4 @@
Object *object = ac->obact;
/* filter data */
filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE | ANIMFILTER_NODUPLIS);
Member

you can make this const int filter instead of declaring at start of function.

you can make this `const int filter` instead of declaring at start of function.
Chao-Li marked this conversation as resolved
@ -382,0 +421,4 @@
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
blender::Span<const Layer *> layers = grease_pencil->layers();
for (const Layer *layer : layers) {
Member

ale->data is Layer. So you can remove this for-loop

`ale->data` is Layer. So you can remove this for-loop
Chao-Li marked this conversation as resolved
@ -382,0 +484,4 @@
break;
}
filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE | ANIMFILTER_NODUPLIS);
Member

same as said above :)

same as said above :)
Chao-Li marked this conversation as resolved
Pratik Borhade requested review from Falk David 2024-01-25 11:31:21 +01:00
Pratik Borhade requested review from Amélie Fondevilla 2024-01-25 11:31:22 +01:00
Pratik Borhade added this to the Grease Pencil project 2024-01-25 11:31:27 +01:00
Pratik Borhade added the
Module
Grease Pencil
label 2024-01-25 11:31:37 +01:00
Pratik Borhade reviewed 2024-01-25 11:54:54 +01:00
@ -381,1 +382,4 @@
/* Datatype for use in copy/paste buffer */
struct BufferItem {
Vector<int> frame_numbers;
Member

Looks like storing frame_number will give wrong results (I mean paste operation fails) if source keyframe is moved or deleted
Instead of frame_number, we Might need to store copy of Drawing

Looks like storing `frame_number` will give wrong results (I mean paste operation fails) if source keyframe is moved or deleted Instead of frame_number, we Might need to store copy of `Drawing`
Chao-Li marked this conversation as resolved
@ -382,0 +407,4 @@
Scene *scene = ac->scene;
Object *object = ac->obact;
/* filter data */
Member

Also include ANIMFILTER_FOREDIT flag here so, locked layers will be excluded.

Also include `ANIMFILTER_FOREDIT` flag here so, locked layers will be excluded.
Chao-Li marked this conversation as resolved
Amélie Fondevilla requested changes 2024-01-26 16:36:42 +01:00
Amélie Fondevilla left a comment
Member

Hi,
Thanks for the patch.

So, first thing I noticed is that the paste does not display correctly the pasted drawing in the viewport, and I think it's because we're not tagging the grease pencil object for recalculating geometry (cf the comment I left on the code).

Second, it seems like we are not dealing with the case of pasting a keyframe onto an existing one.
I think the default behavior is to create a mix between the drawings of the copied keyframe and the existing one, but this behavior can change according to the Type/merge parameter of the operator (see action_edit.cc:748-753).

Also, I noted the issue #114352 and I will try to look into it. Unfortunately I've been away from the blender code for some time, so it takes a bit more time for me to review and test things.

Hi, Thanks for the patch. So, first thing I noticed is that the paste does not display correctly the pasted drawing in the viewport, and I think it's because we're not tagging the grease pencil object for recalculating geometry (cf the comment I left on the code). Second, it seems like we are not dealing with the case of pasting a keyframe onto an existing one. I think the default behavior is to create a mix between the drawings of the copied keyframe and the existing one, but this behavior can change according to the `Type`/`merge` parameter of the operator (see `action_edit.cc:748-753`). Also, I noted the issue #114352 and I will try to look into it. Unfortunately I've been away from the blender code for some time, so it takes a bit more time for me to review and test things.
@ -382,0 +498,4 @@
blender::Span<Layer *> layers = grease_pencil->layers_for_write();
for (const BufferItem buffer : copy_buffer) {
for (Layer *layer : layers) {
if (layer->name() == buffer.layer_name) {

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; } ... `
Chao-Li marked this conversation as resolved
@ -382,0 +505,4 @@
}
}
}
}

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.
Member

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

Yes, I also noticed the missing update after pasting keys.
Chao-Li marked this conversation as resolved
Author
Contributor

Hello. Thanks for reviewing! I'll update the PR.

Hello. Thanks for reviewing! I'll update the PR.
Chao Li changed title from GPv3: Copy/Paste Keyframes to WIP: GPv3: Copy/Paste Keyframes 2024-01-27 21:20:13 +01:00
Chao Li added 3 commits 2024-02-03 01:47:43 +01:00
Chao Li added 4 commits 2024-03-24 19:31:53 +01:00
Chao Li added 1 commit 2024-03-24 19:37:48 +01:00
Chao Li added 1 commit 2024-03-24 23:04:53 +01:00
Chao Li added 1 commit 2024-03-24 23:11:49 +01:00
Chao Li changed title from WIP: GPv3: Copy/Paste Keyframes to GPv3: Copy/Paste Keyframes 2024-03-24 23:12:04 +01:00
Chao Li requested review from Pratik Borhade 2024-03-24 23:20:51 +01:00
Chao Li requested review from Amélie Fondevilla 2024-03-24 23:20:54 +01:00
Pratik Borhade reviewed 2024-03-27 12:14:02 +01:00
@ -434,0 +556,4 @@
blender::Span<Layer *> layers = grease_pencil->layers_for_write();
bool change = false;
for (const BufferItem buffer : copy_buffer) {
for (Layer *layer : layers) {
Member

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 🙂
Member

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.
Author
Contributor

Thanks for reviewing! I've made changes accordingly.

Thanks for reviewing! I've made changes accordingly.
Chao-Li marked this conversation as resolved
Chao Li added 2 commits 2024-03-27 16:17:33 +01:00
Author
Contributor

I also noticed that KEYFRAME_PASTE_MERGE_OVER_RANGE mix mode didn't work properly because DrawingBufferItem is not sorted by frame number so I added a LayerBufferItem struct to store the correct range of a layer.

I also noticed that `KEYFRAME_PASTE_MERGE_OVER_RANGE` mix mode didn't work properly because DrawingBufferItem is not sorted by frame number so I added a `LayerBufferItem` struct to store the correct range of a layer.
Chao Li added 1 commit 2024-03-27 16:37:26 +01:00
Chao Li added 1 commit 2024-03-27 19:18:46 +01:00
Chao Li added 1 commit 2024-03-27 19:45:38 +01:00
Chao Li added 1 commit 2024-03-27 20:13:45 +01:00
Chao Li reviewed 2024-03-27 20:15:46 +01:00
@ -434,0 +581,4 @@
/* remove all keys */
Vector<int> frames_to_remove;
for (auto frame_number : layer.frames().keys()) {
frames_to_remove.append(frame_number);
Author
Contributor
          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?
Member

@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
Author
Contributor

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.
Chao-Li marked this conversation as resolved
Chao Li added 1 commit 2024-03-27 20:36:18 +01:00
Pratik Borhade requested changes 2024-03-28 12:43:46 +01:00
Dismissed
Pratik Borhade left a comment
Member

Hi, found few changes

Hi, found few changes
@ -434,0 +564,4 @@
GreasePencil *grease_pencil = reinterpret_cast<GreasePencil *>(ale->id);
bool change = false;
for (auto [layer_name, layer_buffer] : copy_buffer.items()) {
Member

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")`
Chao-Li marked this conversation as resolved
@ -434,0 +616,4 @@
}
}
for (auto drawing_buffer : layer_buffer.drawing_buffers) {
int target_frame_number = drawing_buffer.frame_number + offset;
Member

can be const

can be `const`
Chao-Li marked this conversation as resolved
Chao Li added 3 commits 2024-03-28 16:08:43 +01:00
Pratik Borhade requested changes 2024-03-29 12:45:01 +01:00
Dismissed
Pratik Borhade left a comment
Member

Hi, thanks for updating the PR. Looks almost complete to me apart from one change (I'll ask about this in module channel)

Edit: yes, this is required.

Hi, thanks for updating the PR. Looks almost complete to me apart from one change (I'll ask about this in module channel) Edit: yes, this is required.
@ -434,0 +620,4 @@
if (layer->frames().contains(target_frame_number)) {
layer->remove_frame(target_frame_number);
}
layer->add_frame(target_frame_number, grease_pencil->drawings().size(), 0);
Member

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`
Chao-Li marked this conversation as resolved
Chao Li added 1 commit 2024-03-29 19:29:38 +01:00
Author
Contributor

Thanks again for reviewing! I added the change. However, I don't know how to create a keyframe with duration so I wasn't able to test the scenario. Duplicating an existing keyframe doesn't connect two.

Thanks again for reviewing! I added the change. However, I don't know how to create a keyframe with duration so I wasn't able to test the scenario. Duplicating an existing keyframe doesn't connect two.
Pratik Borhade requested changes 2024-03-30 11:19:42 +01:00
Dismissed
Pratik Borhade left a comment
Member

Thanks. In GPv2 if only one frame is copied in buffer then that single keyframe can be pasted in selected layer.
We could do this similar here as well.

Apart from this, no blocking from my side :)

Thanks. In GPv2 if only one frame is copied in buffer then that single keyframe can be pasted in selected layer. We could do this similar here as well. Apart from this, no blocking from my side :)
Member

I don't know how to create a keyframe with duration so I wasn't able to test the scenari

I went through the existing code, looks like operator/property to add keyframe with certain duration is not implemented yet.

Duplicating an existing keyframe doesn't connect two.

Right, I think that's a bug :)

> I don't know how to create a keyframe with duration so I wasn't able to test the scenari I went through the existing code, looks like operator/property to add keyframe with certain duration is not implemented yet. > Duplicating an existing keyframe doesn't connect two. Right, I think that's a bug :)
Iliya Katushenock reviewed 2024-03-30 11:40:47 +01:00
@ -434,0 +447,4 @@
/* Globals for copy/paste data (like for other copy/paste buffers). */
static Map<std::string, LayerBufferItem> copy_buffer;
static int copy_buffer_first_frame = INT_MAX;

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

`std::numeric_limits<int>::max()`
Chao-Li marked this conversation as resolved
Chao Li added 2 commits 2024-03-31 03:25:49 +02:00
Author
Contributor

In GPv2 if only one frame is copied in buffer then that single keyframe can be pasted in selected layer.

Thanks for pointing this out. I think this applies not only to the case where one frame in buffer? I updated and tested the patch.

> In GPv2 if only one frame is copied in buffer then that single keyframe can be pasted in selected layer. Thanks for pointing this out. I think this applies not only to the case where one frame in buffer? I updated and tested the patch.
Member

Thanks.

I think this applies not only to the case where one frame in buffer? I updated and tested the patch.

Sorry. I didn't explain this correctly. There is one more special case in gpv2 when only "keyframes of one channel" is copied in buffer. In that case you can paste these keyframes to any other channel/layer which is selected: https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/gpencil_legacy/editaction_gpencil.cc#L398-L401

Thanks. > I think this applies not only to the case where one frame in buffer? I updated and tested the patch. Sorry. I didn't explain this correctly. There is one more special case in gpv2 when only "keyframes of one channel" is copied in buffer. In that case you can paste these keyframes to any other channel/layer which is selected: https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/gpencil_legacy/editaction_gpencil.cc#L398-L401
Pratik Borhade reviewed 2024-04-01 13:01:34 +02:00
@ -434,0 +565,4 @@
ac->data,
eAnimCont_Types(ac->datatype)) == 0)
{
ANIM_animdata_filter(
Member

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.
Author
Contributor

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.
Member

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)
Author
Contributor

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.
Chao-Li marked this conversation as resolved
Author
Contributor

There is one more special case in gpv2 when only "keyframes of one channel" is copied in buffer. In that case you can paste these keyframes to any other channel/layer which is selected: https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/gpencil_legacy/editaction_gpencil.cc#L398-L401

Let me check. Thank you.

> There is one more special case in gpv2 when only "keyframes of one channel" is copied in buffer. In that case you can paste these keyframes to any other channel/layer which is selected: https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/gpencil_legacy/editaction_gpencil.cc#L398-L401 Let me check. Thank you.
Chao Li added 2 commits 2024-04-01 22:02:05 +02:00
Chao Li added 1 commit 2024-04-01 22:04:52 +02:00
Author
Contributor

Updated.

Updated.
Pratik Borhade requested changes 2024-04-02 13:27:11 +02:00
Dismissed
Pratik Borhade left a comment
Member

Hi, final few comments.

All other recent changes are correct btw

Hi, final few comments. All other recent changes are correct btw
@ -434,0 +582,4 @@
continue;
}
LayerBufferItem layer_buffer = from_single_channel ? *copy_buffer.values() :
copy_buffer.lookup(layer_name);
Member

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

`*copy_buffer.values().begin()`. Just values() did not work for me
Chao-Li marked this conversation as resolved
@ -10,6 +10,7 @@
#include <cmath>
#include <cstdlib>
#include <cstring>
#include <stdio.h>
Member

this is not required :)

this is not required :)
Chao-Li marked this conversation as resolved
Chao Li added 3 commits 2024-04-02 17:03:36 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
2b499de5c7
only paste to selected channels
Pratik Borhade approved these changes 2024-04-03 06:02:21 +02:00
Pratik Borhade left a comment
Member

Thanks. All good from my side 🙂
let's wait for Falk/Amélie now.

Thanks. All good from my side 🙂 let's wait for Falk/Amélie now.
Member

@blender-bot build

@blender-bot build
Falk David requested changes 2024-04-08 14:29:27 +02:00
Dismissed
Falk David left a comment
Member

Did a pass on this. Looks ok overall.

Did a pass on this. Looks ok overall.
@ -434,0 +445,4 @@
blender::bke::greasepencil::FramesMapKey last_frame;
};
/* Globals for copy/paste data (like for other copy/paste buffers). */
Member

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

Would be a bit better if these were grouped in a `struct`.
Chao-Li marked this conversation as resolved
@ -434,0 +537,4 @@
int offset = 0;
/* Methods of offset (eKeyPasteOffset). */
switch (offset_mode) {
Member

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 = ...;`.
Chao-Li marked this conversation as resolved
@ -434,0 +621,4 @@
break;
}
}
for (auto drawing_buffer : layer_buffer.drawing_buffers) {
Member

Would prefer DrawingBufferItem item here instead of auto drawing_buffer

Would prefer `DrawingBufferItem item` here instead of `auto drawing_buffer`
Chao-Li marked this conversation as resolved
@ -434,0 +624,4 @@
for (auto 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);
Member

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.
Chao Li added 4 commits 2024-04-09 03:06:17 +02:00
Lukas Tönne requested changes 2024-04-09 17:18:35 +02:00
Dismissed
@ -434,0 +446,4 @@
};
struct Clipboard {
Map<std::string, LayerBufferItem> copy_buffer;
Member

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.
Author
Contributor

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.
Member

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>
Author
Contributor

Thank you for helping me debug the issue!

Thank you for helping me debug the issue!
Chao-Li marked this conversation as resolved
Chao Li added 1 commit 2024-04-09 19:03:33 +02:00
Lukas Tönne reviewed 2024-04-10 11:43:32 +02:00
@ -434,0 +569,4 @@
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);
Member

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.
Author
Contributor

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
Member

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
Member

Ok, fair enough.

Ok, fair enough.
LukasTonne marked this conversation as resolved
Chao Li added 1 commit 2024-04-12 03:57:10 +02:00
Lukas Tönne approved these changes 2024-04-18 10:10:43 +02:00
Falk David approved these changes 2024-04-22 12:04:05 +02:00
Falk David merged commit 74b9c6a8a0 into main 2024-04-22 12:06:33 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117388
No description provided.