GPv3: Segment selection support for all relevant selection operators #109221

Sietse Brouwer · 2023-06-22T00:27:51+02:00

Sietse Brouwer commented

2023-06-22 00:27:51 +02:00

This PR implements the 'Segment' mode for all relevant Grease Pencil
selection operators. Segments are the points in a stroke between
other, intersecting strokes.

Selecting and deselecting segments is supported by the following
operators:

Select pick
Box select
Lasso select
Circle select
Select all/none/invert
Select first/last
Select more/less
Select random

No action was needed for 'Select linked'. It works the same for 'Point'
and 'Segment' mode.

Technical notes
Since segment is not a natively supported selection domain, segments
are handled in the point domain, with the following pre- and post-
processing steps for each selection operator.

Pre-processing:

The selection state of all editable stroke points is stored in a
vector
A 2D viewport representation of all editable strokes is calculated
and stored. This 2D representation is used to detect stroke
intersections.

After this the selection operator alters the point selection.

Post-processing:

A comparison is made between the stored point selection and the new
selection. This 'diff' tells us which points are selected or deselected.
Each changed selection point is expanded to a segment by following the
points backward and forward until an intersection with an other stroke
is detected (or the end of the stroke is reached).

Where possible, parallel execution of the computations is used. The old
GPv2 was lacking simple bounding box checks when searching for stroke
intersections. So with this new implementation, in large scenes a
noticeable performance improvement is to be expected.

This PR implements the 'Segment' mode for all relevant Grease Pencil selection operators. Segments are the points in a stroke between other, intersecting strokes. Selecting and deselecting segments is supported by the following operators: - Select pick - Box select - Lasso select - Circle select - Select all/none/invert - Select first/last - Select more/less - Select random No action was needed for 'Select linked'. It works the same for 'Point' and 'Segment' mode. **Technical notes** Since segment is not a natively supported selection domain, segments are handled in the point domain, with the following pre- and post- processing steps for each selection operator. Pre-processing: - The selection state of all editable stroke points is stored in a vector - A 2D viewport representation of all editable strokes is calculated and stored. This 2D representation is used to detect stroke intersections. After this the selection operator alters the point selection. Post-processing: - A comparison is made between the stored point selection and the new selection. This 'diff' tells us which points are selected or deselected. - Each changed selection point is expanded to a segment by following the points backward and forward until an intersection with an other stroke is detected (or the end of the stroke is reached). Where possible, parallel execution of the computations is used. The old GPv2 was lacking simple bounding box checks when searching for stroke intersections. So with this new implementation, in large scenes a noticeable performance improvement is to be expected.

Sietse Brouwer added 4 commits 2023-06-22 00:28:02 +02:00

22021f406d GPv3: segment selection support for all relevant selection operators

fe8ed7eec7 Merge branch 'main' into gp3-select-segments

9097332b0f Cleanup: manual merge with main

e20a52ec8b Cleanup: rename function for expanding a selection point to segment

Sietse Brouwer requested review from Falk David 2023-06-22 00:28:21 +02:00

Sietse Brouwer added this to the Grease Pencil project 2023-06-22 00:28:27 +02:00

Falk David requested changes 2023-06-22 10:30:03 +02:00

Falk David left a comment

First pass, did not look at the segment selection algorithm yet.

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

						
				@ -189,3 +216,3 @@

				  ot->exec = select_random_exec;

				  ot->poll = editable_grease_pencil_poll;

				  ot->poll = editable_grease_pencil_no_segment_selection_poll;

Falk David commented

2023-06-22 10:24:34 +02:00

Not sure if this is necessary. IMO selecting random segments makes sense.

SietseB marked this conversation as resolved

source/blender/editors/include/ED_grease_pencil.h Outdated

						
				@ -34,2 +39,3 @@

				 * \param segment_mode: Pointer to bool, indicating 'select segment' mode is active.

				 */

				eAttrDomain ED_grease_pencil_selection_domain_get(struct bContext *C);

				eAttrDomain ED_grease_pencil_selection_domain_get(struct bContext *C, bool *segment_mode);

Falk David commented

2023-06-22 10:21:43 +02:00

I prefer if this would be it's own function. E.g. bool ED_grease_pencil_segment_selection_mode(struct bContext *C);

I prefer if this would be it's own function. E.g. `bool ED_grease_pencil_segment_selection_mode(struct bContext *C);`

SietseB marked this conversation as resolved

Sietse Brouwer added 1 commit 2023-06-22 14:45:14 +02:00

6c00fb10ee Cleanup after review and Select random method added for segments

Sietse Brouwer commented

2023-06-22 14:50:00 +02:00

I had a brainwave and added a Select Random method for segments with relative ease.

🎉 1

Sietse Brouwer added 1 commit 2023-06-22 17:46:02 +02:00

c3dd115096 Support for cyclic curves in segment selection

Hans Goudey changed title from ~~GPv3: Segment selection support for all relevant selection operators~~ to WIP: GPv3: Segment selection support for all relevant selection operators

2023-06-22 19:15:37 +02:00

Hans Goudey requested review from Hans Goudey 2023-06-22 19:17:56 +02:00

Hans Goudey requested changes 2023-06-22 23:09:02 +02:00

Hans Goudey left a comment

Thanks for the contribution!

I left some significant comments inline, mostly about how the algorithm is structured and how data is laid out in memory. But I also think some larger refactoring would be helpful here, since this is starting to muddy the distinction in abstraction levels (the high-level GP operators calling lower level curves functions). That distinction isn't working so well anymore.

In chat I mentioned a concern that this is quadratic. In theory it is, since it does things like "for all curves: for all other curves". But you're right in the patch description that the bounding box checks should help a lot at least. In the future an acceleration structure like a KD tree might help though, if that becomes a problem.

Thanks for the contribution! I left some significant comments inline, mostly about how the algorithm is structured and how data is laid out in memory. But I also think some larger refactoring would be helpful here, since this is starting to muddy the distinction in abstraction levels (the high-level GP operators calling lower level curves functions). That distinction isn't working so well anymore. In chat I mentioned a concern that this is quadratic. In theory it is, since it does things like "for all curves: for all other curves". But you're right in the patch description that the bounding box checks should help a lot at least. In the future an acceleration structure like a KD tree might help though, if that becomes a problem.

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

						
				@ -250,0 +326,4 @@

				Vector<bool> point_selection_get(const GreasePencilDrawing *drawing)

				{

				  /* Get point selection in the drawing. */

				  bke::CurvesGeometry curves = drawing->geometry.wrap();

Hans Goudey commented

2023-06-22 23:05:14 +02:00

This is making a copy of the curves, like above. Best to use a const reference. Then you can't call ensure_selection_attribute, but that's on purpose. A function like this shouldn't be able to modify the curves.

This is making a copy of the curves, like above. Best to use a const reference. Then you can't call `ensure_selection_attribute`, but that's on purpose. A function like this shouldn't be able to modify the curves.

Sietse Brouwer commented

2023-06-23 11:40:32 +02:00

We have to ensure then that is selection domain is always correct. That's something for a separate PR, I guess. I made a task for it. @filedescriptor

SietseB marked this conversation as resolved

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

						
				@ -250,0 +332,4 @@

				  MutableSpan<bool> selection_typed = selection.span.typed<bool>();

				  /* Copy selection. */

				  Vector<bool> point_selection(selection.span.size());

Hans Goudey commented

2023-06-22 23:05:26 +02:00

Vector -> Array, since the size doesn't need to change.

`Vector` -> `Array`, since the size doesn't need to change.

SietseB marked this conversation as resolved

source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc

						
				@ -0,0 +23,4 @@

				{

				  /* Count total number of editable strokes in grease pencil object. */

				  int stroke_count = 0;

				  grease_pencil->foreach_editable_drawing(

Hans Goudey commented

2023-06-22 22:51:18 +02:00

I'd recommend running foreach_editable_drawing just once, at the start of the algorithm, and keeping a Vector of drawings. Then iteration can be simpler after that, and code can be parallelized over all drawing.

But actually, even better might be to handle each drawing separately, so:

for each editable drawing:
    do selection operation
    project drawing to 2d
    do segment extension, etc.

That way memory access is more localized (i.e. only dealing with a single drawing at a time means CPU caches aren't being repeatedly clobbered by loading the data from a new drawing), and each piece of code only has to deal with a single CurvesGeometry

I'd recommend running `foreach_editable_drawing` just once, at the start of the algorithm, and keeping a `Vector` of drawings. Then iteration can be simpler after that, and code can be parallelized over all drawing. But actually, even better might be to handle each drawing separately, so: ``` for each editable drawing: do selection operation project drawing to 2d do segment extension, etc. ``` That way memory access is more localized (i.e. only dealing with a single drawing at a time means CPU caches aren't being repeatedly clobbered by loading the data from a new drawing), and each piece of code only has to deal with a single `CurvesGeometry`

Sietse Brouwer commented

2023-06-23 11:56:53 +02:00

I'll handle the first one.

Unfortunately, the second one isn't going to work. A drawing is just a layer, intersections can come from other layers to. So the 'project to 2d' can't be inside the drawing loop, then we're missing information.

I'll handle the first one. Unfortunately, the second one isn't going to work. A drawing is just a layer, intersections can come from other layers to. So the 'project to 2d' can't be inside the drawing loop, then we're missing information.

Hans Goudey commented

2023-06-23 13:24:16 +02:00

Ah, I didn't realize this was meant to handle intersections with other drawings too

SietseB marked this conversation as resolved

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

						
				@ -0,0 +43,4 @@

				  grease_pencil->foreach_editable_drawing(

				      vc->scene->r.cfra, [&](int drawing_index, GreasePencilDrawing &drawing) {

				        /* Get deformed geomtry. */

				        const bke::CurvesGeometry curves = drawing.geometry.wrap();

Hans Goudey commented

2023-06-22 22:47:19 +02:00

It's critical not to forget the & here when declaring a CurvesGeometry variable. Currently this copies the struct (though implicit sharing will keep this from copying all the attribute layers).

It's critical not to forget the `&` here when declaring a `CurvesGeometry` variable. Currently this copies the struct (though implicit sharing will keep this from copying all the attribute layers).

SietseB marked this conversation as resolved

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

						
				@ -0,0 +50,4 @@

				                ob_eval, *vc->obedit, drawing_index);

				        /* Loop all strokes (curves). */

				        threading::parallel_for(curves.curves_range(), 256, [&](const IndexRange range) {

Hans Goudey commented

2023-06-22 22:53:55 +02:00

For a single CurvesGeometry, I'd suggest structuring it like this:

Array<float2> positions_re(points_num); // positions in region space
for point : curves.points_range():
   positions_re = project_point_into_2D(positions[point])
Array<Bounds<float2>> curve_bounds; // array of bounds from BLI_bounds.hh (half the size of 
for each curve:
   curve_bounds[i] = *bounds::min_max(positions_re.slice(points_by_curve[curve]);
etc..

Not storing a Vector for every curve is important, since it means all the positions are in one contiguous array. This makes it faster to access, and avoids the 56 bytes of overhead for each Vector (pointers and inline buffer). Doing the projection in one loop just makes it simpler, more parallel, and easier to optimize (with SIMD, etc.) in the future.

For a single `CurvesGeometry`, I'd suggest structuring it like this: ```Cpp Array<float2> positions_re(points_num); // positions in region space for point : curves.points_range(): positions_re = project_point_into_2D(positions[point]) Array<Bounds<float2>> curve_bounds; // array of bounds from BLI_bounds.hh (half the size of for each curve: curve_bounds[i] = *bounds::min_max(positions_re.slice(points_by_curve[curve]); etc.. ``` Not storing a `Vector` for every curve is important, since it means all the positions are in one contiguous array. This makes it faster to access, and avoids the 56 bytes of overhead for each Vector (pointers and inline buffer). Doing the projection in one loop just makes it simpler, more parallel, and easier to optimize (with SIMD, etc.) in the future.

Sietse Brouwer commented

2023-06-23 12:00:02 +02:00

Will do, thanks for the insights!

SietseB marked this conversation as resolved

source/blender/editors/include/ED_grease_pencil.h Outdated

						
				@ -32,2 +36,3 @@

				/**

				 * Get the selection mode for Grease Pencil selection operators: point, stroke, segment.

				 * Get the selection domain for Grease Pencil selection operators.

				 * \param C: Context.

Hans Goudey commented

2023-06-22 22:58:21 +02:00

Avoid this sort of meaningless comment. "Context" is obvious from the name of the variable's type, so this just takes up space.

SietseB marked this conversation as resolved

source/blender/editors/space_view3d/view3d_select.cc Outdated

						
				@ -1197,0 +1205,4 @@

				        /* In segment mode, store the pre-change point selection. */

				        Vector<bool> selection_before;

				        if (segment_mode) {

				          selection_before = ed::greasepencil::point_selection_get(&drawing);

Hans Goudey commented

2023-06-22 23:03:20 +02:00

Combining these two levels of abstraction is a bit troublesome. One the one hand, currently these grease pencil operators just call the curves functions, which are meant to do all of the real work. The GP operator doesn't have anything to do with the actual selection values (i.e. Vector<bool>). Combining the two is confusing, because it means this code is more "tacked on at the end" than it should be.

I wonder if maybe select_lasso and friends should return an Array<bool> or IndexMask of the points to change. Then those points could be adjusted later, at the same level.

Maybe @filedescriptor has a different opinion about this though.

Combining these two levels of abstraction is a bit troublesome. One the one hand, currently these grease pencil operators just call the `curves` functions, which are meant to do all of the real work. The GP operator doesn't have anything to do with the actual selection values (i.e. `Vector<bool>`). Combining the two is confusing, because it means this code is more "tacked on at the end" than it should be. I wonder if maybe `select_lasso` and friends should return an `Array<bool>` or `IndexMask` of the points to change. Then those points could be adjusted later, at the same level. Maybe @filedescriptor has a different opinion about this though.

Falk David commented

2023-06-23 12:01:26 +02:00

I do think that the better solution is to make the selection operators return not just a bool, but what they changed. I can also see this being useful for Curves in the future.

Sietse Brouwer commented

2023-06-23 12:31:30 +02:00

Yes, that would be great, when the operators return what they changed. Then I don't have to hack into these functions, which I don't like either.
Modifying all the curves functions to support that though – it's quite a task, I'm not confident that I can pull that off myself...

Yes, that would be great, when the operators return what they changed. Then I don't have to hack into these functions, which I don't like either. Modifying all the `curves` functions to support that though – it's quite a task, I'm not confident that I can pull that off myself...

Falk David commented

2023-06-23 14:31:59 +02:00

Just had a chat with Hans about this.

What might make the most sense is refactor the selection code to do the following instead:

have functions that return an IndexMask of the elements that are affected by the particular selection. E.g. IndexMask mask = elements_inside_lasso(...).
have the actual selection function change the selection attribute. E.g. ed::curves::select_lasso would call points_inside_lasso then use index_mask::masked_fill(...) to fill true or false based on the selection operation (deselect/select/invert) using the index mask.

This way, we wouldn't use ed::curves::select_lasso but the IndexMask instead to do the selection in grease pencil.
And that solves the problem we have with the segment selection now.

Just had a chat with Hans about this. What might make the most sense is refactor the selection code to do the following instead: - have functions that return an `IndexMask` of the elements that are affected by the particular selection. E.g. `IndexMask mask = elements_inside_lasso(...)`. - have the actual selection function change the selection attribute. E.g. `ed::curves::select_lasso` would call `points_inside_lasso` then use `index_mask::masked_fill(...)` to fill `true` or `false` based on the selection operation (deselect/select/invert) using the index mask. This way, we wouldn't use `ed::curves::select_lasso` but the `IndexMask` instead to do the selection in grease pencil. And that solves the problem we have with the segment selection now.

👍 1

Sietse Brouwer commented

2023-07-13 01:16:09 +02:00

I'm afraid I have to reopen this discussion. A problem with the IndexMask solution is that it returns affected points, but for an efficient segment expansion you only want to inspect the selection points that really changed. For circle select for example, it won't be performant at all when the segments under the 'brush' are recalculated all the time.
So I could convert the IndexMask to an array of changed selection points. But then it's getting pretty bloated, IMHO. To be honest, the leanest code I can think of is what it is now in the PR: 1. take a snapshot of the selection, 2. apply the selection function, 3. expand the changes to segments.

I think the underlying problem of this 'level trouble' is that there isn't a 'segment' selection domain (and there never will be). That's why we need the post-processing step.

I'm afraid I have to reopen this discussion. A problem with the `IndexMask` solution is that it returns _affected_ points, but for an efficient segment expansion you only want to inspect the selection points that really _changed_. For circle select for example, it won't be performant at all when the segments under the 'brush' are recalculated all the time. So I could convert the `IndexMask` to an array of changed selection points. But then it's getting pretty bloated, IMHO. To be honest, the leanest code I can think of is what it is now in the PR: 1. take a snapshot of the selection, 2. apply the selection function, 3. expand the changes to segments. I think the underlying problem of this 'level trouble' is that there isn't a 'segment' selection domain (and there never will be). That's why we need the post-processing step.

Iliya Katushenock commented

2023-07-13 01:33:56 +02:00

Why does a segment have such complex implicit behavior?
More precisely, if it must have such a complex behavior, why should it create complexity for other behaviors?
First, if want to end up with selected curve segments, then make segments.
I mean, you can define curve intersections before the start operator?
Based on the intersections, construct the vectors of offsets for each segment. And after that, make selection simular both for selection curves, or such segments (only based on offsets and positions), and index mas as result.

Why does a segment have such complex implicit behavior? More precisely, if it must have such a complex behavior, why should it create complexity for other behaviors? First, if want to end up with selected curve segments, then make segments. I mean, you can define curve intersections before the start operator? Based on the intersections, construct the vectors of offsets for each segment. And after that, make selection simular both for selection curves, or such segments (only based on offsets and positions), and index mas as result.

Sietse Brouwer commented

2023-07-13 16:23:50 +02:00

Interesting idea, but finding intersections is the heavy part of the operation. So finding all intersections before the select operator starts, would be horribly slow, I'm afraid.

Interesting idea, but finding intersections is the heavy part of the operation. So finding _all_ intersections before the select operator starts, would be horribly slow, I'm afraid.

Iliya Katushenock commented

2023-06-22 23:24:06 +02:00

If you already in blender::ed::greasepencil:: name space, you may not added this in any places.

If you already in `blender::ed::greasepencil::` name space, you may not added this in any places.

Hans Goudey reviewed 2023-06-22 23:36:15 +02:00

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

						
				@ -0,0 +95,4 @@

				  /* Loop all strokes, looking for an intersecting segment. */

				  std::atomic<bool> intersect = false;

				  threading::parallel_for(strokes_2d.index_range(), 256, [&](const IndexRange range) {

Hans Goudey commented

2023-06-22 23:36:15 +02:00

Take a look at parallel_reduce. It's used for a similar purpose, and removes the need for an atomic. has_anything_selected also returns a boolean, so you can copy the structure with that. It's not really that different in the end, but at least it tells the reader that a reduction is happening (many computations giving one result).

Take a look at `parallel_reduce`. It's used for a similar purpose, and removes the need for an atomic. `has_anything_selected` also returns a boolean, so you can copy the structure with that. It's not really that different in the end, but at least it tells the reader that a reduction is happening (many computations giving one result).

SietseB marked this conversation as resolved

Iliya Katushenock reviewed 2023-06-22 23:43:47 +02:00

Sietse Brouwer commented

2023-06-23 13:08:34 +02:00

@HooglyBoogly Thanks for the review! I learned a lot from it already. I replied to some of your remarks – the others I will just implement.

I know the algorithm is rather poor and scales badly. I did think about using a KD tree. The thing is, finding intersections is not just about proximity of other points. You can have a line of two points across the entire viewport, the two points far away from the vector you're inspecting and still that line will intersect with the vector.
With the GP primitives like line and rectangle with low subdivision in mind, that's not a purely hypothetical problem.

So, to avoid that this task would culminate in a mathematical quest, I took the poor man's option.

@HooglyBoogly Thanks for the review! I learned a lot from it already. I replied to some of your remarks – the others I will just implement. I know the algorithm is rather poor and scales badly. I did think about using a KD tree. The thing is, finding intersections is not just about proximity of other points. You can have a line of two points across the entire viewport, the two points far away from the vector you're inspecting and still that line will intersect with the vector. With the GP primitives like line and rectangle with low subdivision in mind, that's not a purely hypothetical problem. So, to avoid that this task would culminate in a mathematical quest, I took the poor man's option.

Falk David commented

2023-06-23 14:37:59 +02:00

@SietseB I'll work on a PR to do the refactor mentioned here: #109221 (comment)
Maybe since you're on holiday anyway, this PR can be on hold in the meantime :)

EDIT: I now did this for one of the selection functions as an example of how that would work. See #109293

@SietseB I'll work on a PR to do the refactor mentioned here: https://projects.blender.org/blender/blender/pulls/109221#issuecomment-965366 Maybe since you're on holiday anyway, this PR can be on hold in the meantime :) EDIT: I now did this for one of the selection functions as an example of how that would work. See https://projects.blender.org/blender/blender/pulls/109293

Sietse Brouwer commented

2023-06-23 18:29:01 +02:00

Sounds like a good plan to me. And even better: you are already on it! ;-)

Sietse Brouwer referenced this pull request

2023-07-11 12:44:59 +02:00

GPv3: convert selection domain on curves when selection mode changes #109964

Falk David referenced this issue from a commit

2023-07-12 10:22:52 +02:00

GPv3: Convert selection domain on curves when selection mode changes

Sietse Brouwer added 4 commits 2023-07-12 23:23:20 +02:00

2aa614a0fc Merge branch 'main' into gp3-select-segments

e35145e3ff Merge branch 'main' into gp3-select-segments

8035a9b640 Merge branch 'main' into gp3-select-segments

ce1172e76a Code review changes and circle select support

Sietse Brouwer changed title from ~~WIP: GPv3: Segment selection support for all relevant selection operators~~ to GPv3: Segment selection support for all relevant selection operators

2023-07-13 01:16:25 +02:00

Sietse Brouwer commented

2023-07-13 01:20:46 +02:00

In my new commit I've:

Handled all the code review remarks. Except for the one I commented on just now.
Added segment support for the circle select operator.
Added automatic conversion of point selection to segment selection when the user changes the selection mode from 'point' to 'segment' in the tool bar.

In my new commit I've: - Handled all the code review remarks. Except for the one I commented on just now. - Added segment support for the circle select operator. - Added automatic conversion of point selection to segment selection when the user changes the selection mode from 'point' to 'segment' in the tool bar.

Hans Goudey reviewed 2023-07-14 14:58:51 +02:00

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

						
				@ -418,0 +528,4 @@

				  }

				  /* Copy selection. */

				  const VArray<bool> selection = *attributes.lookup(".selection").typed<bool>();

Hans Goudey commented

2023-07-14 14:58:51 +02:00

Would be simpler to look up the selection with a default of true like is done elsewhere, and use the virtual array's materialize function to fill the array.

Would be simpler to look up the selection with a default of true like is done elsewhere, and use the virtual array's `materialize` function to fill the array.

SietseB marked this conversation as resolved

Sietse Brouwer added 2 commits 2023-07-15 22:35:42 +02:00

c057104417 Merge branch 'main' into gp3-select-segments

fa79b1eaaf Copy selection attribute with builtin 'materialize' method

Sietse Brouwer added 3 commits 2023-11-01 18:25:12 +01:00

4cad71b604 Merge branch 'main' into gp3-select-segments

97788dba3c Cleanup after merge with main

67594a65aa Improve intersection detection when strokes are almost touching

Sietse Brouwer commented

2023-11-01 19:23:04 +01:00

@filedescriptor @HooglyBoogly
To revive this PR, I'll summarize the discussion:

Main objection to the PR is that is mixes two levels of abstraction. Expanding the point selection to segment selection is more "tacked on at the end" than it should be:

static bool do_lasso_select_grease_pencil() {
  ...
  ed::curves::select_lasso(curves, ...);
  ed::greasepencil::expand_changed_selection_to_segments(curves, ...);
}

The proposed solution to that is changing select_lasso and friends to return an IndexMask of affected points and apply the change in selection after obtaining the mask.
Applying the selection change has to be done for two cases, OB_CURVES and OB_GREASE_PENCIL. To avoid code duplication, it makes sense to put the apply-selection in a shared function, which handles all the selection operations (set, add, sub, and, xor etc.).
When we do that, we get something like:

static bool do_lasso_select_grease_pencil() {
  ...
  IndexMask affected_points = ed::curves::select_lasso(curves, ...);
  ed::curves::apply_selection_operation(curves, affected_points, sel_op);
  ed::greasepencil::expand_changed_selection_to_segments(curves, affected_points, ...);
}

Which brings us to the question: does that solve the main objection? TBH, I don't see that much of a difference. And since rewriting the selection functions to IndexMask ones, isn't a task done in 10 minutes, I prefer to be sure in advance that we are happy with the outcome of such rewrite.

@filedescriptor @HooglyBoogly To revive this PR, I'll summarize the discussion: - Main objection to the PR is that is mixes two levels of abstraction. Expanding the point selection to segment selection is more "tacked on at the end" than it should be: ``` C static bool do_lasso_select_grease_pencil() { ... ed::curves::select_lasso(curves, ...); ed::greasepencil::expand_changed_selection_to_segments(curves, ...); } ``` - The proposed solution to that is changing `select_lasso` and friends to return an `IndexMask` of affected points and apply the change in selection after obtaining the mask. - Applying the selection change has to be done for two cases, `OB_CURVES` and `OB_GREASE_PENCIL`. To avoid code duplication, it makes sense to put the apply-selection in a shared function, which handles all the selection operations (set, add, sub, and, xor etc.). - When we do that, we get something like: ``` C static bool do_lasso_select_grease_pencil() { ... IndexMask affected_points = ed::curves::select_lasso(curves, ...); ed::curves::apply_selection_operation(curves, affected_points, sel_op); ed::greasepencil::expand_changed_selection_to_segments(curves, affected_points, ...); } ``` Which brings us to the question: does that solve the main objection? TBH, I don't see that much of a difference. And since rewriting the selection functions to `IndexMask` ones, isn't a task done in 10 minutes, I prefer to be sure in advance that we are happy with the outcome of such rewrite.

This pull request has changes conflicting with the target branch.

source/blender/editors/grease_pencil/intern/grease_pencil_select.cc
source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc
source/blender/editors/include/ED_grease_pencil.hh
source/blender/editors/space_view3d/view3d_select.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u gp3-select-segments:SietseB-gp3-select-segments

git checkout SietseB-gp3-select-segments

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPv3: Segment selection support for all relevant selection operators #109221

Checkout