GPv3: Basic layer channel selection #110133

Pratik Borhade · 2023-07-15T12:16:48+02:00

Pratik Borhade commented

2023-07-15 12:16:48 +02:00

PR adds support for the selection of grease pencil layers in
animation editor.

selection status is stored in GreasePencilLayerTreeNode
Existing GP_LAYER_TREE_NODE_SELECT flag is used for selection
selection of channel updates the active layer in object properties

PR adds support for the selection of grease pencil layers in animation editor. - selection status is stored in `GreasePencilLayerTreeNode` - Existing `GP_LAYER_TREE_NODE_SELECT` flag is used for selection - selection of channel updates the active layer in object properties Part of #110056

Pratik Borhade added 1 commit 2023-07-15 12:16:57 +02:00

df60efee65 GPv3: Basic layer channel selection

PR adds support for the selection of grease pencil layers in
animation editor.
- selection status is stored in `GreasePencilLayerTreeNode`
- Existing `GP_LAYER_TREE_NODE_SELECT` flag is used for selection
- selection of channel updates the active layer in object properties

Pratik Borhade requested review from Falk David 2023-07-15 12:17:08 +02:00

Pratik Borhade commented

2023-07-15 12:19:49 +02:00

@filedescriptor hi, changing active layer from properties tab do not change the layer selection in anim-editors. Any idea how to send notifier after execution of on_activate() function of LayerViewItem?

@filedescriptor hi, changing active layer from properties tab do not change the layer selection in anim-editors. Any idea how to send notifier after execution of `on_activate()` function of `LayerViewItem`?

Pratik Borhade added 1 commit 2023-07-15 12:31:39 +02:00

c8fcd9b274 Cleanup: Remove comment

Pratik Borhade added the

Module

Grease Pencil

label 2023-07-17 12:00:20 +02:00

Amélie Fondevilla added this to the Grease Pencil project 2023-07-17 17:55:55 +02:00

Falk David requested changes 2023-07-18 11:17:07 +02:00

Falk David left a comment

Thank you for the patch :) Some comments about the GP_LAYER_TREE_NODE_ACTIVE flag.

Thank you for the patch :) Some comments about the `GP_LAYER_TREE_NODE_ACTIVE` flag.

source/blender/editors/animation/anim_channels_edit.cc Outdated

						
				@ -299,1 +305,3 @@

				      case ANIMTYPE_GREASE_PENCIL_LAYER: {

				        GreasePencilLayer *layer = (GreasePencilLayer *)channel_data;

				        layer->base.flag |= GP_LAYER_TREE_NODE_ACTIVE;

Falk David commented

2023-07-18 11:16:16 +02:00

Instead of setting a flag here, this needs to set the active_layer pointer.

Instead of setting a flag here, this needs to set the `active_layer` pointer.

Pratik Borhade commented

2023-07-18 12:19:47 +02:00

Hi, thanks for the review.
Right now this is done in click_select_channel_grease_pencil_layer

But if we want this change in ANIM_set_active_channel() function, I think we'd need to pass bAnimListElem instead of ale->data (otherwise GreasePencil can not be obtained in this function)

Hi, thanks for the review. Right now this is done in `click_select_channel_grease_pencil_layer` But if we want this change in `ANIM_set_active_channel()` function, I think we'd need to pass `bAnimListElem` instead of `ale->data` (otherwise GreasePencil can not be obtained in this function)

Falk David commented

2023-07-18 12:35:26 +02:00

Can you check if ale->id is the right data here? Maybe that works.

Can you check if `ale->id` is the right data here? Maybe that works.

Pratik Borhade commented

2023-07-18 12:46:50 +02:00

No. ale itself is null here.
because we first iterate over all the present/visible channels to clear active flag
Then after exiting for loop (from here), ale is null

No. `ale` itself is null here. because we first iterate over all the present/visible channels to clear active flag Then after exiting for loop ([from here](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/animation/anim_channels_edit.cc#L246)), `ale` is null

Falk David commented

2023-07-18 13:53:55 +02:00

Right, maybe this is actually not needed for grease pencil then. Since changing the pointer to another layer does not require any change in all the other layers.

source/blender/makesdna/DNA_grease_pencil_types.h Outdated

						
				@ -234,6 +234,7 @@ typedef enum GreasePencilLayerTreeNodeFlag {

				  GP_LAYER_TREE_NODE_MUTE = (1 << 3),

				  GP_LAYER_TREE_NODE_USE_LIGHTS = (1 << 4),

				  GP_LAYER_TREE_NODE_USE_ONION_SKINNING = (1 << 5),

				  GP_LAYER_TREE_NODE_ACTIVE = (1 << 6),

Falk David commented

2023-07-18 11:13:47 +02:00

I don't think this flag is needed. We should just check the active_layer pointer in the grease pencil data.

I don't think this flag is needed. We should just check the `active_layer` pointer in the grease pencil data.

Pratik Borhade commented

2023-07-18 12:21:10 +02:00

Make sense. I thought this might be helpful for further selection operations (extend, invert, range select).

Falk David commented

2023-07-18 11:18:18 +02:00

@PratikPB2123 I think the active layer is now changed directly. Instead, the properties panel should call an operator or Python API function to change the active layer. That will both handle undo and also sending notifiers.

👍 1

Pratik Borhade added 1 commit 2023-07-18 15:45:28 +02:00

8e6305bcc7 Remove active flag and use of ANIM_set_active_channel

Pratik Borhade commented

2023-07-23 06:58:47 +02:00

@PratikPB2123 I think the active layer is now changed directly. Instead, the properties panel should call an operator or Python API function to change the active layer. That will both handle undo and also sending notifiers.

Done here: #110378

> @PratikPB2123 I think the active layer is now changed directly. Instead, the properties panel should call an operator or Python API function to change the active layer. That will both handle undo and also sending notifiers. Done here: https://projects.blender.org/blender/blender/pulls/110378

Pratik Borhade referenced this pull request

2023-07-23 07:00:49 +02:00

GPv3: send notifiers and undo push when changing the active layer #110378

Amélie Fondevilla referenced this pull request

2023-07-26 10:08:29 +02:00

GPv3: Integration in the Dopesheets #110056

Pratik Borhade referenced this issue from a commit

2023-07-27 11:37:54 +02:00

GPv3: Send notifiers and undo push when changing the active layer

Pratik Borhade added 3 commits 2023-07-27 13:17:35 +02:00

67fd064105 Merge branch 'main' into gpv3-active-layer-channel

884913f6ef Merge branch 'main' into gpv3-active-layer-channel

d976bcd5f6 Remove duplicate code

Done already in 0c07fb50c8

Falk David requested review from Amélie Fondevilla 2023-07-27 14:08:43 +02:00

Falk David requested changes 2023-07-27 14:10:13 +02:00

Falk David left a comment

Comment about casts.

source/blender/editors/animation/anim_channels_edit.cc Outdated

						
				@ -3653,0 +3656,4 @@

				    const short /* eEditKeyframes_Select or -1 */ selectmode,

				    const int filter)

				{

				  GreasePencilLayer *layer = (GreasePencilLayer *)ale->data;

Falk David commented

2023-07-27 14:09:26 +02:00

Use static_cast for newly added code.

Use `static_cast` for newly added code.

Pratik Borhade added 1 commit 2023-07-27 14:57:48 +02:00

66caad75d5 Cleanup: Use c++ casting

Almaz-Shinbay referenced this issue from a commit

2023-07-27 16:24:13 +02:00

GPv3: Send notifiers and undo push when changing the active layer

Almaz-Shinbay referenced this issue from a commit

2023-07-27 17:22:47 +02:00

GPv3: Send notifiers and undo push when changing the active layer

William Leeson referenced this issue from a commit

2023-07-28 18:18:59 +02:00

GPv3: Send notifiers and undo push when changing the active layer

Falk David requested changes 2023-08-01 18:12:09 +02:00

Falk David left a comment

Another comment. This PR should also be rebase on top of main.

Another comment. This PR should also be rebase on top of `main`.

source/blender/editors/animation/anim_channels_edit.cc Outdated

						
				@ -3653,0 +3653,4 @@

				    bContext *C,

				    bAnimContext *ac,

				    bAnimListElem *ale,

				    const short /* eEditKeyframes_Select or -1 */ selectmode,

Falk David commented

2023-08-01 18:11:07 +02:00

I'm getting some warnings about selectmode and filter being unused.
I think selectmode should be used. The different selection modes like shift+select and ctrl+select should work.

I'm getting some warnings about `selectmode` and `filter` being unused. I think `selectmode` should be used. The different selection modes like shift+select and ctrl+select should work.

Pratik Borhade commented

2023-08-02 04:45:45 +02:00

selectmode and filter is for future use (for other channel selection operations)
I don't think extend and extend range selection should be part of this PR (because this PR only changes the active channel and syncs it with layer in object properties panel)

Let me know if you want it within this PR or separate PR.

`selectmode` and `filter` is for future use (for other channel selection operations) I don't think extend and extend range selection should be part of this PR (because this PR only changes the active channel and syncs it with layer in object properties panel) Let me know if you want it within this PR or separate PR.

Falk David commented

2023-08-02 10:27:56 +02:00

Ok I'm ok with this coming in a later PR. But in this case the parameters need to be commented out e.g. const short /*selectmode*/, const int /*filter*/ and there should be a TODO comment in this function stating that the selection modes need to be implemented.

Ok I'm ok with this coming in a later PR. But in this case the parameters need to be commented out e.g. `const short /*selectmode*/, const int /*filter*/` and there should be a `TODO` comment in this function stating that the selection modes need to be implemented.

Pratik Borhade added 1 commit 2023-08-02 10:52:12 +02:00

cbb3b1d8be Comment out parameters

Pratik Borhade added 1 commit 2023-08-02 10:52:45 +02:00

053450d9e4 Merge branch 'main' into gpv3-active-layer-channel

Falk David reviewed 2023-08-02 11:39:23 +02:00

source/blender/editors/animation/anim_channels_edit.cc Outdated

						
				@ -3653,0 +3652,4 @@

				static int click_select_channel_grease_pencil_layer(bContext *C,

				                                                    bAnimContext *ac,

				                                                    bAnimListElem *ale,

				                                                    const short /* selectmode */,