GPv3: Refactor code for the hard eraser #111390

Amélie Fondevilla · 2023-08-22T16:11:39+02:00

2023-08-22 16:11:39 +02:00

This patch refactors most of the code of the hard eraser, both for readability purpose, and to prepare the integration of the soft mode of the eraser.
The refactoring includes :

the use of specific structures and enum types SegmentCircleIntersection, PointCircleSide, and PointTransferData to handle the data more easily,
improve readability of the intersections functions (better naming, and more accurate and generic comments),
definition of a more generic compute_topology_change function that handles point insertion, removal, and curve splitting for curves geometry.

This patch refactors most of the code of the hard eraser, both for readability purpose, and to prepare the integration of the soft mode of the eraser. The refactoring includes : - the use of specific structures and enum types `SegmentCircleIntersection`, `PointCircleSide`, and `PointTransferData` to handle the data more easily, - improve readability of the intersections functions (better naming, and more accurate and generic comments), - definition of a more generic `compute_topology_change` function that handles point insertion, removal, and curve splitting for curves geometry.

Amélie Fondevilla added this to the Grease Pencil project 2023-08-22 16:12:00 +02:00

Amélie Fondevilla added a new dependency 2023-08-22 16:12:31 +02:00

#110310 GPv3: Soft mode for the Eraser tool

Amélie Fondevilla requested review from Hans Goudey 2023-08-22 16:56:14 +02:00

Amélie Fondevilla force-pushed gpv3-erase-operator-refactor from b564fc1610 to 9dd9bc918d

2023-08-22 17:25:46 +02:00

Compare

Hans Goudey reviewed 2023-08-22 21:53:11 +02:00

Hans Goudey left a comment

Take my comments in the context of what you find reasonable, obviously you have a better idea of what's possible here.

I'd guess the most efficient way to transfer attribute when some points are copied and some are interpolated is to do it in two loops, where one selects the source points with an index mask and the other uses the int,float combination. But it's probably not needed here, if this makes the code easier to deal with, that's good :)

Take my comments in the context of what you find reasonable, obviously you have a better idea of what's possible here. I'd guess the most efficient way to transfer attribute when some points are copied and some are interpolated is to do it in two loops, where one selects the source points with an index mask and the other uses the `int,float` combination. But it's probably not needed here, if this makes the code easier to deal with, that's good :)

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -115,1 +115,4 @@

				  struct SegmentCircleIntersection {

				    /* Position of the intersection in the segment. */

				    float factor{0.0f};

Hans Goudey commented

2023-08-22 21:37:51 +02:00

It's not in the style guide I guess, but usually these defaults are written with =

It's not in the style guide I guess, but usually these defaults are written with `=`

amelief marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -116,0 +121,4 @@

				     * circle, false if it corresponds to an outside/inside transition . */

				    bool inside_outside_intersection{false};

				  };

				  enum PointCircleSide { Outside, OutsideInsideBoundary, InsideOutsideBoundary, Inside };

Hans Goudey commented

2023-08-22 21:38:46 +02:00

enum class is generally preferred since it requires each item to be scoped with the name of the enum. Though it's a bit longer, the context is usually helpful when reading code.

`enum class` is generally preferred since it requires each item to be scoped with the name of the enum. Though it's a bit longer, the context is usually helpful when reading code.

amelief marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -144,0 +148,4 @@

				                                                const int64_t squared_radius,

				                                                float &r_mu0,

				                                                float &r_mu1,

				                                                PointCircleSide &point_side,

Hans Goudey commented

2023-08-22 21:39:04 +02:00

Return arguments usually have the r_ prefix

Return arguments usually have the `r_` prefix

amelief marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -254,0 +258,4 @@

				      const bke::CurvesGeometry &src,

				      const Span<float2> screen_space_positions,

				      MutableSpan<PointCircleSide> r_point_side,

				      MutableSpan<Vector<SegmentCircleIntersection>> r_intersections) const

Hans Goudey commented

2023-08-22 21:42:46 +02:00

Not sure about the details of this case, but though the Array<Vector>> storage is pretty easy to use, it can be very slow and memory intensive. The overhead of a vector per point can make a big difference. Ultimately that's the goal of classes like OffsetIndices-- to make one alternative, slicing larger contiguous arrays, more intuitive.

Not sure about the details of this case, but though the `Array<Vector>>` storage is pretty easy to use, it can be very slow and memory intensive. The overhead of a vector per point can make a big difference. Ultimately that's the goal of classes like `OffsetIndices`-- to make one alternative, slicing larger contiguous arrays, more intuitive.

Amélie Fondevilla commented

2023-08-23 10:50:39 +02:00

Agree. It was so much easier to use Array<Vector> to figure out how to deal with intersections, because for the hard eraser there's a max of 2 intersections per segment, but for the soft eraser, this number can be larger, depending on the brush's falloff curve.
This number is the same for each segment though, so I think it's possible indeed to use a contiguous array while keeping multithread computation..
Let me know what you think of my proposed solution for this (commit 1d1f030ef0).

Agree. It was so much easier to use `Array<Vector>` to figure out how to deal with intersections, because for the hard eraser there's a max of 2 intersections per segment, but for the soft eraser, this number can be larger, depending on the brush's falloff curve. This number is the same for each segment though, so I think it's possible indeed to use a contiguous array while keeping multithread computation.. Let me know what you think of my proposed solution for this (commit 1d1f030ef0).

Hans Goudey commented

2023-08-23 13:41:11 +02:00

Yeah, seems good-- a bit of a complexity cost but hopefully people notice the tool is fast :P And if intersections_max_per_segment is ever too big, making it wasteful, offsets could be used too.

Yeah, seems good-- a bit of a complexity cost but hopefully people notice the tool is fast :P And if `intersections_max_per_segment` is ever too big, making it wasteful, offsets could be used too.

amelief marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -323,0 +365,4 @@

				   *

				   */

				  struct PointTransferData {

				    int64_t src_point;

Hans Goudey commented

2023-08-22 21:46:13 +02:00

I'd recommend sticking with 32 bit integers (int) unless there's a reason not to. For the forseable future I don't see us supporting more than 2 billion points in these geometries, and the 2x memory consumption cam also make a difference.

I'd recommend sticking with 32 bit integers (`int`) unless there's a reason not to. For the forseable future I don't see us supporting more than 2 billion points in these geometries, and the 2x memory consumption cam also make a difference.

👍 1

amelief marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_erase.cc Outdated

						
				@ -460,3 +476,1 @@

				      const IndexRange dst_points(dst_interm_curves_offsets[src_curve],

				                                  dst_interm_curves_offsets[src_curve + 1] -

				                                      dst_interm_curves_offsets[src_curve]);

				      const IndexRange dst_points_range(dst_interm_curves_offsets[src_curve],