GPv3: Autolock inactive layers #118323

Pratik Borhade · 2024-02-15T13:04:38+01:00

Pratik Borhade commented

2024-02-15 13:04:38 +01:00

Port legacy autolock_layers property to gpv3

Port legacy `autolock_layers` property to gpv3

Pratik Borhade added 1 commit 2024-02-15 13:04:48 +01:00

b6ee282100 GPv3: Autolock inactive layers

Port legacy `autolock_layers` property to gpv3

Pratik Borhade requested review from Falk David 2024-02-15 13:05:31 +01:00

Pratik Borhade added the

Module

Grease Pencil

label 2024-02-15 13:05:38 +01:00

Pratik Borhade added this to the Grease Pencil project 2024-02-15 13:06:20 +01:00

Falk David requested changes 2024-02-16 11:14:10 +01:00

Falk David left a comment

Added some comments. I think the double use of autolock_layers_set (clearing the locked state for all layers and locking everything but the active layer) makes the code a bit less readable.

Added some comments. I think the double use of `autolock_layers_set` (clearing the locked state for all layers and locking everything but the active layer) makes the code a bit less readable.

source/blender/blenkernel/intern/grease_pencil.cc Outdated

						
				@ -2189,0 +2199,4 @@

				      layer->set_locked(false);

				      continue;

				    }

				    layer->set_locked(is_autolock);

Falk David commented

2024-02-16 11:11:45 +01:00

Instead of checking the flag, always use false.

Instead of checking the flag, always use `false`.

Pratik Borhade commented

2024-02-16 11:46:53 +01:00

Hmm, then no layers will be locked 🙂
What the code doing is:
when autolock inactive layers property is enabled. is_autolock=true (lock other layers): layer->set_locked(true);

when property is disabled, is_autolock=flase (unlock all layers): layer->set_locked(false);

Hmm, then no layers will be locked 🙂 What the code doing is: when `autolock inactive layers` property is enabled. `is_autolock=true` (lock other layers): `layer->set_locked(true);` when property is disabled, `is_autolock=flase` (unlock all layers): `layer->set_locked(false);`

Falk David commented

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

Sorry, I meant true

Sorry, I meant `true`

👍 1

filedescriptor marked this conversation as resolved

source/blender/makesdna/DNA_grease_pencil_types.h Outdated

						
				@ -488,6 +489,7 @@ typedef struct GreasePencil {

				  blender::bke::greasepencil::Layer *get_active_layer();

				  void set_active_layer(const blender::bke::greasepencil::Layer *layer);

				  bool is_layer_active(const blender::bke::greasepencil::Layer *layer) const;

				  void autolock_layers_set();

Falk David commented

2024-02-16 10:48:21 +01:00

I think it's better to call this autolock_inactive_layers();

I think it's better to call this `autolock_inactive_layers();`

PratikPB2123 marked this conversation as resolved

source/blender/makesrna/intern/rna_grease_pencil.cc Outdated

						
				@ -43,0 +43,4 @@

				static void rna_grease_pencil_autolock(Main * /*bmain*/, Scene * /*scene*/, PointerRNA *ptr)

				{

				  GreasePencil *grease_pencil = rna_grease_pencil(ptr);

				  grease_pencil->autolock_layers_set();

Falk David commented

2024-02-16 11:13:26 +01:00

Instead, check the status of the flag, and if it's true, call the autolock, otherwise iterate over all layers and set the locked to false.

Instead, check the status of the flag, and if it's true, call the autolock, otherwise iterate over all layers and set the locked to `false`.

Pratik Borhade commented

2024-02-16 11:49:24 +01:00

Didn't understand this 😅

Pratik Borhade commented

2024-02-16 11:51:00 +01:00

Hi, thanks for the review :)

I think the double use of autolock_layers_set (clearing the locked state for all layers and locking everything but the active layer) makes the code a bit less readable.

We're not really clearing the lock state of all layer before locking them

Hi, thanks for the review :) > I think the double use of `autolock_layers_set` (clearing the locked state for all layers and locking everything but the active layer) makes the code a bit less readable. We're not really clearing the lock state of all layer before locking them

Pratik Borhade added 1 commit 2024-02-16 12:04:39 +01:00

b2c8f36ecd Rename to autolock_inactive_layers()

Falk David commented

2024-02-16 12:16:10 +01:00

@PratikPB2123 The update function of the property calls autolock_inactive_layers() in case the autolock flag is true and when it's false. Basically, I'm suggesting to only call autolock_inactive_layers() when GREASE_PENCIL_AUTOLOCK_LAYERS is set, otherwise just unlock all layers.

@PratikPB2123 The update function of the property calls `autolock_inactive_layers()` in case the autolock flag is true and when it's false. Basically, I'm suggesting to only call `autolock_inactive_layers()` when `GREASE_PENCIL_AUTOLOCK_LAYERS` is set, otherwise just unlock all layers.

Pratik Borhade commented

2024-02-16 12:37:44 +01:00

I see what you mean. In autolock_inactive_layers() function, set lock=true for non-active layers (and remove is_autolock variable)

And in property update function, check for the "autolock" flag to lock/unlock non-active layers. but that would be some additional lines which we can avoid 😅

But anyways, I'll update the PR.

I see what you mean. In `autolock_inactive_layers()` function, set lock=true for non-active layers (and remove `is_autolock` variable) And in property update function, check for the "autolock" flag to lock/unlock non-active layers. but that would be some additional lines which we can avoid 😅 But anyways, I'll update the PR.

Pratik Borhade added 1 commit 2024-02-16 12:55:19 +01:00

143b239197 Handle separately when autolock prop is disabled

Falk David approved these changes 2024-02-16 13:56:46 +01:00

Falk David left a comment

Thanks for making the changes :) Looks good

Pratik Borhade added 1 commit 2024-02-17 07:13:51 +01:00