GPv3: Join Operator #117916

Pedro · 2024-02-06T20:02:19+01:00

Pedro commented

2024-02-06 20:02:19 +01:00

First-time contributor

Create new stroke of selected points/strokes in active layer. Resolves #113582 and #113570

In terms of UI, the operator is in the Grease Pencil menu, since it works for both point and stroke mode. It has also the default keymap Ctrl + J

The operator affects the selection in the active layer in two possible ways, depending on the value of the rna parameter active_layer_behavior This enum parameter can have these values:

copy selection: the new stroke has a copy of the selected points. The already existing curves are not affected.
move selection: the points selected in the active layer are moved to the new stroke, thus affecting the existing curves.

Default value is copy selection

Video with demo for the points selection mode, using copy mode:

Same action in move mode:

Example in strokes selection mode:

Create new stroke of selected points/strokes in active layer. Resolves #113582 and #113570 In terms of UI, the operator is in the `Grease Pencil` menu, since it works for both point and stroke mode. It has also the default keymap `Ctrl + J` The operator affects the selection in the active layer in two possible ways, depending on the value of the rna parameter `active_layer_behavior` This enum parameter can have these values: - `copy selection`: the new stroke has a copy of the selected points. The already existing curves are not affected. - `move selection`: the points selected in the active layer are moved to the new stroke, thus affecting the existing curves. Default value is `copy selection` Video with demo for the points selection mode, using copy mode: <video src="/attachments/de4ef09b-3860-4b5e-8040-fdb2e05de719" title="points_copy.webm" controls></video> Same action in move mode: <video src="/attachments/951a239e-60f3-4260-928e-b67ce960f671" title="points_move.webm" controls></video> Example in strokes selection mode: <video src="/attachments/460fd654-2e24-4e7e-8c3b-f1bd58fc1cd1" title="strokes_move.webm" controls></video>

strokes_move.webm

428 KiB

points_copy.webm

1.5 MiB

points_move.webm

990 KiB

👍 1

Hans Goudey changed title from ~~Join Operator~~ to GPv3: Join Operator

2024-02-06 20:03:36 +01:00

Iliya Katushenock added this to the Grease Pencil project 2024-02-06 20:05:05 +01:00

Pedro referenced this pull request

2024-02-06 20:06:48 +01:00

GPv3: Merge Points #116726

Pedro requested review from Falk David 2024-02-06 20:08:02 +01:00

Pedro requested review from Hans Goudey 2024-02-06 20:08:03 +01:00

Pedro requested review from Pratik Borhade 2024-02-06 20:08:03 +01:00

Pedro requested review from casey-bianco-davis 2024-02-06 20:08:04 +01:00

casey-bianco-davis referenced this pull request

2024-02-06 21:52:02 +01:00

GPv3: Edit Mode #112858

Falk David commented

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

Thanks for opening it again!

❤️ 1

Pedro commented

2024-02-07 15:41:30 +01:00

First-time contributor

BTW, I don't know why the modal that appears in the bottom-left is disabled. Perhaps I'm missing to implement some callback? Would appreciate some guidance with this.

Also, I forgot to mention in the description that, despite the rna parameter behaves effectively as a boolean, I thought it would make more sense from the UX's perspective to implement it as an enum. I think in this case a combobox is a better choice than a checkbox

BTW, I don't know why the modal that appears in the bottom-left is disabled. Perhaps I'm missing to implement some callback? Would appreciate some guidance with this. Also, I forgot to mention in the description that, despite the rna parameter behaves effectively as a boolean, I thought it would make more sense from the UX's perspective to implement it as an enum. I think in this case a combobox is a better choice than a checkbox

Falk David commented

2024-02-07 15:50:52 +01:00

@PRiera1 This is because GPv3 doesn't have an edit mode undo system yet. See !117072.

👍 1

Falk David commented

2024-02-07 15:52:14 +01:00

@PRiera1 Also, please keep the UI the same as GPv2. We're trying to just be compatible for now and not introduce functional changes if we can avoid it.

👍 1

Falk David requested changes 2024-02-08 14:29:51 +01:00

Falk David left a comment

I noticed while testing that when using the Copy selection mode, the previous selection on the active layer doesn't get cleared. That's not the case in GPv2.

I noticed while testing that when using the `Copy selection` mode, the previous selection on the active layer doesn't get cleared. That's not the case in GPv2.

source/blender/blenkernel/BKE_curves.hh Outdated

						
				@ -398,0 +399,4 @@

				   * Change the direction of selected points (switch the start and end) without changing their

				   * shape.

				   */

				  void reverse_points(const IndexMask &points_to_reverse);

Falk David commented

2024-02-08 14:25:04 +01:00

This API shouldn't be added here. It's too specific for what you're doing. Keep it local to the operator file.

PRiera1 marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_join_selection.cc Outdated

						
				@ -0,0 +38,4 @@

				  bool belongs_to_active_layer;

				};

				enum class ActionOnNextRange { NOTHING, REVERSE_EXISTING, REVERSE_ADDITION, REVERSE_BOTH };

Falk David commented

2024-02-08 14:20:53 +01:00

These don't need to be capitalized.

PRiera1 marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_join_selection.cc Outdated

						
				@ -0,0 +40,4 @@

				enum class ActionOnNextRange { NOTHING, REVERSE_EXISTING, REVERSE_ADDITION, REVERSE_BOTH };

				enum ActiveLayerBehavior { COPY_SELECTION, MOVE_SELECTION };

Falk David commented

2024-02-08 14:22:15 +01:00

Can also use enum class. Just use int(ActiveLayerBehavior::<...>) in the EnumPropertyItem.
Also: Same as above.

Can also use `enum class`. Just use `int(ActiveLayerBehavior::<...>)` in the `EnumPropertyItem`. Also: Same as above.

PRiera1 marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_join_selection.cc Outdated

						
				@ -0,0 +423,4 @@

				  const int active_layer_behavior = RNA_enum_get(op->ptr, "active_layer_behavior");

				  const Layer *active_layer = grease_pencil.get_active_layer();

				  if (active_layer == nullptr) {

Falk David commented

2024-02-08 11:39:12 +01:00

Better to use the dedicated function here:

if (!grease_pencil.has_active_layer()) {
  return OPERATOR_CANCELLED;
}
const Layer &active_layer = *grease_pencil.get_active_layer();

Better to use the dedicated function here: ``` if (!grease_pencil.has_active_layer()) { return OPERATOR_CANCELLED; } const Layer &active_layer = *grease_pencil.get_active_layer(); ```

PRiera1 marked this conversation as resolved

Pedro force-pushed gpv3_merge_points from b02afb17dd to 059f0abaa3

2024-02-09 18:42:56 +01:00

Compare

Pedro requested review from Falk David 2024-02-09 18:44:10 +01:00

Pedro commented

2024-02-20 17:41:04 +01:00

First-time contributor

Hello @filedescriptor, I solved the issue you mention in your last comment about the selection. This PR has been open for some time. I will appreciate any review.

casey-bianco-davis approved these changes 2024-03-22 23:29:26 +01:00

Pedro commented

2024-03-23 10:31:10 +01:00

First-time contributor

Hi! Thanks for approving!

👍 1

This pull request has changes conflicting with the target branch.

source/blender/editors/grease_pencil/CMakeLists.txt

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u gpv3_merge_points:PRiera1-gpv3_merge_points

git checkout PRiera1-gpv3_merge_points

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Join Operator #117916

Checkout