blender
/
blender
136
406
Fork 588
Code Issues 6.2k Pull Requests 753 Projects 19 Wiki Activity

Curves: Add support for proportional editing #104620

Closed
Falk David wants to merge 15 commits from filedescriptor:curves-proportional-editing into blender-v3.5-release
pull from: filedescriptor:curves-proportional-editing
merge into: blender:blender-v3.5-release

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 2 Commits 15 Files Changed 4 +59 -17
1 changed files with 59 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 547a196c01 - Show all commits

76
source/blender/editors/transform/transform_convert_curves.cc
View File

@ -28,6 +28,7 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
MutableSpan<TransDataContainer> trans_data_contrainers(t->data_container, t->data_container_len);
Array<Vector<int64_t>> selected_indices_per_object(t->data_container_len);
Array<IndexMask> selection_per_object(t->data_container_len);
const bool is_prop_edit = (t->flag & T_PROP_EDIT) != 0;
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-13 02:26:59 +01:00
Review
Copy Link

prop -> proportional? Curious what you think about that-- saving a few characters doesn't help so much here.

`prop` -> `proportional`? Curious what you think about that-- saving a few characters doesn't help so much here.
/* Count selected elements per object and create TransData structs. */
for (const int i : trans_data_contrainers.index_range()) {
@ -35,10 +36,15 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
Curves *curves_id = static_cast<Curves *>(tc.obedit->data);
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
selection_per_object[i] = ed::curves::retrieve_selected_points(curves,
selected_indices_per_object[i]);
if (is_prop_edit) {
tc.data_len = curves.point_num;
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-20 17:50:01 +01:00
Review
Copy Link

Tiny thing, but fairly sure this will end up in a cleanup commit by Campbell to remove the space between TODO and (Falk) :P

I'd remove your name or the space

Tiny thing, but fairly sure this will end up in a cleanup commit by Campbell to remove the space between `TODO` and `(Falk)` :P I'd remove your name or the space
}
else {
selection_per_object[i] = ed::curves::retrieve_selected_points(
curves, selected_indices_per_object[i]);
tc.data_len = selection_per_object[i].size();
}
tc.data_len = selection_per_object[i].size();
if (tc.data_len > 0) {
tc.data = MEM_cnew_array<TransData>(tc.data_len, __func__);
}
@ -52,28 +58,64 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
}
Curves *curves_id = static_cast<Curves *>(tc.obedit->data);
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
IndexMask selected_indices = selection_per_object[i];
float mtx[3][3], smtx[3][3];
copy_m3_m4(mtx, tc.obedit->object_to_world);
pseudoinverse_m3_m3(smtx, mtx, PSEUDOINVERSE_EPSILON);
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-17 19:59:51 +01:00
Review
Copy Link

is_prop_edit -> use_proportional_edit
is_prop_connected -> proportional_connected_only

Or something like that

`is_prop_edit` -> `use_proportional_edit` `is_prop_connected` -> `proportional_connected_only` Or something like that
MutableSpan<float3> positions = curves.positions_for_write();
threading::parallel_for(selected_indices.index_range(), 1024, [&](const IndexRange range) {
for (const int selection_i : range) {
TransData *td = &tc.data[selection_i];
float *elem = reinterpret_cast<float *>(&positions[selected_indices[selection_i]]);
copy_v3_v3(td->iloc, elem);
copy_v3_v3(td->center, td->iloc);
td->loc = elem;
if (is_prop_edit) {
OffsetIndices<int> points_by_curve = curves.points_by_curve();
VArray<bool> selection = curves.attributes().lookup_or_default<bool>(
".selection", ATTR_DOMAIN_POINT, true);
threading::parallel_for(curves.curves_range(), 1024, [&](const IndexRange range) {
for (const int curve_i : range) {
bool has_any_selected = false;
Hans Goudey commented 2023-02-13 02:08:09 +01:00
Review
Copy Link

What do you think about processing has_any_selected in a separate loop? It's probably possible to use one of the curves utilities with that name, right? In that case, maybe it's possible to skip the rest of the work in the curve in that case, and it probably makes the code a bit more readable.

What do you think about processing `has_any_selected` in a separate loop? It's probably possible to use one of the curves utilities with that name, right? In that case, maybe it's possible to skip the rest of the work in the curve in that case, and it probably makes the code a bit more readable.
for (const int point_i : points_by_curve[curve_i]) {
TransData *td = &tc.data[point_i];
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-13 02:06:41 +01:00
Review
Copy Link

TransData &td = tc.data[point_i];

`TransData &td = tc.data[point_i];`
float *elem = reinterpret_cast<float *>(&positions[point_i]);
copy_v3_v3(td->iloc, elem);
copy_v3_v3(td->center, td->iloc);
td->loc = elem;
td->flag = TD_SELECTED;
td->ext = nullptr;
if (selection[point_i]) {
has_any_selected = true;
}
copy_m3_m3(td->smtx, smtx);
copy_m3_m3(td->mtx, mtx);
}
});
td->flag = (selection[point_i]) ? TD_SELECTED : 0;
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-13 02:10:56 +01:00
Review
Copy Link

(selection[point_i]) -> selection[point_i]

Unnecessary parentheses

`(selection[point_i])` -> `selection[point_i]` Unnecessary parentheses
td->ext = nullptr;
copy_m3_m3(td->smtx, smtx);
copy_m3_m3(td->mtx, mtx);
}
if (!has_any_selected) {
for (const int point_i : points_by_curve[curve_i]) {
TransData *td = &tc.data[point_i];
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-13 02:19:29 +01:00
Review
Copy Link
TransData *td = &tc.data[point_i];
td->flag |= TD_NOTCONNECTED;

->

tc.data[point_i].flag |= TD_NOTCONNECTED;
``` TransData *td = &tc.data[point_i]; td->flag |= TD_NOTCONNECTED; ``` -> ``` tc.data[point_i].flag |= TD_NOTCONNECTED; ```
td->flag |= TD_NOTCONNECTED;
}
}
}
});
}
else {
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-17 20:01:36 +01:00
Review
Copy Link

Separate positions_read and positions_ptr shouldn't be necessary, the old positions span should still work fine

Separate `positions_read` and `positions_ptr` shouldn't be necessary, the old `positions` span should still work fine
Falk David commented 2023-02-18 15:48:27 +01:00
Review
Copy Link

Last time I tried this, the compiler complained because I was passing pointers to td->loc when the Span is const. So I think I need float3 *positions_ptr = curves.positions_for_write().data(); just to pass the pointers into the TransData struct.

Last time I tried this, the compiler complained because I was passing pointers to `td->loc` when the Span is const. So I think I need `float3 *positions_ptr = curves.positions_for_write().data();` just to pass the pointers into the `TransData` struct.
Hans Goudey commented 2023-02-20 18:45:35 +01:00
Review
Copy Link

Just replacing positions_ptr with a mutable span seems to work fine here

Just replacing `positions_ptr` with a mutable span seems to work fine here
IndexMask selected_indices = selection_per_object[i];
threading::parallel_for(selected_indices.index_range(), 1024, [&](const IndexRange range) {
for (const int selection_i : range) {
TransData *td = &tc.data[selection_i];
float *elem = reinterpret_cast<float *>(&positions[selected_indices[selection_i]]);
copy_v3_v3(td->iloc, elem);
copy_v3_v3(td->center, td->iloc);
td->loc = elem;
td->flag = TD_SELECTED;
td->ext = nullptr;
copy_m3_m3(td->smtx, smtx);
copy_m3_m3(td->mtx, mtx);
}
});
}
}
}
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-17 20:02:45 +01:00
Review
Copy Link

for (const int point_i : points) {

`for (const int point_i : points) {`