blender
/
blender
136
406
Fork 588
Code Issues 6.2k Pull Requests 751 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 +43 -34
1 changed files with 43 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 34c5ac1c59 - Show all commits

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

@ -25,33 +25,33 @@
namespace blender::ed::transform::curves {
static void calculate_curve_point_distances_for_proportional_editing(
Span<float3> positions, MutableSpan<float> r_distances)
const Span<float3> positions, MutableSpan<float> r_distances)
{
Array<bool> visited(positions.size(), false);
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.
InplacePriorityQueue<float, std::less<float>> queue(r_distances);
while (!queue.is_empty()) {
int64_t idx = queue.pop_index();
if (visited[idx]) {
int64_t index = queue.pop_index();
if (visited[index]) {
continue;
}
visited[idx] = true;
visited[index] = true;
/* TODO (Falk): Handle cyclic curves here. */
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
if (idx > 0 && !visited[idx - 1]) {
int adj = idx - 1;
float dist = r_distances[idx] + math::distance(positions[idx], positions[adj]);
if (dist < r_distances[adj]) {
r_distances[adj] = dist;
queue.priority_changed(adj);
if (index > 0 && !visited[index - 1]) {
int adjacent = index - 1;
float dist = r_distances[index] + math::distance(positions[index], positions[adjacent]);
if (dist < r_distances[adjacent]) {
r_distances[adjacent] = dist;
queue.priority_changed(adjacent);
}
}
if (idx < positions.size() - 1 && !visited[idx + 1]) {
int adj = idx + 1;
float dist = r_distances[idx] + math::distance(positions[idx], positions[adj]);
if (dist < r_distances[adj]) {
r_distances[adj] = dist;
queue.priority_changed(adj);
if (index < positions.size() - 1 && !visited[index + 1]) {
int adjacent = index + 1;
float dist = r_distances[index] + math::distance(positions[index], positions[adjacent]);
if (dist < r_distances[adjacent]) {
r_distances[adjacent] = dist;
queue.priority_changed(adjacent);
}
}
}
@ -98,7 +98,7 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
copy_m3_m4(mtx, tc.obedit->object_to_world);
pseudoinverse_m3_m3(smtx, mtx, PSEUDOINVERSE_EPSILON);
MutableSpan<float3> positions = curves.positions_for_write();
float3 *positions_ptr = curves.positions_for_write().data();
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
if (is_prop_edit) {
const Span<float3> positions_read = curves.positions();
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
@ -107,22 +107,38 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
threading::parallel_for(curves.curves_range(), 512, [&](const IndexRange range) {
for (const int curve_i : range) {
bool has_any_selected = false;
const IndexRange points = points_by_curve[curve_i];
for (const int i : IndexRange(points.size())) {
const int point_i = points[i];
if (selection[point_i]) {
has_any_selected = true;
break;
}
}
if (!has_any_selected) {
for (const int point_i : points_by_curve[curve_i]) {
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) {`
TransData &td = tc.data[point_i];
td.flag |= TD_NOTCONNECTED;
td.dist = FLT_MAX;
}
continue;
}
const Span<float3> positions_curve = positions_read.slice(points_by_curve[curve_i]);
Array<float> closest_distances(positions_curve.size(), FLT_MAX);
for (const int i : IndexRange(points.size())) {
filedescriptor marked this conversation as resolved
Hans Goudey commented 2023-02-17 20:05:22 +01:00
Review
Copy Link

This array makes things a bit more readable, but it isn't really necessary considering all the data will go into TransData.dist right after. It might be worth skipping the array, since this will be an allocation and free for every single curve.

This array makes things a bit more readable, but it isn't really necessary considering all the data will go into `TransData.dist` right after. It might be worth skipping the array, since this will be an allocation and free for every single curve.
Falk David commented 2023-02-18 15:44:07 +01:00
Review
Copy Link

I am not sure how I would rewrite the code to not use an array in this case. The td->dist values are not sequential, so I can't write to them directly. But the implementation needs some container that I can build the InplacePriorityQueue on top of.

I am not sure how I would rewrite the code to not use an array in this case. The `td->dist` values are not sequential, so I can't write to them directly. But the implementation needs some container that I can build the `InplacePriorityQueue` on top of.
Hans Goudey commented 2023-02-18 16:40:01 +01:00
Review
Copy Link

Ah right! That's totally fine, it was just a thought.

Ah right! That's totally fine, it was just a thought.
const int point_i = points[i];
TransData &td = tc.data[point_i];
float *elem = reinterpret_cast<float *>(&positions[point_i]);
copy_v3_v3(td.iloc, elem);
float3 *elem = &positions_ptr[point_i];
copy_v3_v3(td.iloc, *elem);
copy_v3_v3(td.center, td.iloc);
td.loc = elem;
td.loc = *elem;
td.flag = 0;
if (selection[point_i]) {
has_any_selected = true;
closest_distances[i] = 0.0f;
td.flag = TD_SELECTED;
}
@ -133,7 +149,7 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
copy_m3_m3(td.mtx, mtx);
}
if (is_prop_connected && has_any_selected) {
if (is_prop_connected) {
calculate_curve_point_distances_for_proportional_editing(
positions_curve, closest_distances.as_mutable_span());
for (const int i : IndexRange(points.size())) {
@ -141,14 +157,6 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
td.dist = closest_distances[i];
}
}
if (!has_any_selected) {
for (const int point_i : points_by_curve[curve_i]) {
TransData &td = tc.data[point_i];
td.flag |= TD_NOTCONNECTED;
td.dist = FLT_MAX;
}
}
}
});
}
@ -157,10 +165,11 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
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);
float3 *elem = &positions_ptr[selected_indices[selection_i]];
copy_v3_v3(td->iloc, *elem);
copy_v3_v3(td->center, td->iloc);
td->loc = elem;
td->loc = *elem;
td->flag = TD_SELECTED;
td->ext = nullptr;