GPv3: Cyclical set operator #111904

casey-bianco-davis · 2023-09-03T23:09:46+02:00

2023-09-03 23:09:46 +02:00

Simply sets the cyclical property selected strokes. Note this does not add extra geometry like the legacy operator.

Note: adds a key bind 'alt c' to toggle, same as for curves.

Simply sets the cyclical property selected strokes. Note this does not add extra geometry like the legacy operator. Note: adds a key bind 'alt c' to toggle, same as for curves. Resolves #113671

casey-bianco-davis added 1 commit 2023-09-03 23:10:00 +02:00

GPv3: Cyclical set operator 9c6a7f74bb

Note adds a key bind 'alt c' to toggle, same as for curves.

Iliya Katushenock added this to the Grease Pencil project 2023-09-03 23:16:21 +02:00

Hans Goudey requested review from Hans Goudey 2023-09-04 01:24:08 +02:00

Falk David requested review from Falk David 2023-09-11 11:51:23 +02:00

casey-bianco-davis added 1 commit 2023-09-13 03:27:28 +02:00

Merge branch 'main' into GPv3-cyclical-set-operator f280d22437

casey-bianco-davis added 1 commit 2023-09-13 03:32:40 +02:00

Fix: merge error. f900ee25e8

casey-bianco-davis added 1 commit 2023-09-13 03:34:44 +02:00

Fix: merge error. 9eefaa2cae

Hans Goudey commented

2023-10-02 23:43:46 +02:00

Is there some design task, or an existing operator to reference? Compared to just setting the "cyclic" attribute on curves, this seems pretty complicated. Anyway, the PR description should explain more about the decisions made in the code.

casey-bianco-davis added 1 commit 2023-10-03 01:58:54 +02:00

Cleanup: add comments. 1c380a0897

casey-bianco-davis added 2 commits 2023-10-14 03:53:16 +02:00

Cleanup: rename "drawing_index" to "layer_index" c06382930d

Merge branch 'main' into GPv3-cyclical-set-operator 04bf9fc002

Falk David commented

2023-10-16 13:46:40 +02:00

@HooglyBoogly Yea the part that takes up most of this PR is the add_geometry option. Basically, when closing a stroke, it would subdivide the last segment and add more points to it.

I agree that this is not ideal and should probably just be handled by the subdivide operator specifically (I am not sure if subdividing the closing segment is possible right now).

@HooglyBoogly Yea the part that takes up most of this PR is the `add_geometry` option. Basically, when closing a stroke, it would subdivide the last segment and add more points to it. I agree that this is not ideal and should probably just be handled by the subdivide operator specifically (I am not sure if subdividing the closing segment is possible right now).

Falk David requested changes 2023-10-16 13:57:45 +02:00

Falk David left a comment

It's probably better if the set_cyclic operator doesn't do more then setting the cyclic attribute even though this would not be consistent with the old operator. If user really wanted to, we could still support this by using an operator macro, that would first set the cyclic attribute and then create the points using another operator.

It's probably better if the `set_cyclic` operator doesn't do more then setting the `cyclic` attribute even though this would not be consistent with the old operator. If user really wanted to, we could still support this by using an operator macro, that would first set the `cyclic` attribute and then create the points using another operator.

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

						
				@ -804,0 +957,4 @@

				        MutableSpan<bool> cyclic = curves.cyclic_for_write();

				        const OffsetIndices<int> points_by_curve = curves.points_by_curve();

				        const VArray<bool> selection = *curves.attributes().lookup_or_default<bool>(

Falk David commented

2023-10-16 13:50:54 +02:00

I think it would be better to use retrieve_selected_curves(const bke::CurvesGeometry &curves, IndexMaskMemory &memory) and then do a IndexMask.foreach_index(...).

I think it would be better to use `retrieve_selected_curves(const bke::CurvesGeometry &curves, IndexMaskMemory &memory)` and then do a `IndexMask.foreach_index(...)`.

casey-bianco-davis marked this conversation as resolved

casey-bianco-davis added 2 commits 2023-10-17 01:35:27 +02:00

Merge branch 'main' into GPv3-cyclical-set-operator 625943f9d5

Cleanup: use retrieve_selected_curves 99a73f55ba

Falk David commented

2023-10-17 10:31:51 +02:00

@casey-bianco-davis I think if we remove the if (add_geometry) case, we could merge this PR already pretty much. You could split the geometry case into another PR.

@casey-bianco-davis I think if we remove the `if (add_geometry) ` case, we could merge this PR already pretty much. You could split the `geometry` case into another PR.

casey-bianco-davis added 2 commits 2023-10-17 21:41:57 +02:00

Cleanup: remove code to and end points 12340f4b11

Cleanup: fix code problem e8352e599a

casey-bianco-davis requested review from Falk David 2023-10-17 21:55:11 +02:00

Falk David approved these changes 2023-10-18 10:16:28 +02:00

Falk David left a comment

Awesome! Looks good.

Hans Goudey requested changes 2023-10-18 10:28:15 +02:00

Hans Goudey left a comment

Looks good, just two suggestions to make this extra nice :)

Suggested inline moving some constant checks out of the loop, and adding a new version of array_utils::invert_booleans that also takes a const IndexMask & parameter.

Also, if the cyclic attribute doesn't already exist, and the mode is CLOSE, it would be nice to avoid adding the attribute.

Also, the description should be updated to reflect the current behavior.

Looks good, just two suggestions to make this extra nice :) Suggested inline moving some constant checks out of the loop, and adding a new version of `array_utils::invert_booleans` that also takes a `const IndexMask &` parameter. Also, if the `cyclic` attribute doesn't already exist, and the mode is `CLOSE`, it would be nice to avoid adding the attribute. Also, the description should be updated to reflect the current behavior.

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

						
				@ -805,0 +824,4 @@

				static int grease_pencil_cyclical_set_exec(bContext *C, wmOperator *op)

				{

				  using namespace blender;

Hans Goudey commented

2023-10-18 10:28:04 +02:00

We're already in the blender:: namespace, this shouldn't be necessary

We're already in the `blender::` namespace, this shouldn't be necessary

casey-bianco-davis marked this conversation as resolved

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

						
				@ -805,0 +847,4 @@

				        IndexMaskMemory memory;

				        const IndexMask curve_selection = ed::curves::retrieve_selected_curves(curves, memory);

				        curve_selection.foreach_index([&](const int64_t curve_i) {

Hans Goudey commented

2023-10-18 10:25:39 +02:00

switch (mode) {
  case CyclicalMode::CLOSE:
    index_mask::masked_fill(cyclic, true, curve_selection);
    break;
  case CyclicalMode::OPEN:
    index_mask::masked_fill(cyclic, false, curve_selection);
    break;
  case CyclicalMode::TOGGLE:
    array_utils::invert_booleans(cyclic, curve_selection);
    break;
}

``` switch (mode) { case CyclicalMode::CLOSE: index_mask::masked_fill(cyclic, true, curve_selection); break; case CyclicalMode::OPEN: index_mask::masked_fill(cyclic, false, curve_selection); break; case CyclicalMode::TOGGLE: array_utils::invert_booleans(cyclic, curve_selection); break; } ```

casey-bianco-davis marked this conversation as resolved

casey-bianco-davis added 2 commits 2023-10-19 02:08:04 +02:00

Cleanup: remove unneeded use of namespace. a2608cf2ba

Cleanup: use masked fill and invert boolean 441ae81f88

casey-bianco-davis added 1 commit 2023-10-19 02:36:48 +02:00

Merge branch 'main' into GPv3-cyclical-set-operator 051bad9e88

casey-bianco-davis added 1 commit 2023-10-19 04:34:28 +02:00

Cleanup: handle non-existing attribute a56851b9e6

casey-bianco-davis requested review from Hans Goudey 2023-10-19 04:34:32 +02:00

Hans Goudey approved these changes 2023-10-19 09:32:07 +02:00

Hans Goudey left a comment

Very nice, thanks!

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

						
				@ -855,0 +914,4 @@

				        }

				        /* Remove the attribute if it is empty. */

				        if (!ed::curves::has_anything_selected(curves.cyclic(), curves.curves_range())) {