blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 766 Projects 19 Wiki Activity

Curves: Add select more/less #104626

Merged
Falk David merged 7 commits from filedescriptor/blender:curves-select-more-less into main 2023-02-16 17:02:54 +01:00
Conversation 0 Commits 7 Files Changed 5 +23 -4
1 changed files with 23 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 179d4bcd2f - Show all commits

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

@ -325,6 +325,7 @@ void select_adjacent(bke::CurvesGeometry &curves, const bool deselect)
const OffsetIndices points_by_curve = curves.points_by_curve();
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-16 14:12:14 +01:00
Review
Copy Link

I think a local const IndexRange points variable to hold the curve points range would make this slightly more readable.

I think a local `const IndexRange points` variable to hold the curve points range would make this slightly more readable.
curves, ATTR_DOMAIN_POINT, CD_PROP_BOOL);
VArray<bool> cyclic = curves.cyclic();
if (deselect) {
invert_selection(selection.span);
@ -335,20 +336,29 @@ void select_adjacent(bke::CurvesGeometry &curves, const bool deselect)
threading::parallel_for(curves.curves_range(), 256, [&](const IndexRange range) {
for (const int curve_i : range) {
MutableSpan<bool> selection_curve = selection_typed.slice(points_by_curve[curve_i]);
const int last_i = points_by_curve.size(curve_i) - 1;
/* Handle all cases in the forward direction. */
for (int point_i = 0; point_i < points_by_curve.size(curve_i) - 1; point_i++) {
for (int point_i = 0; point_i < last_i; point_i++) {
if (!selection_curve[point_i] && selection_curve[point_i + 1]) {
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-16 14:15:10 +01:00
Review
Copy Link

Nice, that's simpler than I expected. Only needing to propagate it by one element helps!

Nice, that's simpler than I expected. Only needing to propagate it by one element helps!
👍 1
selection_curve[point_i] = true;
}
}
/* Handle all cases in the backwards direction. */
for (int point_i = points_by_curve.size(curve_i) - 1; point_i > 0; point_i--) {
for (int point_i = last_i; point_i > 0; point_i--) {
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-13 02:02:58 +01:00
Review
Copy Link

Looks like this doesn't handle cyclic curves. For some similar logic, ControlPointNeighborFieldInput might be worth checking out.

IMO, this is pushing the boundaries of what should be done with dynamic types per index, since there's 2 reads per index. But I strongly doubt the selection operators will noticeably non-instantaneous, and it can always be improved later.

Looks like this doesn't handle cyclic curves. For some similar logic, `ControlPointNeighborFieldInput` _might_ be worth checking out. IMO, this is pushing the boundaries of what should be done with dynamic types per index, since there's 2 reads per index. But I strongly doubt the selection operators will noticeably non-instantaneous, and it can always be improved later.
if (!selection_curve[point_i] && selection_curve[point_i - 1]) {
selection_curve[point_i] = true;
}
}
/* Handle cyclic curve case. */
if (cyclic[curve_i]) {
if (selection_curve[0] != selection_curve[last_i]) {
selection_curve[0] = true;
selection_curve[last_i] = true;
}
}
}
});
}
@ -357,20 +367,29 @@ void select_adjacent(bke::CurvesGeometry &curves, const bool deselect)
threading::parallel_for(curves.curves_range(), 256, [&](const IndexRange range) {
for (const int curve_i : range) {
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-16 14:14:45 +01:00
Review
Copy Link

This mix of using selection_typed and selection_curve is confusing, it's hard to tell what's correct here.

If it's possible, I'd suggest only using actual point indices (rather than 0..points.size() for each curve). Having a local IndexRange variable with the .last(N) method should make that simpler.

This mix of using `selection_typed` and `selection_curve` is confusing, it's hard to tell what's correct here. If it's possible, I'd suggest only using actual point indices (rather than 0..points.size() for each curve). Having a local `IndexRange` variable with the `.last(N)` method should make that simpler.
Falk David commented 2023-02-16 14:23:41 +01:00
Review
Copy Link

Ah good catch, that's actually a bug I was trying to fix. It didn't work with float selection, so I think that is why.

Ah good catch, that's actually a bug I was trying to fix. It didn't work with float selection, so I think that is why.
MutableSpan<float> selection_curve = selection_typed.slice(points_by_curve[curve_i]);
const int last_i = points_by_curve.size(curve_i) - 1;
/* Handle all cases in the forward direction. */
for (int point_i = 0; point_i < points_by_curve.size(curve_i) - 1; point_i++) {
for (int point_i = 0; point_i < last_i; point_i++) {
if ((selection_typed[point_i] == 0.0f) && (selection_typed[point_i + 1] > 0.0f)) {
selection_curve[point_i] = 1.0f;
}
}
/* Handle all cases in the backwards direction. */
for (int point_i = points_by_curve.size(curve_i) - 1; point_i > 0; point_i--) {
for (int point_i = last_i; point_i > 0; point_i--) {
if ((selection_typed[point_i] == 0.0f) && (selection_typed[point_i - 1] > 0.0f)) {
selection_curve[point_i] = 1.0f;
}
}
/* Handle cyclic curve case. */
if (cyclic[curve_i]) {
if (selection_curve[0] != selection_curve[last_i]) {
selection_curve[0] = 1.0f;
selection_curve[last_i] = 1.0f;
}
}
}
});
}