GPv3: Time offset modifier #118371

Lukas Tönne · 2024-02-16T12:29:53+01:00

Lukas Tönne commented

2024-02-16 12:29:53 +01:00

Port of the Time Offset modifier from GPv2.

Changed behavior

The GPv2 version of this modifier handles many edge cases incorrectly:

The difference in scene frame range and the modifier frame range (matches scene range by default) should be added to the offset. But the modifier always uses 1 for the scene frame range.
In Reverse mode, if there is an input keyframe on frame 1 (start of the range more generally) both the last frame and the second-to-last show the first frame. If the first input frame is empty the last two are empty and the output starts with the 2nd keyframe. This is clearly a bug and is corrected in the GPv3 implementation.
Scaling affects frames unevenly, e.g. a scale of 0.5 should make every frame last 2 frames. But the result is that frame 1 lasts 3 frames and every subsequent frame lasts 2.

Port of the Time Offset modifier from GPv2. ## Changed behavior The GPv2 version of this modifier handles many edge cases incorrectly: 1. The difference in scene frame range and the modifier frame range (matches scene range by default) should be added to the offset. But the modifier always uses 1 for the scene frame range. 2. In _Reverse_ mode, if there is an input keyframe on frame 1 (start of the range more generally) both the last frame and the second-to-last show the first frame. If the first input frame is empty the last two are empty and the output starts with the 2nd keyframe. This is clearly a bug and is corrected in the GPv3 implementation. ![image](/attachments/9bd9e0bf-2e8c-410a-b358-2b126008da71) ![image](/attachments/fe61dc78-714d-469a-836e-b6ab36651f5a) 3. Scaling affects frames unevenly, e.g. a scale of `0.5` should make every frame last 2 frames. But the result is that frame 1 lasts _3_ frames and every subsequent frame lasts 2.

image.png

15 KiB

image.png

16 KiB

Lukas Tönne added 4 commits 2024-02-16 12:30:11 +01:00

101ac305f7 Boilerplate for the time offset modifier.

da830c282d Added missing UI bits and operators.

d6a7685a79 Added linear time transformation mode.

8101207341 Support for forward and reverse modes.

Lukas Tönne added 1 commit 2024-02-16 15:07:14 +01:00

efa962cadb Fixes.

Lukas Tönne force-pushed gp3-time-offset-modifier from 85b1e701d4 to 39ab85138e

2024-02-17 10:07:21 +01:00

Compare

Lukas Tönne added 1 commit 2024-02-19 09:16:10 +01:00

3e77283d87 Alternative implemnentation.

Lukas Tönne added 1 commit 2024-02-19 23:38:17 +01:00

a09e91938b Merge branch 'main' into gp3-time-offset-modifier

Lukas Tönne added 1 commit 2024-02-20 00:33:06 +01:00

76a3cc4de0 Fixed build errors.

Lukas Tönne added 1 commit 2024-02-20 09:23:48 +01:00

2754f2677f Merge branch 'main' into gp3-time-offset-modifier

Lukas Tönne added 2 commits 2024-02-20 15:14:18 +01:00

f1f9d5e3b6 Merge branch 'main' into gp3-time-offset-modifier

2fb1c5d4b7 progress

Lukas Tönne added 1 commit 2024-02-22 14:13:56 +01:00

62b85b0182 Merge branch 'main' into gp3-time-offset-modifier

Lukas Tönne added 1 commit 2024-02-22 15:01:04 +01:00

89b0a09ece Correct frame shift calculation for repeated frame ranges.

Lukas Tönne added 1 commit 2024-02-22 16:48:43 +01:00

33e33ad187 Reverse mode.

Lukas Tönne added 1 commit 2024-02-23 14:48:34 +01:00

8deb1cef5b Fix calculation of scene target frames when scaling is involved.

Lukas Tönne added 1 commit 2024-02-23 16:29:22 +01:00

ea6dab27a3 Implemented ping-pong mode.

Lukas Tönne added 2 commits 2024-02-24 10:41:11 +01:00

15c2b2e7ef Merge branch 'main' into gp3-time-offset-modifier

6ec2c1d8cb Cleanup: code deduplication.

Lukas Tönne added 1 commit 2024-02-24 11:11:47 +01:00

0cd640105d More simpler code.

Lukas Tönne added 1 commit 2024-02-24 14:50:08 +01:00

57b5cb50a5 Implemented chain mode.

Lukas Tönne added 1 commit 2024-02-24 16:07:24 +01:00

999adf421b Implemented "Fixed" mode.

Lukas Tönne changed title from ~~WIP: GPv3: Time offset modifier~~ to GPv3: Time offset modifier

2024-02-24 16:07:54 +01:00

Lukas Tönne added this to the Grease Pencil project 2024-02-24 16:08:00 +01:00

Lukas Tönne added 1 commit 2024-02-26 11:11:34 +01:00

73872a3e47 Conversion for the time offset modifier.

Lukas Tönne added 1 commit 2024-02-26 11:32:55 +01:00

6209b2f548 Fix memory leak when converting time offset and dash modifiers.

These modifiers create an initial segments array that must be freed
before replacing it with the converted segments array.

Falk David reviewed 2024-02-26 11:33:24 +01:00

Falk David left a comment

I see that there is commented out code and a few #if 0s. Should this PR still be WIP ?

I see that there is commented out code and a few `#if 0`s. Should this PR still be `WIP` ?

Lukas Tönne added 3 commits 2024-02-26 12:01:34 +01:00

bf2118067d Merge branch 'main' into gp3-time-offset-modifier

0a52782ccb Cleanup: remove unused code.

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

d85c4c739e Implemented frame range limit function.

Falk David requested changes 2024-02-26 12:20:56 +01:00

Dismissed

Falk David left a comment

Good job on making the code very readable 👍 I left a few cleanup comments.

source/blender/modifiers/intern/MOD_grease_pencil_time.cc

						
				@ -0,0 +134,4 @@

				 * reversing the direction. */

				static const IndexRange find_key_range(const Span<int> sorted_keys, const FrameRange &frame_range)

				{

				  IndexRange result = sorted_keys.index_range();

Falk David commented

2024-02-26 12:16:05 +01:00

Can be const

Can be `const`

Lukas Tönne commented

2024-02-26 12:31:33 +01:00

Is modified below (after function was cleaned up), the function snips off the head and tail keys that don't affect the frame range, and then returns result.

Is modified below (after function was cleaned up), the function snips off the head and tail keys that don't affect the frame range, and then returns `result`.

👍 1

LukasTonne marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_time.cc

						
				@ -0,0 +449,4 @@

				  const auto mode = GreasePencilTimeModifierMode(tmd.mode);

				  const bool use_custom_range = tmd.flag & MOD_GREASE_PENCIL_TIME_CUSTOM_RANGE;

				  const Scene *scene = DEG_get_evaluated_scene(ctx.depsgraph);

Falk David commented

2024-02-26 12:14:28 +01:00

I think fill_scene_timeline should just get a const Scene &eval_scene (instead of const ModifierEvalContext &ctx). Then DEG_get_evaluated_scene can just be called once before the loop over the layers in modify_geometry_set.

I think `fill_scene_timeline` should just get a `const Scene &eval_scene` (instead of `const ModifierEvalContext &ctx`). Then `DEG_get_evaluated_scene` can just be called once before the loop over the layers in `modify_geometry_set`.

LukasTonne marked this conversation as resolved

source/blender/modifiers/intern/MOD_grease_pencil_time.cc Outdated

						
				@ -0,0 +497,4 @@

				  const IndexMask layer_mask = modifier::greasepencil::get_filtered_layer_mask(

				      grease_pencil, tmd->influence, mask_memory);

				  for (const int64_t i : layer_mask.index_range()) {

Falk David commented

2024-02-26 12:10:45 +01:00

Would be a bit better to use the foreach_index API here:

const Span<Layer *> layers_for_write = grease_pencil.layers_for_write();
layer_mask.foreach_index([&] (const int64_t layer_i) {
   Layer &layer = *layers_for_write[layer_i];
   ...
});

Would be a bit better to use the `foreach_index` API here: ``` const Span<Layer *> layers_for_write = grease_pencil.layers_for_write(); layer_mask.foreach_index([&] (const int64_t layer_i) { Layer &layer = *layers_for_write[layer_i]; ... }); ```

LukasTonne marked this conversation as resolved