GPv3: Split Stroke #115842

In summary, the Split Strokes operator will make sure all the point selection ranges within a stroke become their own stroke. This means that
a) The number of points stays exactly the same and all the point attributes stay the same.
b) The number of curves/strokes changes so we need to change the offsets and propagate the curve attributes.

So we don't need to recreate a new CurvesGeometry. We can resize the existing CurvesGeometry and only change the number of strokes.

The duplicate_points function in grease_pencil_edit.cc is doing something similar. Note that for the duplicate operation we do need to add more points, which in the split case, we don't.

I have some higher level thoughts about this. In summary, the `Split Strokes` operator will make sure all the point selection ranges within a stroke become their own stroke. This means that a) The number of points stays exactly the same and all the point attributes stay the same. b) The number of curves/strokes changes so we need to change the `offsets` and propagate the curve attributes. So we don't need to recreate a new `CurvesGeometry`. We can `resize` the existing `CurvesGeometry` and only change the number of strokes. The `duplicate_points` function in `grease_pencil_edit.cc` is doing something similar. Note that for the duplicate operation we do need to add more points, which in the split case, we don't.

👍 1

Ayanfe Ibitoye added 1 commit 2023-12-07 16:01:46 +01:00

2567cdb556 Refactor: update instead of recreate curve

Ayanfe Ibitoye requested review from Falk David 2023-12-07 16:03:57 +01:00

Ayanfe Ibitoye added 1 commit 2023-12-10 15:24:14 +01:00

390d054085 Merge branch 'main' into gpv3-stroke-split

Ayanfe Ibitoye commented

2023-12-10 15:25:35 +01:00

First-time contributor

I made changes here in case you hadn't seen @filedescriptor

Falk David requested review from Hans Goudey 2023-12-15 13:40:11 +01:00

Ayanfe Ibitoye added 1 commit 2024-01-02 00:55:51 +01:00

039fb4df62 Merge branch 'main' into gpv3-stroke-split

Ayanfe Ibitoye added 1 commit 2024-01-02 01:23:47 +01:00

c9e38eac69 Cleanup: update use of AttrDomain

Pratik Borhade commented

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

Hi, I'm not sure why selected and unselected cases are handled separately. Would it not work if we just change the offset to separate strokes based on selection_range. I mean for each range_i , do curve_counts.append(range.first()) and curve_counts.append(range.last()); so we can have 3 strokes (old_curve_count = 1) in results when selection_range is 1
I think I'll implement this roughly and share it here later

Hi, I'm not sure why selected and unselected cases are handled separately. Would it not work if we just change the offset to separate strokes based on selection_range. I mean for each range_i , do `curve_counts.append(range.first())` and `curve_counts.append(range.last());` so we can have 3 strokes (old_curve_count = 1) in results when selection_range is 1 I think I'll implement this roughly and share it here later

Ayanfe Ibitoye commented

2024-01-02 15:22:32 +01:00

First-time contributor

@PratikPB2123 I did want to combine the loops but wasn't sure how to do that. Your help is appreciated.

Pratik Borhade commented

2024-01-03 14:05:57 +01:00

Did some changes in existing "duplicate operator" code and got this working.
Points we're going to split the curve at, are end points of each "selection range":

+      dst_curve_counts.append(range.first());
+      dst_curve_counts.append(range.last() + 1);

Following code adds the new offset values to the end of new_curve_offsets then reorders this span/array in ascending order.
Feel free to try this.

I'm not sure whether this approach is correct. (@filedescriptor @HooglyBoogly knows better)

Something like this should work

diff --git a/source/blender/editors/curves/intern/curves_edit.cc b/source/blender/editors/curves/intern/curves_edit.cc
index 337d6f99bde..0f12ccb93a0 100644
--- a/source/blender/editors/curves/intern/curves_edit.cc
+++ b/source/blender/editors/curves/intern/curves_edit.cc
@@ -82,9 +82,13 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask)
           range.start() + points.first());
       curr_dst_point_start += range.size();

-      dst_curve_counts.append(range.size());
+      dst_curve_counts.append(range.first());
+      dst_curve_counts.append(range.last() + 1);
+
+      dst_to_src_curve.append(curve_i);
       dst_to_src_curve.append(curve_i);
       dst_cyclic.append(is_cyclic);
+      dst_cyclic.append(is_cyclic);
     }

     /* Join the first range to the end of the last range. */
@@ -105,15 +109,13 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask)
   bke::MutableAttributeAccessor attributes = curves.attributes_for_write();

   /* Delete selection attribute so that it will not have to be resized. */
-  attributes.remove(".selection");

-  curves.resize(old_points_num + num_points_to_add, old_curves_num + num_curves_to_add);
+  curves.resize(old_points_num, old_curves_num + num_curves_to_add);

   MutableSpan<int> new_curve_offsets = curves.offsets_for_write();
   array_utils::copy(dst_curve_counts.as_span(),
                     new_curve_offsets.drop_front(old_curves_num).drop_back(1));
-  offset_indices::accumulate_counts_to_offsets(new_curve_offsets.drop_front(old_curves_num),
-                                               old_points_num);
+  std::sort(new_curve_offsets.begin(), new_curve_offsets.end());

   /* Transfer curve and point attributes. */
   attributes.for_all([&](const bke::AttributeIDRef &id, const bke::AttributeMetaData meta_data) {
@@ -134,10 +136,10 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask)
         break;
       }
       case bke::AttrDomain::Point: {
-        bke::attribute_math::gather(
+        /*bke::attribute_math::gather(
             attribute.span,
             dst_to_src_point,
-            attribute.span.slice(IndexRange(old_points_num, num_points_to_add)));
+            attribute.span.slice(IndexRange(old_points_num, num_points_to_add)));*/
         break;
       }
       default: {
@@ -158,11 +160,6 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask)

   curves.update_curve_types();
   curves.tag_topology_changed();
-
-  bke::SpanAttributeWriter<bool> selection = attributes.lookup_or_add_for_write_span<bool>(
-      ".selection", bke::AttrDomain::Point);
-  selection.span.take_back(num_points_to_add).fill(true);
-  selection.finish();
 }

 void duplicate_curves(bke::CurvesGeometry &curves, const IndexMask &mask)
diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc
index 4b006748513..b77f66ff2a2 100644
--- a/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc
+++ b/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc
@@ -26,7 +26,6 @@ void ED_operatortypes_grease_pencil()
 void ED_operatormacros_grease_pencil()
 {
   wmOperatorType *ot;
-  wmOperatorTypeMacro *otmacro;

   /* Duplicate + Move = Interactively place newly duplicated strokes */
   ot = WM_operatortype_append_macro(
@@ -35,7 +34,4 @@ void ED_operatormacros_grease_pencil()
       "Make copies of the selected Grease Pencil strokes and move them",
       OPTYPE_UNDO | OPTYPE_REGISTER);
   WM_operatortype_macro_define(ot, "GREASE_PENCIL_OT_duplicate");
-  otmacro = WM_operatortype_macro_define(ot, "TRANSFORM_OT_translate");
-  RNA_boolean_set(otmacro->ptr, "use_proportional_edit", false);
-  RNA_boolean_set(otmacro->ptr, "mirror", false);
 }

Did some changes in existing "duplicate operator" code and got this working. Points we're going to split the curve at, are end points of each "selection range": ``` + dst_curve_counts.append(range.first()); + dst_curve_counts.append(range.last() + 1); ``` Following code adds the new offset values to the end of `new_curve_offsets` then reorders this span/array in ascending order. Feel free to try this. I'm not sure whether this approach is correct. (@filedescriptor @HooglyBoogly knows better) Something like this should work ```Diff diff --git a/source/blender/editors/curves/intern/curves_edit.cc b/source/blender/editors/curves/intern/curves_edit.cc index 337d6f99bde..0f12ccb93a0 100644 --- a/source/blender/editors/curves/intern/curves_edit.cc +++ b/source/blender/editors/curves/intern/curves_edit.cc @@ -82,9 +82,13 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask) range.start() + points.first()); curr_dst_point_start += range.size(); - dst_curve_counts.append(range.size()); + dst_curve_counts.append(range.first()); + dst_curve_counts.append(range.last() + 1); + + dst_to_src_curve.append(curve_i); dst_to_src_curve.append(curve_i); dst_cyclic.append(is_cyclic); + dst_cyclic.append(is_cyclic); } /* Join the first range to the end of the last range. */ @@ -105,15 +109,13 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask) bke::MutableAttributeAccessor attributes = curves.attributes_for_write(); /* Delete selection attribute so that it will not have to be resized. */ - attributes.remove(".selection"); - curves.resize(old_points_num + num_points_to_add, old_curves_num + num_curves_to_add); + curves.resize(old_points_num, old_curves_num + num_curves_to_add); MutableSpan<int> new_curve_offsets = curves.offsets_for_write(); array_utils::copy(dst_curve_counts.as_span(), new_curve_offsets.drop_front(old_curves_num).drop_back(1)); - offset_indices::accumulate_counts_to_offsets(new_curve_offsets.drop_front(old_curves_num), - old_points_num); + std::sort(new_curve_offsets.begin(), new_curve_offsets.end()); /* Transfer curve and point attributes. */ attributes.for_all([&](const bke::AttributeIDRef &id, const bke::AttributeMetaData meta_data) { @@ -134,10 +136,10 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask) break; } case bke::AttrDomain::Point: { - bke::attribute_math::gather( + /*bke::attribute_math::gather( attribute.span, dst_to_src_point, - attribute.span.slice(IndexRange(old_points_num, num_points_to_add))); + attribute.span.slice(IndexRange(old_points_num, num_points_to_add)));*/ break; } default: { @@ -158,11 +160,6 @@ void duplicate_points(bke::CurvesGeometry &curves, const IndexMask &mask) curves.update_curve_types(); curves.tag_topology_changed(); - - bke::SpanAttributeWriter<bool> selection = attributes.lookup_or_add_for_write_span<bool>( - ".selection", bke::AttrDomain::Point); - selection.span.take_back(num_points_to_add).fill(true); - selection.finish(); } void duplicate_curves(bke::CurvesGeometry &curves, const IndexMask &mask) diff --git a/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc b/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc index 4b006748513..b77f66ff2a2 100644 --- a/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc +++ b/source/blender/editors/grease_pencil/intern/grease_pencil_ops.cc @@ -26,7 +26,6 @@ void ED_operatortypes_grease_pencil() void ED_operatormacros_grease_pencil() { wmOperatorType *ot; - wmOperatorTypeMacro *otmacro; /* Duplicate + Move = Interactively place newly duplicated strokes */ ot = WM_operatortype_append_macro( @@ -35,7 +34,4 @@ void ED_operatormacros_grease_pencil() "Make copies of the selected Grease Pencil strokes and move them", OPTYPE_UNDO | OPTYPE_REGISTER); WM_operatortype_macro_define(ot, "GREASE_PENCIL_OT_duplicate"); - otmacro = WM_operatortype_macro_define(ot, "TRANSFORM_OT_translate"); - RNA_boolean_set(otmacro->ptr, "use_proportional_edit", false); - RNA_boolean_set(otmacro->ptr, "mirror", false); } ```

Ayanfe Ibitoye added 2 commits 2024-01-04 19:27:44 +01:00

61681dd500 Cleanup: Use one loop rather than two to gen maps

dea31c60ac Merge branch 'main' into gpv3-stroke-split

Ayanfe Ibitoye commented

2024-01-04 19:35:20 +01:00

First-time contributor

I managed to get it down to one loop. I made a to_ranges function so I could have the one range and iterate only over that.

Falk David reviewed 2024-01-08 12:24:20 +01:00

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

						
				@ -587,0 +591,4 @@

				/**

				 * Finds all the index ranges for consecutive values in \a span.

				 */

				template<typename T> inline Vector<IndexRange> to_ranges(const Span<T> span)

Falk David commented

2024-01-08 12:23:59 +01:00

No need to reimplement this. Use array_utils::find_all_ranges (from here).

No need to reimplement this. Use `array_utils::find_all_ranges` ([from here](https://projects.blender.org/blender/blender/src/commit/4d56b696ece2e2376658afd1773c7de020ae2b79/source/blender/blenlib/BLI_array_utils.hh#L242)).

Ayanfe Ibitoye commented

2024-01-08 12:33:32 +01:00

First-time contributor

I was using array_utils::find_all_ranges before, but it takes a value and only returns the ranges for indices equal to that value, so I had to have two ranges for selected and unselected points. I added this so I could get all the ranges at once and only need one loop.

I was using `array_utils::find_all_ranges` before, but it takes a value and only returns the ranges for indices equal to that value, so I had to have two ranges for selected and unselected points. I added this so I could get all the ranges at once and only need one loop.

Ayanfe Ibitoye added 1 commit 2024-01-15 18:05:50 +01:00

d6bd7f3afa Merge branch 'main' into gpv3-stroke-split

Ayanfe Ibitoye added 1 commit 2024-01-15 21:56:13 +01:00

78d64fef19 Cleanup: Move to_ranges to array_utils

Falk David requested changes 2024-01-23 16:13:46 +01:00

Falk David left a comment

The structure looks good to me, I left some cleanup comments.
Would like @HooglyBoogly to take a look too.

The structure looks good to me, I left some cleanup comments. Would like @HooglyBoogly to take a look too.

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

						
				@ -592,0 +705,4 @@

				  });

				  array_utils::copy(cyclic.as_span(), curves.cyclic_for_write());

				  curves.remove_attributes_based_on_types();

Falk David commented

2024-01-23 16:10:44 +01:00

The types of the curves did not change, so this line can be removed.

arryemoji marked this conversation as resolved

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

						
				@ -592,0 +707,4 @@

				  curves.remove_attributes_based_on_types();

				  return;

Falk David commented

2024-01-23 16:10:16 +01:00

Remove return, not needed

Remove `return`, not needed

arryemoji marked this conversation as resolved

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

						
				@ -592,0 +717,4 @@

				  Object *object = CTX_data_active_object(C);

				  GreasePencil &grease_pencil = *static_cast<GreasePencil *>(object->data);

				  const blender::bke::AttrDomain selection_domain = ED_grease_pencil_selection_domain_get(scene->toolsettings);

Falk David commented

2024-01-23 16:06:31 +01:00

Looks like this is unused, can be removed.

arryemoji marked this conversation as resolved

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

						
				@ -592,0 +723,4 @@

				  const Array<MutableDrawingInfo> drawings = retrieve_editable_drawings(*scene, grease_pencil);

				  threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) {

				    IndexMaskMemory memory;

				    const IndexMask mask = ed::greasepencil::retrieve_editable_and_selected_elements(

Falk David commented

2024-01-23 16:07:32 +01:00

I believe this operator should only work in point selection mode? I that case this should just use retrieve_editable_and_selected_points

I believe this operator should only work in point selection mode? I that case this should just use `retrieve_editable_and_selected_points`

arryemoji marked this conversation as resolved

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

						
				@ -592,0 +751,4 @@

				  /* callbacks */

				  ot->exec = grease_pencil_stroke_split_exec;

				  ot->poll = editable_grease_pencil_poll;

Falk David commented

2024-01-23 16:08:06 +01:00

-> editable_grease_pencil_point_selection_poll

-> `editable_grease_pencil_point_selection_poll`

arryemoji marked this conversation as resolved

Ayanfe Ibitoye added 2 commits 2024-01-23 16:42:25 +01:00

1d200d8a09 Merge branch 'main' into gpv3-stroke-split

974c89bbc6 Cleanup: Remove unnecessary code

Falk David commented

2024-01-23 17:01:35 +01:00

While testing this PR locally, I noticed that when selecting the first point, then using the operator, it does not create a new stroke for that point.

Ayanfe Ibitoye added 1 commit 2024-01-23 18:01:59 +01:00

e5db8f1751 Fix: don't skip first point in to_ranges

Ayanfe Ibitoye commented

2024-01-23 18:03:47 +01:00

First-time contributor

While testing this PR locally, I noticed that when selecting the first point, then using the operator, it does not create a new stroke for that point.

Figured out what I did wrong. Fixed now.

> While testing this PR locally, I noticed that when selecting the first point, then using the operator, it does not create a new stroke for that point. Figured out what I did wrong. Fixed now.

Falk David requested changes 2024-02-06 12:19:14 +01:00

Falk David left a comment

The code for this looks generally fine now, one comment

source/blender/blenlib/BLI_array_utils.hh Outdated

						
				@ -252,0 +252,4 @@

				/**

				 * Finds all the index ranges for consecutive values in \a span.

				 */

				template<typename T> inline Vector<IndexRange> to_ranges(const Span<T> span)

Falk David commented

2024-02-06 12:16:58 +01:00

I'd expect this to return OffsetIndices<int>, which would make this a bit better. Indexing into OffsetIndices with operator[] still gives you an IndexRange so it shouldn't change the rest of your code :)

I'd expect this to return `OffsetIndices<int>`, which would make this a bit better. Indexing into `OffsetIndices` with `operator[]` still gives you an `IndexRange` so it shouldn't change the rest of your code :)

Ayanfe Ibitoye commented

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

First-time contributor

I've done this. If there's a better way to handle the backing vector, please let me know. I couldn't find any other examples so I went this route. Thanks!

Iliya Katushenock reviewed 2024-02-06 12:23:15 +01:00

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

						
				@ -408,0 +409,4 @@

				/** \name Split Stroke Operator

				 * \{ */

				static void split_points(bke::CurvesGeometry &curves, const IndexMask &mask)

Iliya Katushenock commented

2024-02-06 12:22:17 +01:00

It is this is not the same thing as remove_points_and_split?

It is this is not the same thing as `remove_points_and_split`?

Falk David commented

2024-02-06 12:32:55 +01:00

No, remove_points_and_split will remove points, this doesn't remove them. It creates new curves.
But I agree, there are some overlapping things. Could be refactored at some point.

No, `remove_points_and_split` will remove points, this doesn't remove them. It creates new curves. But I agree, there are some overlapping things. Could be refactored at some point.

Iliya Katushenock commented

2024-02-06 12:44:04 +01:00

remove_points_and_split(mask) + remove_points_and_split(!mask) != split_points(mask) ?

`remove_points_and_split(mask)` + `remove_points_and_split(!mask)` != `split_points(mask)` ?

Falk David commented

2024-02-06 12:46:36 +01:00

I would argue remove_points_and_split = split_points + curves_copy_point_selection ;)

I would argue `remove_points_and_split` = `split_points` + `curves_copy_point_selection` ;)

Ayanfe Ibitoye added 1 commit 2024-02-06 16:58:52 +01:00

c146868d99 Refactor: use OffsetIndices

~~casey-bianco-davis referenced this pull request 2024-02-06 21:49:19 +01:00~~

GPv3: Edit Mode #112858