GPv3: Apply modifier to all frames #128487

Falk David · 2024-10-02T17:37:39+02:00

Falk David commented

2024-10-02 17:37:39 +02:00

This adds an option all_keyframes to the object.modifier_apply
operator.
With the option enabled, the operator will iterate through all the keyframes,
then apply the modifier and merge the result back into the original
object. This is only done for Grease Pencil objects.

This is how the default Apply operation worked in GPv2. This adds the
functionality back but also keeps the current Apply behavior for consistency
with other object types.

The UI is also changed to show both options in the dropdown menu.
Again, this is only shown for Grease Pencil objects.

This adds an option `all_keyframes` to the `object.modifier_apply` operator. With the option enabled, the operator will iterate through all the keyframes, then apply the modifier and merge the result back into the original object. This is only done for Grease Pencil objects. This is how the default `Apply` operation worked in GPv2. This adds the functionality back but also keeps the current `Apply` behavior for consistency with other object types. The UI is also changed to show both options in the dropdown menu. Again, this is only shown for Grease Pencil objects. <img width="474" alt="image" src="attachments/9d8a5635-4ba5-4938-9013-cd2c10c0c071">

image.png

41 KiB

image.png

28 KiB

Falk David added 1 commit 2024-10-02 17:37:48 +02:00

GPv3: Apply modifier to all frames cbb268bfee

This adds an option `all_keyframes` to the `object.modifier_apply`
operator.
With the option enabled, the operator will iterate through all the keyed
frames, apply the modifier and merge the result back into the original
object.

Falk David added this to the Grease Pencil project 2024-10-02 17:37:56 +02:00

Simon Thommes requested review from Simon Thommes 2024-10-02 17:41:12 +02:00

Falk David requested review from Jacques Lucke 2024-10-02 17:42:09 +02:00

Simon Thommes requested changes 2024-10-02 18:16:36 +02:00

Dismissed

Simon Thommes left a comment

Let's call the Apply operator Apply (Active Keyframe) in order to be more explicit about the difference and make it consistent
There is an issue right now, when there is a mismatch between the keyframe timings of individual layers. file: GP3_apply_all_keyframes.blend

I'm not entirely sure what the intended behavior is. Let's take this case:
Would the 2 apply operators give different results?
Or even simpler:

With Apply I would expect just what I currently see to be written to the active keyframe(s) per layer. But When doing this to all keyframes, I would expect the scene evaluation to be done on the respective frame of the keyframe per layer.

Regardless of the intention, there seems to be a bug because either way in the file I sent there shouldn't be any difference between the two.

* Let's call the `Apply` operator `Apply (Active Keyframe)` in order to be more explicit about the difference and make it consistent * There is an issue right now, when there is a mismatch between the keyframe timings of individual layers. file: [GP3_apply_all_keyframes.blend](/attachments/dda3aa25-af5e-43e4-8630-72280837bf67) I'm not entirely sure what the intended behavior is. Let's take this case: ![image](/attachments/a0a1cbde-d2ab-463d-a597-65c920470cd4) Would the 2 apply operators give different results? Or even simpler: ![image](/attachments/125a5ce5-cea2-4dc2-b49e-918901869464) With `Apply` I would expect just what I currently see to be written to the active keyframe(s) per layer. But When doing this to all keyframes, I would expect the scene evaluation to be done on the respective frame of the keyframe per layer. Regardless of the intention, there seems to be a bug because either way in the file I sent there shouldn't be any difference between the two.

image.png

18 KiB

image.png

19 KiB

GP3_apply_all_keyframes.blend

553 KiB

Falk David added 4 commits 2024-10-03 12:50:38 +02:00

Merge branch 'blender-v4.3-release' into gpv3-modifier-apply-all-frames e15c26e91d

Change Apply to Apply (Active Keyframe) a7a9a91665

Fix memory leaks 753ac7c744

Apply modifier only to layers with starting keyframe at eval frame

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

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-lint Build done.

Details

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

Details

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

Details

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

Details

4b370bf50c

Falk David commented

2024-10-03 13:04:25 +02:00

Updated the PR:

Fixed memory leaks
For Grease Pencil objects, Apply is now named Apply (Active Keyframe).
Fixed issue where applying to all frames would override changes to drawings for later frames. Now keyframes are applied by evaluating their respective frame only once.

Updated the PR: * Fixed memory leaks * For Grease Pencil objects, `Apply` is now named `Apply (Active Keyframe)`. * Fixed issue where applying to all frames would override changes to drawings for later frames. Now keyframes are applied by evaluating their respective frame only once.

Falk David requested review from Simon Thommes 2024-10-03 13:04:32 +02:00

Falk David commented

2024-10-03 13:04:42 +02:00

@blender-bot build

Simon Thommes requested changes 2024-10-03 14:48:48 +02:00

Simon Thommes left a comment

There is an issue when applying a modifier that adds a new layer.
For the existing Apply operator that works fine, as it just inserts a new keyframe on the current scene frame. But when doing this on all keyframes that is not really what I would expect (and currently also not what is happening).

I would suggest that the most expected behavior would be to go through all existing keyframes of all layers and create a new keyframe on those new layers.

In addition: If a layer does not exist in the evaluated data on one of those frames, it should insert an empty one. (that needs to be tracked throughout, since the final list of layers is not known until the final evaluation)

There is an issue when applying a modifier that adds a new layer. For the existing `Apply` operator that works fine, as it just inserts a new keyframe on the current scene frame. But when doing this on all keyframes that is not really what I would expect (and currently also not what is happening). I would suggest that the most expected behavior would be to go through all existing keyframes of all layers and create a new keyframe on those new layers. In addition: If a layer does not exist in the evaluated data on one of those frames, it should insert an empty one. (that needs to be tracked throughout, since the final list of layers is not known until the final evaluation)

Falk David commented

2024-10-03 15:57:00 +02:00

@SimonThommes and I talked about this a bit. In the end, this is what we ended up with:

For now, newly added layers will just insert a single keyframe (at the beginning of evaluation). This can be improved further at a later stage.
Newly created layers with an empty name need to be renamed at the end of evaluation
Layers that are not present in the evaluated state should not be removed, instead empty keyframes should be inserted.

@SimonThommes and I talked about this a bit. In the end, this is what we ended up with: * For now, newly added layers will just insert a single keyframe (at the beginning of evaluation). This can be improved further at a later stage. * Newly created layers with an empty name need to be renamed at the end of evaluation * Layers that are not present in the evaluated state should not be removed, instead empty keyframes should be inserted.

Falk David added 3 commits 2024-10-03 16:21:56 +02:00

Evaluate frames in sequential order 12bd1ff24d

Rename newly created layers after evaluation f6a1f79f98

Clear keyframes of layers that were removed in the evaluated geometry 5424ce9e85

Falk David commented

2024-10-03 16:25:44 +02:00

Updates:

Fix evaluation of all frames not happening sequentially
Procedurally created layers with an empty name will be renamed after evaluation of all frames. This means that this no longer creates a bunch of layers (Layer, Layer.001, etc.) for all the keyframes.
Layers that have been removed in the evaluated state don't get removed anymore, instead it gets (or creates) the keyframe and clears it.

Updates: * Fix evaluation of all frames not happening sequentially * Procedurally created layers with an empty name will be renamed after evaluation of all frames. This means that this no longer creates a bunch of layers (`Layer`, `Layer.001`, etc.) for all the keyframes. * Layers that have been removed in the evaluated state don't get removed anymore, instead it gets (or creates) the keyframe and clears it.

Falk David requested review from Simon Thommes 2024-10-03 16:25:51 +02:00

Falk David added 1 commit 2024-10-03 16:27:28 +02:00

Merge branch 'blender-v4.3-release' into gpv3-modifier-apply-all-frames 1b34343310

Falk David added 1 commit 2024-10-03 17:11:16 +02:00

Fix clearing keyframes before the current one

buildbot/vexp-code-patch-lint Build done.

Details

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

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

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

Details

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

Details

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

Details

6a68dab9f1

Falk David commented

2024-10-03 18:16:30 +02:00

@blender-bot build

Jacques Lucke approved these changes 2024-10-04 11:12:00 +02:00

Jacques Lucke left a comment

Can't comment too much on the exact behavior as this should be determined by artists, but it seems to work as I'd expect on my simple test case. The code looks reasonable overall.

source/blender/modifiers/intern/MOD_ui_common.cc

						
				@ -223,3 +222,1 @@

				          CTX_IFACE_(BLT_I18NCONTEXT_OPERATOR_DEFAULT, "Apply"),

				          ICON_CHECKMARK,

				          "OBJECT_OT_modifier_apply");

				  if (ob->type == OB_GREASE_PENCIL) {