GPv3: Select Alternate #109240

Lorenzo-Carpaneto · 2023-06-22T14:11:38+02:00

Lorenzo-Carpaneto commented

2023-06-22 14:11:38 +02:00

Adds a "Select Alternate" operator for the new Grease Pencil data type.
This replaces the previous gpencil.select_alternate.

Resolves #109204.

Adds a "Select Alternate" operator for the new Grease Pencil data type. This replaces the previous `gpencil.select_alternate`. Resolves #109204.

Iliya Katushenock added this to the Grease Pencil project 2023-06-22 14:12:55 +02:00

Falk David requested review from Falk David 2023-06-22 14:15:33 +02:00

Falk David requested changes 2023-06-22 14:27:27 +02:00

Falk David left a comment

A few initial comments.

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

						
				@ -275,0 +278,4 @@

				      curves, ATTR_DOMAIN_POINT, CD_PROP_BOOL);

				  const VArray<bool> cyclic = curves.cyclic();

				  BLI_assert_msg(selection.span.type().is<bool>(), "Error: Cannot alternate select on selection type different than bool.");

Falk David commented

2023-06-22 14:20:21 +02:00

The ensure_selection_attribute call already makes sure we have a bool attribute. No need for the assert here.

The `ensure_selection_attribute` call already makes sure we have a `bool` attribute. No need for the assert here.

Hans Goudey commented

2023-06-22 15:44:50 +02:00

It doesn't actually. The type argument is only used when the selection attribute doesn't exist yet. So if the selection already has a float type, this operator will have to support that or change the attribute's type to boolean.

😆 1

Lorenzo-Carpaneto commented

2023-06-22 16:50:36 +02:00

what do we want to do? We could template the function and support both selection; we could change the selection type, we could do nothing and prevent the user to use this if the type of selection isn't bool...

Falk David commented

2023-06-22 17:21:36 +02:00

Since there is no exposed way of creating a float selection at the moment, I would just remove the assert and leave it as is.

Since there is no exposed way of creating a `float` selection at the moment, I would just remove the assert and leave it as is.

Lorenzo-Carpaneto commented

2023-06-22 17:30:23 +02:00

Then we are ready to merge this pr I think :)

Lorenzo-Carpaneto marked this conversation as resolved

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

						
				@ -275,0 +285,4 @@

				    for (const int curve_i : range) {

				      const IndexRange points = points_by_curve[curve_i];

				      if (has_anything_selected(selection.span.slice(points))) {

Falk David commented

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

I think it make sense to avoid the indentation and continue early.

if (!has_anything_selected(selection.span.slice(points))) {
   continue;

I think it make sense to avoid the indentation and continue early. ``` if (!has_anything_selected(selection.span.slice(points))) { continue; ```

Lorenzo-Carpaneto marked this conversation as resolved

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

						
				@ -275,0 +311,4 @@

				          }

				        }

				        else {

				          if (!select_ends)

Falk David commented

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

Missing brackets.
For formatting issues like these, please make sure to use clang-format. See https://wiki.blender.org/wiki/Tools/ClangFormat

Missing brackets. For formatting issues like these, please make sure to use `clang-format`. See https://wiki.blender.org/wiki/Tools/ClangFormat

Lorenzo-Carpaneto marked this conversation as resolved

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

						
				@ -198,0 +233,4 @@

				                  "select_ends",

				                  true,

				                  "Select Ends",

				                  "Select the first and enable the selection of the last point of each stroke");

Falk David commented

2023-06-22 14:27:07 +02:00

"(De)select the first and last point of each stroke"

Lorenzo-Carpaneto commented

2023-06-22 14:34:01 +02:00

This description is not precise though, because we select the first point but the last point is selected only if we get there alternated (e.g. if we have two points we select only the first one....).
Do I change the description anyway?

This description is not precise though, because we select the first point but the last point is selected only if we get there alternated (e.g. if we have two points we select only the first one....). Do I change the description anyway?

Falk David commented

2023-06-22 14:44:34 +02:00

Hm looking at the old operator, I think it makes sense to keep the same behavior. What we should do is call this option deselect_ends (everywhere it's used).
If deselect_ends is true, then we just deselect the fist and last point. Otherwise we leave the selection as is.
Then the description would be "Deselect the first and last point of each stroke".

Hm looking at the old operator, I think it makes sense to keep the same behavior. What we should do is call this option `deselect_ends` (everywhere it's used). If `deselect_ends` is true, then we just deselect the fist and last point. Otherwise we leave the selection as is. Then the description would be "Deselect the first and last point of each stroke".

Lorenzo-Carpaneto commented

2023-06-22 14:54:38 +02:00

So right now it is coded as the old behavior, we just need to decide on description and if we want to use select_ends or deselect_ends :)
I will make it as similar as the old version as possible, thus using deselect_ends

So right now it is coded as the old behavior, we just need to decide on description and if we want to use select_ends or deselect_ends :) I will make it as similar as the old version as possible, thus using deselect_ends

Lorenzo-Carpaneto marked this conversation as resolved

Falk David reviewed 2023-06-22 14:47:19 +02:00

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

						
				@ -275,0 +288,4 @@

				      if (has_anything_selected(selection.span.slice(points))) {

				        bool prev_selected = !select_ends;

				        for (int point_i = points.first(); point_i <= points.last(); point_i++) {

Falk David commented

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

I think this loop can now be simplified:

for (const int point_i : points.index_range()) {
   selection[point_i] = (point_i % 2 == 0);
}

I think this loop can now be simplified: ``` for (const int point_i : points.index_range()) { selection[point_i] = (point_i % 2 == 0); } ```

Lorenzo-Carpaneto commented

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

mmm, this is not entirely right I think: the select ends param makes us start or from first or from second point... So the selection is kind of "inverted" based on this params (I am basing this fact on the older version of gp, that works like this)

Falk David commented

2023-06-22 15:07:44 +02:00

Ah then you use (point_i % 2 == unselect_ends) instead.

Ah then you use `(point_i % 2 == unselect_ends)` instead.

Sietse Brouwer commented

2023-06-22 15:08:39 +02:00

It's weird, but I noticed we can't use points.index_range(), because it always starts at 0. While points.first() gives the correct offset.
So keeping
for (int point_i = points.first(); point_i <= points.last(); point_i++) {
is the right thing to do.

It's weird, but I noticed we can't use `points.index_range()`, because it always starts at `0`. While `points.first()` gives the correct offset. So keeping ` for (int point_i = points.first(); point_i <= points.last(); point_i++) {` is the right thing to do.

👍 2

Iliya Katushenock commented

2023-06-22 15:10:53 +02:00

const int half_of_size = points.size() / 2;
const IndexRange selected = points.shift(select_ends ? 0 : 1);
const IndexRange deselected = points.shift(select_ends ? 1 : 0);
for (const int index : IndexRange(half_of_size)) {
  const int selected_index = selected[index * 2];
  const int deselected_index = deselected[index * 2];
  selection_typed[selected_index] = true;
  selection_typed[deselected_index] = false;
}
selection_typed.last() = select_ends;

Maybe something like this?

```Cpp const int half_of_size = points.size() / 2; const IndexRange selected = points.shift(select_ends ? 0 : 1); const IndexRange deselected = points.shift(select_ends ? 1 : 0); for (const int index : IndexRange(half_of_size)) { const int selected_index = selected[index * 2]; const int deselected_index = deselected[index * 2]; selection_typed[selected_index] = true; selection_typed[deselected_index] = false; } selection_typed.last() = select_ends; ``` Maybe something like this?

Lorenzo-Carpaneto commented

2023-06-22 15:12:22 +02:00

@mod_moder to me this seems more difficult to read than what I had originally written though...

Lorenzo-Carpaneto commented

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

Ah then you use (point_i % 2 == unselect_ends) instead.

Are we entirely sure that the first point is always 0? Because I see that index_range has a start_ init param

> Ah then you use `(point_i % 2 == unselect_ends)` instead. Are we entirely sure that the first point is always 0? Because I see that index_range has a start_ init param

Falk David commented

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

@SietseB is right, the for loop can stay as is.

Iliya Katushenock commented

2023-06-22 17:40:34 +02:00

@Lorenzo-Carpaneto The main reason for my approach is that it is:

It doesn't require you to calculate each step of the loop in your head that you know the result.
It can be multi-threaded. Remember, these arrays can be millions of points long.

@Lorenzo-Carpaneto The main reason for my approach is that it is: - It doesn't require you to calculate each step of the loop in your head that you know the result. - It can be multi-threaded. Remember, these arrays can be millions of points long.

Lorenzo-Carpaneto commented

2023-06-22 17:46:45 +02:00

@mod_moder These are good motivations, but I think there might be problems with your solution.
@SietseB and @filedescriptor said that points.index_range() is not working (and that function returns: IndexRange(size_);. So I fear that your approach suffers from the same bug. So for now maybe better keep the loop as is, and as soon as we fix the problem with iteration on indexRange, we change the code

@mod_moder These are good motivations, but I think there might be problems with your solution. @SietseB and @filedescriptor said that `points.index_range()` is not working (and that function returns: `IndexRange(size_);`. So I fear that your approach suffers from the same bug. So for now maybe better keep the loop as is, and as soon as we fix the problem with iteration on indexRange, we change the code

Iliya Katushenock commented

2023-06-22 18:23:42 +02:00

IndexRange is iterator factory itself. If you use index_range() method, you will create new IndexRange for iterating on something. In my case, points sampled directly, look at const IndexRange selected = points.shift(select_ends ? 0 : 1);... lines.

IndexRange is iterator factory itself. If you use `index_range()` method, you will create new IndexRange for iterating on something. In my case, `points` sampled directly, look at `const IndexRange selected = points.shift(select_ends ? 0 : 1);...` lines.

Lorenzo-Carpaneto commented

2023-06-22 18:26:27 +02:00

I was referring to for (const int index : IndexRange(half_of_size))

I was referring to `for (const int index : IndexRange(half_of_size))`

Iliya Katushenock commented

2023-06-22 18:27:44 +02:00

const int selected_index = selected[index * 2];
You can sampling IndexRage to maping indices.

`const int selected_index = selected[index * 2];` You can sampling IndexRage to maping indices.

Lorenzo-Carpaneto commented

2023-06-22 23:54:53 +02:00

@mod_moder @HooglyBoogly I looked better at the problem that @SietseB was saying about IndexRange and I am now convinced that the solution proposed should work :) I have made the change (a bit different from suggestion but basically the same) check it out :)

Lorenzo-Carpaneto marked this conversation as resolved

Lorenzo-Carpaneto force-pushed gpv3-select-alternate from 38ba283662 to c66efbd41b

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

Compare

Lorenzo-Carpaneto requested review from Falk David 2023-06-22 15:30:15 +02:00

Hans Goudey reviewed 2023-06-22 18:26:43 +02:00

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

						
				@ -275,0 +289,4 @@

				      }

				      bool prev_selected = deselect_ends;

				      for (int point_i = points.first(); point_i <= points.last(); point_i++) {

Hans Goudey commented

2023-06-22 18:26:33 +02:00

for (const int point : points) {

`for (const int point : points) {`

Lorenzo-Carpaneto commented

2023-06-22 18:29:09 +02:00

We are just discussing about this in the other thread, please join in the dicussion :D

Hans Goudey commented

2023-06-22 18:32:21 +02:00

Ah sorry, haha
Think I agree with Iliya in principle (whenever possible, better to write loops in a parallel way, so they have no dependence on the previous iteration)

Ah sorry, haha Think I agree with Iliya in principle (whenever possible, better to write loops in a parallel way, so they have no dependence on the previous iteration)

Lorenzo-Carpaneto marked this conversation as resolved

Lorenzo-Carpaneto added 1 commit 2023-06-22 23:52:04 +02:00

ecaee61f57 Make loop more clear and parallelizable

Iliya Katushenock reviewed 2023-06-22 23:57:07 +02:00

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

						
				@ -275,0 +289,4 @@

				      }

				      for (const int offset : IndexRange(points.size())) {

				        selection_typed[points.first() + offset] = deselect_ends ? offset % 2 : !(offset % 2);

Iliya Katushenock commented

2023-06-22 23:57:07 +02:00

for (const int index : points.index_range()) {
  selection_typed[points[index]] = deselect_ends ? index % 2 : !(index % 2);

This one is exactly the same

```Cpp for (const int index : points.index_range()) { selection_typed[points[index]] = deselect_ends ? index % 2 : !(index % 2); ``` This one is exactly the same

Lorenzo-Carpaneto commented

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

According to @SietseB (previous comment in the other thread)

It's weird, but I noticed we can't use points.index_range(), because it always starts at 0. While points.first() gives the correct offset.

This way should avoid this problem (cause we need to start from 0)

According to @SietseB (previous comment in the other thread) ``` It's weird, but I noticed we can't use points.index_range(), because it always starts at 0. While points.first() gives the correct offset. ``` This way should avoid this problem (cause we need to start from 0)

Iliya Katushenock commented

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

IndexRange(points.size()) == points.index_range()
points.first() + 0..1..2... == points[0...1...2...]

`IndexRange(points.size())` == `points.index_range()` `points.first() + 0..1..2...` == `points[0...1...2...]`

Lorenzo-Carpaneto commented

2023-06-23 00:09:58 +02:00

Ah, you are totally right, pushing the proposed version, it's a bit more readable and efficient :)

Lorenzo-Carpaneto marked this conversation as resolved

Lorenzo-Carpaneto added 1 commit 2023-06-23 00:45:39 +02:00

465d3d258c loop more readable

Falk David requested changes 2023-06-23 10:49:50 +02:00

Falk David left a comment

Looks very close now! Just one comment on the todo.

Looks very close now! Just one comment on the `todo`.

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

						
				@ -198,0 +205,4 @@

				  grease_pencil.foreach_editable_drawing(

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

				        // todo: check that some points of stroke are selected

Falk David commented

2023-06-23 10:49:15 +02:00

We can do this with ed::curves::has_anything_selected. This check should probably be done inside of select_alternate though and return early if nothing in the entire CurvesGeometry is selected.

We can do this with `ed::curves::has_anything_selected`. This check should probably be done inside of `select_alternate` though and return early if nothing in the entire `CurvesGeometry` is selected.

Lorenzo-Carpaneto commented

2023-06-23 11:07:37 +02:00

The check is already there, just forgot the comment :(

Falk David commented

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

The check inside the loop is fine, what I mean is that there should be a check at the top of the function like this:

if (!has_anything_selected(curves)) {
   return;
}

The check inside the loop is fine, what I mean is that there should be a check at the top of the function like this: ``` if (!has_anything_selected(curves)) { return; } ```

Lorenzo-Carpaneto marked this conversation as resolved

Lorenzo-Carpaneto added 1 commit 2023-06-23 11:12:27 +02:00

3569c9d0d2 Remove useless comment

Falk David commented

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

@Lorenzo-Carpaneto The check inside the loop is fine, what I meant is that there should be a check at the top of the function like this:

if (!has_anything_selected(curves)) {
   return;
}

@Lorenzo-Carpaneto The check inside the loop is fine, what I meant is that there should be a check at the top of the function like this: ``` if (!has_anything_selected(curves)) { return; } ```

Lorenzo-Carpaneto added 1 commit 2023-06-23 11:33:02 +02:00

b6523945bc return early if nothing selected

Falk David approved these changes 2023-06-23 11:33:37 +02:00

Falk David left a comment

Looking good!

Falk David merged commit fdc0402a50 into main

2023-06-23 11:34:06 +02:00

Falk David referenced this issue from a commit

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

GPv3: Select Alternate

Pratik Borhade reviewed 2023-06-24 11:57:21 +02:00

source/blender/editors/curves/intern/curves_selection.cc

						
				@ -275,0 +280,4 @@

				  const OffsetIndices points_by_curve = curves.points_by_curve();

				  bke::GSpanAttributeWriter selection = ensure_selection_attribute(

				      curves, ATTR_DOMAIN_POINT, CD_PROP_BOOL);

Pratik Borhade commented

2023-06-24 11:57:07 +02:00

Hi, looks like this operator is only supported in point selection mode.
In that case, we could change the poll function to editable_grease_pencil_point_selection_poll

Hi, looks like this operator is only supported in point selection mode. In that case, we could change the poll function to `editable_grease_pencil_point_selection_poll`

👍 1

Falk David commented

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

That is correct, yes. Should be changed.

👍 2

Pratik Borhade commented

2023-06-24 13:35:25 +02:00

I'll commit then directly? (or want a PR again?)

Edit: pushed to main: 51a5be8913

I'll commit then directly? (or want a PR again?) Edit: pushed to main: 51a5be891308e174e2bd8bc862bdde41545b08d1

❤️ 1

Falk David commented