GPv3: Copy/Paste Keyframes #117388
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#117388
Loading…
Reference in New Issue
No description provided.
Delete Branch "Chao-Li/blender:113110"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 ofBufferItem
. 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 inVector<DrawingBufferItem>
will be added and new frames created at the right place.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);
you can make this
const int filter
instead of declaring at start of function.@ -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) {
ale->data
is Layer. So you can remove this for-loop@ -382,0 +484,4 @@
break;
}
filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_LIST_VISIBLE | ANIMFILTER_NODUPLIS);
same as said above :)
@ -381,1 +382,4 @@
/* Datatype for use in copy/paste buffer */
struct BufferItem {
Vector<int> frame_numbers;
Looks like storing
frame_number
will give wrong results (I mean paste operation fails) if source keyframe is moved or deletedInstead of frame_number, we Might need to store copy of
Drawing
@ -382,0 +407,4 @@
Scene *scene = ac->scene;
Object *object = ac->obact;
/* filter data */
Also include
ANIMFILTER_FOREDIT
flag here so, locked layers will be excluded.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 (seeaction_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; } ...
@ -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.Yes, I also noticed the missing update after pasting keys.
Hello. Thanks for reviewing! I'll update the PR.
GPv3: Copy/Paste Keyframesto WIP: GPv3: Copy/Paste KeyframesWIP: GPv3: Copy/Paste Keyframesto GPv3: Copy/Paste Keyframes@ -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) {
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.
Thanks for reviewing! I've made changes accordingly.
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 aLayerBufferItem
struct to store the correct range of a layer.@ -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);
is not working properly so I had to copy out all the keys. The
Span<FramesMapKey>
returned bysorted_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
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.
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()) {
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")
@ -434,0 +616,4 @@
}
}
for (auto drawing_buffer : layer_buffer.drawing_buffers) {
int target_frame_number = drawing_buffer.frame_number + offset;
can be
const
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);
instead of 0, we need to consider the
is_implicit_hold()
hereSo when copying frame, we would need to do:
and also include a new member
duration
inDrawingBufferItem
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. 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 :)
I went through the existing code, looks like operator/property to add keyframe with certain duration is not implemented yet.
Right, I think that's a bug :)
@ -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()
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.
Thanks.
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
@ -434,0 +565,4 @@
ac->data,
eAnimCont_Types(ac->datatype)) == 0)
{
ANIM_animdata_filter(
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.
I mean when no channel is selected, simply don't paste any keys. To elaborate further, revert the changes done in
a6befa9108
and includeANIMFILTER_SEL
flag in filtersSame as GPv2 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.
Let me check. Thank you.
Updated.
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);
*copy_buffer.values().begin()
.Just values() did not work for me
@ -10,6 +10,7 @@
#include <cmath>
#include <cstdlib>
#include <cstring>
#include <stdio.h>
this is not required :)
Thanks. All good from my side 🙂
let's wait for Falk/Amélie now.
@blender-bot build
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). */
Would be a bit better if these were grouped in a
struct
.@ -434,0 +537,4 @@
int offset = 0;
/* Methods of offset (eKeyPasteOffset). */
switch (offset_mode) {
Bit of a nitpick, but I would but this into a function. And then retrieve the offset directly:
const int offset = ...;
.@ -434,0 +621,4 @@
break;
}
}
for (auto drawing_buffer : layer_buffer.drawing_buffers) {
Would prefer
DrawingBufferItem item
here instead ofauto drawing_buffer
@ -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);
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 theLayer
since it leads to bugs like the drawing user counts being wrong. This is on my TODO list and I will refactor this too.@ -434,0 +446,4 @@
};
struct Clipboard {
Map<std::string, LayerBufferItem> copy_buffer;
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.
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 explicitClipboard &clipboard
argument and move the static variable toaction_edit.cc
, which seems to solve the issue.show diff
Thank you for helping me debug the issue!
@ -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);
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
Right. This is how GPv2 is working ¯_(ツ)_/¯
See: #117388 (comment)
Ok, fair enough.