GPv3: Duplicate keyframes #111051

Amélie Fondevilla · 2023-08-11T17:09:17+02:00

2023-08-11 17:09:17 +02:00

Implementation of the duplicate and move operator for grease pencil frames.

The ACTION_OT_duplicate operator now creates duplicates of the selected frames and stores them in the layer runtime data.

The TRANSFORM_OT_transform operator is updated to take care of these duplicated frames.

Implementation of the duplicate and move operator for grease pencil frames. The `ACTION_OT_duplicate` operator now creates duplicates of the selected frames and stores them in the layer runtime data. The `TRANSFORM_OT_transform` operator is updated to take care of these duplicated frames.

Amélie Fondevilla added this to the Grease Pencil project 2023-08-11 17:09:47 +02:00

Amélie Fondevilla requested review from Falk David 2023-08-11 17:09:53 +02:00

Amélie Fondevilla referenced this pull request

2023-08-11 17:10:15 +02:00

GPv3: Integration in the Dopesheets #110056

Falk David requested changes 2023-08-14 11:37:32 +02:00

Falk David left a comment

Some comments on the code. And one thing I would change for now is that the channel should not use the is_editable() function of the layer (only check is_locked())so that we can move keyframes of layers that are hidden. Maybe this can be a separate small PR that we merge before this one.

Some comments on the code. And one thing I would change for now is that the channel should not use the `is_editable()` function of the layer (only check `is_locked()`)so that we can move keyframes of layers that are hidden. Maybe this can be a separate small PR that we merge before this one.

source/blender/blenkernel/BKE_grease_pencil.hh Outdated

						
				@ -209,6 +209,8 @@ struct LayerTransformData {

				   * i.e. each frame that is not an implicit hold. */

				  Map<int, int> frames_duration;

				  Map<int, GreasePencilFrame> duplicated_frames;

Falk David commented

2023-08-14 11:21:51 +02:00

This should have a comment explaining for what it is used.
Also maybe we could just name it temp_frames_buffer since it's probably going to be used for the copy/paste as well?

This should have a comment explaining for what it is used. Also maybe we could just name it `temp_frames_buffer` since it's probably going to be used for the copy/paste as well?

amelief marked this conversation as resolved

source/blender/editors/transform/transform_convert_action.cc Outdated

						
				@ -227,0 +255,4 @@

				                                      const char side,

				                                      const float cfra,

				                                      const bool is_prop_edit,

				                                      const bool duplicate)

Falk David commented

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

Maybe const bool use_duplicated ?

Maybe `const bool use_duplicated` ?

amelief marked this conversation as resolved

source/blender/editors/transform/transform_convert_action.cc Outdated

						
				@ -507,6 +559,8 @@ static void createTransActionData(bContext *C, TransInfo *t)

				  TransData2D *td2d = nullptr;

				  tGPFtransdata *tfd = nullptr;

				  const bool duplicate = (t->flag & T_AUTOMERGE) != 0;

Falk David commented

2023-08-14 11:30:19 +02:00

I would definitely add a comment here explaining why this flag is used to transform duplicated keyframes.
Also same comment as above.

I would definitely add a comment here explaining why this flag is used to transform duplicated keyframes. Also same comment as above.

amelief marked this conversation as resolved

Amélie Fondevilla force-pushed gpv3-duplicate-keyframe from 1af8b1e537 to edd4f4013e

2023-08-16 12:35:59 +02:00

Compare

Falk David approved these changes 2023-08-16 14:14:54 +02:00

Falk David left a comment

LGTM

Falk David commented

2023-08-16 16:34:29 +02:00

@dr.sybren There is one question regrading this PR that I have for you.

The transform code has an T_AUTOMERGE option. In the context of transforming keyframes, this option is used to deduplicate keys that exist on the same scene frame. It turns out that ACTION_OT_duplicate will duplicate keyframes "in-place" meaning you can have an FCurve with handles that perfectly overlap in the same spot.
The T_AUTOMERGE option is used to indicate that ACTION_OT_duplicate was called before (e.g. in the operator macro for duplicate_move) so that overlapping keys are merged.

For grease pencil, this makes little to no sense. There can only ever be one keyframe at each point in time. For duplicate and duplicate_move to work, we write the duplicated keys into a runtime buffer and then use the T_AUTOMERGE flag to indicate that we really want to transform the keys that are in this buffer.

My question: Should we add a new option to the transform code? Or should we just reuse the option as it is now?

@dr.sybren There is one question regrading this PR that I have for you. The transform code has an `T_AUTOMERGE` option. In the context of transforming keyframes, this option is used to deduplicate keys that exist on the same scene frame. It turns out that `ACTION_OT_duplicate` will duplicate keyframes "in-place" meaning you can have an FCurve with handles that perfectly overlap in the same spot. The `T_AUTOMERGE` option is used to indicate that `ACTION_OT_duplicate` was called before (e.g. in the operator macro for `duplicate_move`) so that overlapping keys are merged. For grease pencil, this makes little to no sense. There can only ever be one keyframe at each point in time. For `duplicate` and `duplicate_move` to work, we write the duplicated keys into a runtime buffer and then use the `T_AUTOMERGE` flag to indicate that we really want to transform the keys that are in this buffer. My question: Should we add a new option to the transform code? Or should we just reuse the option as it is now?

Falk David referenced this pull request

2023-08-18 15:39:12 +02:00

Cleanup: Anim: Replace use of `T_AUTOMERGE` #111268

Falk David referenced this issue from a commit

2023-09-11 14:43:44 +02:00

Cleanup: Anim: Replace use of `T_AUTOMERGE`

Falk David force-pushed gpv3-duplicate-keyframe from d82073b2b0 to 3e6236fec1

2023-09-11 15:21:11 +02:00

Compare

Falk David force-pushed gpv3-duplicate-keyframe from 3e6236fec1 to 45763571c1

2023-09-12 12:04:48 +02:00

Compare