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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
4 changed files with 129 additions and 24 deletions

View File

@ -123,7 +123,7 @@ void fill_selection_true(GMutableSpan selection)
}
}
static bool contains(const VArray<bool> &varray, const bool value)
static bool contains(const VArray<bool> &varray, const IndexRange range_to_check, const bool value)
{
const CommonVArrayInfo info = varray.common_info();
if (info.type == CommonVArrayInfo::Type::Single) {
@ -132,7 +132,7 @@ static bool contains(const VArray<bool> &varray, const bool value)
if (info.type == CommonVArrayInfo::Type::Span) {
const Span<bool> span(static_cast<const bool *>(info.data), varray.size());
return threading::parallel_reduce(
span.index_range(),
range_to_check,
4096,
false,
[&](const IndexRange range, const bool init) {
@ -141,7 +141,7 @@ static bool contains(const VArray<bool> &varray, const bool value)
[&](const bool a, const bool b) { return a || b; });
}
return threading::parallel_reduce(
varray.index_range(),
range_to_check,
2048,
false,
[&](const IndexRange range, const bool init) {
@ -159,10 +159,15 @@ static bool contains(const VArray<bool> &varray, const bool value)
[&](const bool a, const bool b) { return a || b; });
}
bool has_anything_selected(const VArray<bool> &varray, const IndexRange range_to_check)
{
return contains(varray, range_to_check, true);
}
bool has_anything_selected(const bke::CurvesGeometry &curves)
{
const VArray<bool> selection = curves.attributes().lookup<bool>(".selection");
return !selection || contains(selection, true);
return !selection || contains(selection, curves.curves_range(), true);
}
bool has_anything_selected(const GSpan selection)

View File

@ -93,6 +93,7 @@ bool has_anything_selected(const bke::CurvesGeometry &curves);
* Return true if any element in the span is selected, on either domain with either type.
*/
bool has_anything_selected(GSpan selection);
bool has_anything_selected(const VArray<bool> &varray, IndexRange range_to_check);
/**
* Find curves that have any point selected (a selection factor greater than zero),

View File

@ -758,8 +758,8 @@ static void init_proportional_edit(TransInfo *t)
else if (t->data_type == &TransConvertType_MeshUV && t->flag & T_PROP_CONNECTED) {
/* Already calculated by uv_set_connectivity_distance. */
}
else if (t->data_type == &TransConvertType_Curve) {
BLI_assert(t->obedit_type == OB_CURVES_LEGACY);
else if (ELEM(t->data_type, &TransConvertType_Curve, &TransConvertType_Curves)) {
BLI_assert(t->obedit_type == OB_CURVES_LEGACY || t->obedit_type == OB_CURVES);
set_prop_dist(t, false);
}
else {

View File

@ -6,6 +6,7 @@
#include "BLI_array.hh"
#include "BLI_index_mask_ops.hh"
#include "BLI_inplace_priority_queue.hh"
#include "BLI_span.hh"
#include "BKE_curves.hh"
@ -23,11 +24,46 @@
namespace blender::ed::transform::curves {
static void calculate_curve_point_distances_for_proportional_editing(
const Span<float3> positions, MutableSpan<float> r_distances)
{
Array<bool, 32> visited(positions.size(), false);
filedescriptor marked this conversation as resolved
Review

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 index = queue.pop_index();
if (visited[index]) {
continue;
}
visited[index] = true;
/* TODO(Falk): Handle cyclic curves here. */
filedescriptor marked this conversation as resolved
Review

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 (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 (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);
}
}
}
}
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 use_proportional_edit = (t->flag & T_PROP_EDIT_ALL) != 0;
filedescriptor marked this conversation as resolved
Review

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
const bool use_connected_only = (t->flag & T_PROP_CONNECTED) != 0;
/* Count selected elements per object and create TransData structs. */
for (const int i : trans_data_contrainers.index_range()) {
@ -35,10 +71,15 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
Curves *curves_id = static_cast<Curves *>(tc.obedit->data);
bke::CurvesGeometry &curves = curves_id->geometry.wrap();
Review

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.
selection_per_object[i] = ed::curves::retrieve_selected_points(curves,
selected_indices_per_object[i]);
if (use_proportional_edit) {
tc.data_len = curves.point_num;
filedescriptor marked this conversation as resolved
Review

TransData &td = tc.data[point_i];

`TransData &td = tc.data[point_i];`
}
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__);
}
filedescriptor marked this conversation as resolved
Review

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

Unnecessary parentheses

`(selection[point_i])` -> `selection[point_i]` Unnecessary parentheses
@ -52,34 +93,92 @@ static void createTransCurvesVerts(bContext * /*C*/, TransInfo *t)
}
Curves *curves_id = static_cast<Curves *>(tc.obedit->data);
filedescriptor marked this conversation as resolved
Review
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; ```
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);
MutableSpan<float3> positions = curves.positions_for_write();
filedescriptor marked this conversation as resolved
Review

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
Review

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.
Review

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
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 (use_proportional_edit) {
const OffsetIndices<int> points_by_curve = curves.points_by_curve();
const VArray<bool> selection = curves.attributes().lookup_or_default<bool>(
".selection", ATTR_DOMAIN_POINT, true);
threading::parallel_for(curves.curves_range(), 512, [&](const IndexRange range) {
Vector<float> closest_distances;
for (const int curve_i : range) {
const IndexRange points = points_by_curve[curve_i];
const bool has_any_selected = ed::curves::has_anything_selected(selection, points);
if (!has_any_selected) {
for (const int point_i : points) {
TransData &td = tc.data[point_i];
td.flag |= TD_NOTCONNECTED;
td.dist = FLT_MAX;
}
if (use_connected_only) {
continue;
}
}
filedescriptor marked this conversation as resolved
Review

for (const int point_i : points) {

`for (const int point_i : points) {`
td->flag = TD_SELECTED;
td->ext = nullptr;
closest_distances.reinitialize(points.size());
closest_distances.fill(std::numeric_limits<float>::max());
copy_m3_m3(td->smtx, smtx);
copy_m3_m3(td->mtx, mtx);
}
});
for (const int i : IndexRange(points.size())) {
const int point_i = points[i];
TransData &td = tc.data[point_i];
float3 *elem = &positions[point_i];
copy_v3_v3(td.iloc, *elem);
copy_v3_v3(td.center, td.iloc);
filedescriptor marked this conversation as resolved
Review

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.
Review

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.
Review

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

Ah right! That's totally fine, it was just a thought.
td.loc = *elem;
td.flag = 0;
if (selection[point_i]) {
closest_distances[i] = 0.0f;
td.flag = TD_SELECTED;
}
td.ext = nullptr;
copy_m3_m3(td.smtx, smtx);
copy_m3_m3(td.mtx, mtx);
}
if (use_connected_only) {
calculate_curve_point_distances_for_proportional_editing(
positions.slice(points), closest_distances.as_mutable_span());
for (const int i : IndexRange(points.size())) {
TransData &td = tc.data[points[i]];
td.dist = closest_distances[i];
}
}
}
});
}
else {
const 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];
float3 *elem = &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);
}
});
}
}
}
static void recalcData_curves(TransInfo *t)
{
Span<TransDataContainer> trans_data_contrainers(t->data_container, t->data_container_len);
const Span<TransDataContainer> trans_data_contrainers(t->data_container, t->data_container_len);
for (const TransDataContainer &tc : trans_data_contrainers) {
Curves *curves_id = static_cast<Curves *>(tc.obedit->data);
bke::CurvesGeometry &curves = curves_id->geometry.wrap();