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

9c6a7f74bb GPv3: Cyclical set operator

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

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

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

f900ee25e8 Fix: merge error.

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

9eefaa2cae Fix: merge error.

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

1c380a0897 Cleanup: add comments.

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

c06382930d Cleanup: rename "drawing_index" to "layer_index"

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

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

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

99a73f55ba Cleanup: use `retrieve_selected_curves`

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

12340f4b11 Cleanup: remove code to and end points

e8352e599a Cleanup: fix code problem

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

a2608cf2ba Cleanup: remove unneeded use of namespace.

441ae81f88 Cleanup: use masked fill and invert boolean

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

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

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

a56851b9e6 Cleanup: handle non-existing attribute

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())) {