GPv3: Extrude operator #121249

Amélie Fondevilla · 2024-04-30T10:25:30+02:00

2024-04-30 10:25:30 +02:00

Implementation of the extrude operator for grease pencil v3.
If an endpoint is selected, the extrusion will extend the stroke.
If an inner point is selected, the extrusion will create a new stroke tied to the one that is selected.
Change of behavior from gpv2 : the endpoints of a cyclic curve follow the same behavior as inner points.

Implementation of the extrude operator for grease pencil v3. If an endpoint is selected, the extrusion will extend the stroke. If an inner point is selected, the extrusion will create a new stroke tied to the one that is selected. Change of behavior from gpv2 : the endpoints of a cyclic curve follow the same behavior as inner points.

Amélie Fondevilla added this to the Module: Grease Pencil project 2024-04-30 10:26:47 +02:00

Amélie Fondevilla requested review from Falk David 2024-04-30 10:27:04 +02:00

Pratik Borhade reviewed 2024-04-30 10:50:04 +02:00

source/blender/editors/curves/intern/curves_edit.cc Outdated

						
				@ -225,4 +225,120 @@ void duplicate_curves(bke::CurvesGeometry &curves, const IndexMask &mask)

				  }

				}

				void extrude_curve_points(const bke::CurvesGeometry &src,

Pratik Borhade commented

2024-04-30 10:50:04 +02:00

Hi, "curves extrude" already exists and all functions are defined in curves_extrude.cc: !116354
this function is only used in grease_pencil_edit.cc so I think it might make sense to move definition in grease_pencil_edit file? 🤔

Hi, "curves extrude" already exists and all functions are defined in curves_extrude.cc: !116354 this function is only used in grease_pencil_edit.cc so I think it might make sense to move definition in grease_pencil_edit file? 🤔

👍 1 😆 1

Falk David commented

2024-04-30 11:43:44 +02:00

Yes, I think for now it's ok if this is just a static function in grease_pencil_edit.cc.

Yes, I think for now it's ok if this is just a static function in `grease_pencil_edit.cc`.

Iliya Katushenock commented

2024-04-30 11:57:04 +02:00

Think its better to use already done version (at least without a lot of vector loops)\

Iliya Katushenock commented

2024-04-30 12:03:10 +02:00

@filedescriptor Wrote stupid question, update page and try to delete my one, but accidentally did delete your response

I see, okay, generalization can be done later\

@filedescriptor Wrote stupid question, update page and try to delete my one, but accidentally did delete your response I see, okay, generalization can be done later\

😆 1

Falk David commented

2024-04-30 12:03:43 +02:00

The curves extrude version doesn't do what GPv2 does, so we can't reuse it.

Amélie Fondevilla commented

2024-04-30 13:08:09 +02:00

Thanks for spotting this @PratikPB2123 !
I will move it to grease_pencil_edit.cc 🙂

Thanks for spotting this @PratikPB2123 ! I will move it to `grease_pencil_edit.cc` 🙂

👍 1

amelief marked this conversation as resolved

Falk David requested changes 2024-04-30 11:42:44 +02:00

Falk David left a comment

Thanks :) Overall, the logic looks good. Just some cleanup comments and C++ thingies.

source/blender/editors/curves/intern/curves_edit.cc Outdated

						
				@ -228,0 +241,4 @@

				  array_utils::fill_index_range(dst_to_src_curves.as_mutable_span());

				  Vector<bool> dst_selected(old_points_num, false);

				  Vector<int> dst_curve_counts(old_curves_num);

Falk David commented

2024-04-30 11:38:31 +02:00

This could be initialized direcly like so:

Vector<int> dst_curve_counts(old_curves_num);
offset_indices::copy_group_sizes(points_by_curve, src.curves_range(), dst_curve_counts.as_mutable_span())

This could be initialized direcly like so: ```cpp Vector<int> dst_curve_counts(old_curves_num); offset_indices::copy_group_sizes(points_by_curve, src.curves_range(), dst_curve_counts.as_mutable_span()) ```

amelief marked this conversation as resolved

source/blender/editors/curves/intern/curves_edit.cc Outdated

						
				@ -228,0 +250,4 @@

				  for (const int curve_index : src.curves_range()) {

				    const IndexRange curve_points = points_by_curve[curve_index];

				    IndexMaskMemory mem;

				    const IndexMask curve_points_to_extrude = IndexMask::from_intersection(

Falk David commented

2024-04-30 11:24:03 +02:00

Using IndexMask::from_intersection works, but is a tad overkill. You can do

const IndexRange curve_points = points_by_curve[curve_index];
const IndexMask curve_points_to_extrude = points_to_extrude.slice_content(curve_points);

It's still not super efficient (slice_content is O(log N)), but will do fine for now.

There is also CurvesGeometry::point_to_curve_map() which will give you the curve index like so:
const int curve_index = point_to_curve_map[point_index];, but I don't know if this would make the rest of your logic work (since we're going over the same curve multiple times then).

Using `IndexMask::from_intersection` works, but is a tad overkill. You can do ```cpp const IndexRange curve_points = points_by_curve[curve_index]; const IndexMask curve_points_to_extrude = points_to_extrude.slice_content(curve_points); ``` It's still not super efficient (`slice_content` is `O(log N)`), but will do fine for now. There is also `CurvesGeometry::point_to_curve_map()` which will give you the curve index like so: `const int curve_index = point_to_curve_map[point_index];`, but I don't know if this would make the rest of your logic work (since we're going over the same curve multiple times then).

Amélie Fondevilla commented

2024-04-30 12:56:21 +02:00

I suspected that the intersection was not efficient yes 😅
For point_to_curve_map, I think I recall Hans saying that it was better to loop over the curves and use points_by_curve, but ofc happy to change if it's not the case here.

I suspected that the intersection was not efficient yes 😅 For `point_to_curve_map`, I think I recall Hans saying that it was better to loop over the curves and use `points_by_curve`, but ofc happy to change if it's not the case here.

Falk David commented

2024-04-30 12:59:35 +02:00

slice_content will do for now, I just mentioned CurvesGeometry::point_to_curve_map() in case it might come in handy in the future.

`slice_content` will do for now, I just mentioned `CurvesGeometry::point_to_curve_map()` in case it might come in handy in the future.

amelief marked this conversation as resolved

source/blender/editors/curves/intern/curves_edit.cc Outdated

						
				@ -228,0 +303,4 @@

				  /* Attributes. */

				  const bke::AttributeAccessor src_attributes = src.attributes();

				  bke::MutableAttributeAccessor dst_attributes = dst.attributes_for_write();

				  const bke::AnonymousAttributePropagationInfo propagation_info{};

Falk David commented

2024-04-30 11:06:46 +02:00

Small thing, but usually we just pass {} to gather_attributes, no need for a variable.

Small thing, but usually we just pass `{}` to `gather_attributes`, no need for a variable.

amelief marked this conversation as resolved

source/blender/editors/curves/intern/curves_edit.cc Outdated

						
				@ -228,0 +338,4 @@

				  }

				  dst.update_curve_types();

				  dst.tag_topology_changed();

Falk David commented

2024-04-30 11:05:37 +02:00

This line is not needed (resize or construction will automatically tag the topology).

This line is not needed (`resize` or construction will automatically tag the topology).

amelief marked this conversation as resolved

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

						
				@ -2478,0 +2490,4 @@

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

				    /* Extrusion always happens in the point domain. */

				    IndexMaskMemory memory;

				    const IndexMask points_to_extrude = retrieve_editable_and_selected_elements(

Falk David commented

2024-04-30 10:48:55 +02:00

Instead of retrieve_editable_and_selected_elements use retrieve_editable_and_selected_points here. The former is used when both domains need to be supported.

Instead of `retrieve_editable_and_selected_elements` use `retrieve_editable_and_selected_points` here. The former is used when both domains need to be supported.

amelief marked this conversation as resolved

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

						
				@ -2478,0 +2496,4 @@

				      return;

				    }

				    bke::CurvesGeometry &curves = info.drawing.strokes_for_write();

Falk David commented

2024-04-30 10:51:36 +02:00

Use info.drawing.strokes() here to get a const bke::CurvesGeometry &curves.

Use `info.drawing.strokes()` here to get a `const bke::CurvesGeometry &curves`.

amelief marked this conversation as resolved

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

						
				@ -2478,0 +2500,4 @@

				    bke::CurvesGeometry dst;

				    curves::extrude_curve_points(curves, points_to_extrude, dst);

				    info.drawing.geometry.wrap() = std::move(dst);

Falk David commented

2024-04-30 10:55:31 +02:00

Instead of declaring bke::CurvesGeometry dst; then passing it into extrude_curve_points as a mutable reference and then moving into the geometry of the drawing, you can do all that like so:

info.drawing.strokes_for_write() = std::move(curves::extrude_curve_points(curves, points_to_extrude));

This means that extrude_curve_points should construct a new CurvesGeometry and return it (by value).
So where you have dst.resize(new_points_num, new_curves_num); now, do

CurvesGeometry dst(new_points_num, new_curves_num);
...
return dst;

Move semantics will ensure that the curves geometry is constructed and then directly moved into the drawing, without any copy.

Instead of declaring `bke::CurvesGeometry dst;` then passing it into `extrude_curve_points` as a mutable reference and then moving into the geometry of the drawing, you can do all that like so: ```cpp info.drawing.strokes_for_write() = std::move(curves::extrude_curve_points(curves, points_to_extrude)); ``` This means that `extrude_curve_points` should construct a new `CurvesGeometry` and return it (by value). So where you have `dst.resize(new_points_num, new_curves_num);` now, do ```cpp CurvesGeometry dst(new_points_num, new_curves_num); ... return dst; ```` Move semantics will ensure that the curves geometry is constructed and then directly moved into the drawing, without any copy.

amelief marked this conversation as resolved

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

						
				@ -182,0 +187,4 @@

				                                    OPTYPE_UNDO | OPTYPE_REGISTER);

				  WM_operatortype_macro_define(ot, "GREASE_PENCIL_OT_extrude");

				  otmacro = WM_operatortype_macro_define(ot, "TRANSFORM_OT_translate");

				  // RNA_boolean_set(otmacro->ptr, "gpencil_strokes", true);

Falk David commented

2024-04-30 11:07:23 +02:00

I suppose this code can just be removed?

amelief marked this conversation as resolved

Amélie Fondevilla commented

2024-04-30 13:05:20 +02:00

Hey @filedescriptor thanks for the review, I'll edit the code.
I was wondering it we could optimize this by separating the case of the extrude containing endpoints or not.
If the selection does not include endpoints, then we can do the whole operation directly on the source, by resizing it and updating the attributes only on the newly created points, so it may be more efficient than the current implementation.
If the selection includes endpoints, then we cannot do that anymore, because we have to insert points inside of the existing curves.

I am wondering if it's worth it though, because we would have to go over all the curves and selected points just to check this. The readability would also be affected, but that's not a huge issue.

Hey @filedescriptor thanks for the review, I'll edit the code. I was wondering it we could optimize this by separating the case of the extrude containing endpoints or not. If the selection does not include endpoints, then we can do the whole operation directly on the source, by resizing it and updating the attributes only on the newly created points, so it may be more efficient than the current implementation. If the selection includes endpoints, then we cannot do that anymore, because we have to insert points inside of the existing curves. I am wondering if it's worth it though, because we would have to go over all the curves and selected points just to check this. The readability would also be affected, but that's not a huge issue.

Falk David commented

2024-04-30 13:09:55 +02:00

@amelief I think they way you wrote it now is fine. We could check for this edge case, but I think it's not worth it. In the end, we might want to change the way extrude works anyway to try and unify it with the curves code.

👍 1

Amélie Fondevilla force-pushed gpv3-extrude-point-operator from 7310379437 to 0b369be623

2024-05-02 12:35:09 +02:00

Compare

Falk David approved these changes 2024-05-02 12:44:52 +02:00

Falk David left a comment

Only have a cleanup comment, but this looks ready to me, so I will accept it. Also did a local test and it works as I expect.

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

						
				@ -2485,0 +2512,4 @@

				  int point_offset = 0;

				  for (const int curve_index : src.curves_range()) {

				    const IndexRange curve_points = points_by_curve[curve_index];

				    IndexMaskMemory mem;