blender
/
blender
137
418
Fork 600
Code Issues 6.3k Pull Requests 781 Projects 19 Wiki Activity

GPv3: Select Alternate #109240

Merged
Falk David merged 6 commits from Lorenzo-Carpaneto/blender:gpv3-select-alternate into main 2023-06-23 11:34:06 +02:00
Conversation 1 Commits 6 Files Changed 4 +2 -8
1 changed files with 2 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ecaee61f57 - Show all commits

10
source/blender/editors/curves/intern/curves_selection.cc
View File

@ -288,14 +288,8 @@ void select_alternate(bke::CurvesGeometry &curves, const bool deselect_ends)
continue;
Lorenzo-Carpaneto marked this conversation as resolved Outdated
Falk David commented 2023-06-22 14:23:29 +02:00
Outdated
Review
Copy Link

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; ```
}
bool prev_selected = deselect_ends;
for (int point_i = points.first(); point_i <= points.last(); point_i++) {
if (prev_selected) {
selection_typed[point_i] = prev_selected = false;
}
else {
selection_typed[point_i] = prev_selected = true;
}
for (const int offset : IndexRange(points.size())) {
Lorenzo-Carpaneto marked this conversation as resolved Outdated
Falk David commented 2023-06-22 14:47:19 +02:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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)

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link
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
Outdated
Review
Copy Link

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

@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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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

@SietseB is right, the for loop can stay as is.
Iliya Katushenock commented 2023-06-22 17:40:34 +02:00
Outdated
Review
Copy Link

@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
Outdated
Review
Copy Link

@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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

@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 :)

@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 :)
selection_typed[points.first() + offset] = deselect_ends ? offset % 2 : !(offset % 2);
Lorenzo-Carpaneto marked this conversation as resolved Outdated
Hans Goudey commented 2023-06-22 18:26:33 +02:00
Outdated
Review
Copy Link

for (const int point : points) {

`for (const int point : points) {`
Lorenzo-Carpaneto commented 2023-06-22 18:29:09 +02:00
Outdated
Review
Copy Link

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

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
Outdated
Review
Copy Link

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)
Iliya Katushenock commented 2023-06-22 23:57:07 +02:00
Outdated
Review
Copy Link
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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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

Ah, you are totally right, pushing the proposed version, it's a bit more readable and efficient :)
}
if (cyclic[curve_i]) {