Fix #105430: Curves pick select selects multiple objects #105495

Merged
Hans Goudey merged 5 commits from HooglyBoogly/blender:fix-curves-pick-select-multi-object into blender-v3.5-release 2023-03-07 21:39:58 +01:00
3 changed files with 154 additions and 114 deletions

View File

@ -167,7 +167,7 @@ bool has_anything_selected(const VArray<bool> &varray, const IndexRange range_to
bool has_anything_selected(const bke::CurvesGeometry &curves)
{
const VArray<bool> selection = curves.attributes().lookup<bool>(".selection");
return !selection || contains(selection, curves.curves_range(), true);
return !selection || contains(selection, selection.index_range(), true);
}
bool has_anything_selected(const GSpan selection)
@ -321,9 +321,9 @@ void select_random(bke::CurvesGeometry &curves,
selection.finish();
}
static void apply_selection_operation_at_index(GMutableSpan selection,
const int index,
const eSelectOp sel_op)
void apply_selection_operation_at_index(GMutableSpan selection,
const int index,
const eSelectOp sel_op)
{
if (selection.type().is<bool>()) {
MutableSpan<bool> selection_typed = selection.typed<bool>();
@ -361,14 +361,15 @@ static void apply_selection_operation_at_index(GMutableSpan selection,
}
}
static bool find_closest_point_to_screen_co(const Depsgraph &depsgraph,
const ARegion *region,
const RegionView3D *rv3d,
const Object &object,
const bke::CurvesGeometry &curves,
float2 mouse_pos,
float radius,
FindClosestData &closest_data)
static std::optional<FindClosestData> find_closest_point_to_screen_co(
const Depsgraph &depsgraph,
const ARegion *region,
const RegionView3D *rv3d,
const Object &object,
const bke::CurvesGeometry &curves,
float2 mouse_pos,
float radius,
const FindClosestData &initial_closest)
{
float4x4 projection;
ED_view3d_ob_project_mat_get(rv3d, &object, projection.ptr());
@ -377,10 +378,10 @@ static bool find_closest_point_to_screen_co(const Depsgraph &depsgraph,
bke::crazyspace::get_evaluated_curves_deformation(depsgraph, object);
const float radius_sq = pow2f(radius);
closest_data = threading::parallel_reduce(
const FindClosestData new_closest_data = threading::parallel_reduce(
curves.points_range(),
1024,
closest_data,
initial_closest,
[&](const IndexRange point_range, const FindClosestData &init) {
FindClosestData best_match = init;
for (const int point_i : point_range) {
@ -411,21 +412,23 @@ static bool find_closest_point_to_screen_co(const Depsgraph &depsgraph,
}
return b;
});
if (closest_data.index >= 0) {
return true;
if (new_closest_data.distance < initial_closest.distance) {
return new_closest_data;
}
return false;
return {};
}
static bool find_closest_curve_to_screen_co(const Depsgraph &depsgraph,
const ARegion *region,
const RegionView3D *rv3d,
const Object &object,
const bke::CurvesGeometry &curves,
float2 mouse_pos,
float radius,
FindClosestData &closest_data)
static std::optional<FindClosestData> find_closest_curve_to_screen_co(
const Depsgraph &depsgraph,
const ARegion *region,
const RegionView3D *rv3d,
const Object &object,
const bke::CurvesGeometry &curves,
float2 mouse_pos,
float radius,
const FindClosestData &initial_closest)
{
float4x4 projection;
ED_view3d_ob_project_mat_get(rv3d, &object, projection.ptr());
@ -436,10 +439,10 @@ static bool find_closest_curve_to_screen_co(const Depsgraph &depsgraph,
const float radius_sq = pow2f(radius);
const OffsetIndices points_by_curve = curves.points_by_curve();
closest_data = threading::parallel_reduce(
const FindClosestData new_closest_data = threading::parallel_reduce(
curves.curves_range(),
256,
closest_data,
initial_closest,
[&](const IndexRange curves_range, const FindClosestData &init) {
FindClosestData best_match = init;
for (const int curve_i : curves_range) {
@ -499,64 +502,44 @@ static bool find_closest_curve_to_screen_co(const Depsgraph &depsgraph,
return b;
});
if (closest_data.index >= 0) {
return true;
if (new_closest_data.distance < initial_closest.distance) {
return new_closest_data;
}
return false;
return {};
}
bool select_pick(const ViewContext &vc,
const Object &object,
bke::CurvesGeometry &curves,
const eAttrDomain selection_domain,
const SelectPick_Params &params,
const int2 coord,
FindClosestData initial)
std::optional<FindClosestData> closest_elem_find_screen_space(
const ViewContext &vc,
const Object &object,
bke::CurvesGeometry &curves,
const eAttrDomain domain,
const int2 coord,
const FindClosestData &initial_closest)
{
FindClosestData closest = initial;
bool found = false;
if (selection_domain == ATTR_DOMAIN_POINT) {
found = find_closest_point_to_screen_co(*vc.depsgraph,
vc.region,
vc.rv3d,
object,
curves,
float2(coord),
ED_view3d_select_dist_px(),
closest);
switch (domain) {
case ATTR_DOMAIN_POINT:
return find_closest_point_to_screen_co(*vc.depsgraph,
vc.region,
vc.rv3d,
object,
curves,
float2(coord),
ED_view3d_select_dist_px(),
initial_closest);
case ATTR_DOMAIN_CURVE:
return find_closest_curve_to_screen_co(*vc.depsgraph,
vc.region,
vc.rv3d,
object,
curves,
float2(coord),
ED_view3d_select_dist_px(),
initial_closest);
default:
BLI_assert_unreachable();
return {};
}
else if (selection_domain == ATTR_DOMAIN_CURVE) {
found = find_closest_curve_to_screen_co(*vc.depsgraph,
vc.region,
vc.rv3d,
object,
curves,
float2(coord),
ED_view3d_select_dist_px(),
closest);
}
bool deselected = false;
if (params.sel_op == SEL_OP_SET) {
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
curves, selection_domain, CD_PROP_BOOL);
if (found || ((params.deselect_all && has_anything_selected(selection.span)))) {
fill_selection_false(selection.span);
deselected = true;
}
selection.finish();
}
if (found) {
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
curves, selection_domain, CD_PROP_BOOL);
apply_selection_operation_at_index(selection.span, closest.index, params.sel_op);
selection.finish();
}
return deselected || found;
}
bool select_box(const ViewContext &vc,

View File

@ -115,6 +115,9 @@ bke::GSpanAttributeWriter ensure_selection_attribute(bke::CurvesGeometry &curves
eAttrDomain selection_domain,
eCustomDataType create_type);
/** Apply a change to a single curve or point. Avoid using this when affecting many elements. */
void apply_selection_operation_at_index(GMutableSpan selection, int index, eSelectOp sel_op);
/**
* (De)select all the curves.
*
@ -149,7 +152,7 @@ void select_random(bke::CurvesGeometry &curves,
float probability);
/**
* Helper struct for `select_pick`.
* Helper struct for `closest_elem_find_screen_space`.
*/
struct FindClosestData {
int index = -1;
@ -157,15 +160,16 @@ struct FindClosestData {
};
/**
* Select point or curve at a (screen-space) point.
* Find the closest screen-space point or curve in projected region-space.
*
* \return A new point or curve closer than the \a initial input, if one exists.
*/
bool select_pick(const ViewContext &vc,
const Object &object,
bke::CurvesGeometry &curves,
eAttrDomain selection_domain,
const SelectPick_Params &params,
int2 coord,
FindClosestData initial = {});
std::optional<FindClosestData> closest_elem_find_screen_space(const ViewContext &vc,
const Object &object,
bke::CurvesGeometry &curves,
eAttrDomain domain,
int2 coord,
const FindClosestData &initial);
/**
* Select points or curves in a (screen-space) rectangle.

View File

@ -31,6 +31,7 @@
#include "BLI_math_bits.h"
#include "BLI_rect.h"
#include "BLI_string.h"
#include "BLI_task.hh"
#include "BLI_utildefines.h"
#include "BLI_vector.hh"
@ -2985,9 +2986,14 @@ static bool ed_wpaint_vertex_select_pick(bContext *C,
return changed || found;
}
struct ClosestCurveDataBlock {
Curves *curves_id = nullptr;
blender::ed::curves::FindClosestData elem = {};
};
/**
* Cursor selection for the Curves object.
*
* Cursor selection for all Curves objects in edit mode.
*
* \returns true if the selection changed.
*/
static bool ed_curves_select_pick(bContext &C, const int mval[2], const SelectPick_Params &params)
@ -2999,35 +3005,81 @@ static bool ed_curves_select_pick(bContext &C, const int mval[2], const SelectPi
ED_view3d_viewcontext_init(&C, &vc, depsgraph);
uint bases_len;
Base **bases = BKE_view_layer_array_from_bases_in_edit_mode_unique_data(
Base **bases_ptr = BKE_view_layer_array_from_bases_in_edit_mode_unique_data(
vc.scene, vc.view_layer, vc.v3d, &bases_len);
Span<Base *> bases(bases_ptr, bases_len);
bool changed = false;
ed::curves::FindClosestData closest_data;
for (uint base_index = 0; base_index < bases_len; base_index++) {
Base *base = bases[base_index];
Object *curves_ob = base->object;
Curves &curves_id = *static_cast<Curves *>(curves_ob->data);
bke::CurvesGeometry &curves = curves_id.geometry.wrap();
Curves &active_curves_id = *static_cast<Curves *>(vc.obedit->data);
const eAttrDomain selection_domain = eAttrDomain(active_curves_id.selection_domain);
if (ed::curves::select_pick(vc,
*curves_ob,
curves,
eAttrDomain(curves_id.selection_domain),
params,
mval,
closest_data)) {
changed = true;
/* Use #ID_RECALC_GEOMETRY instead of #ID_RECALC_SELECT because it is handled as a
* generic attribute for now. */
DEG_id_tag_update(&curves_id.id, ID_RECALC_GEOMETRY);
WM_event_add_notifier(&C, NC_GEOM | ND_DATA, &curves_id);
}
const ClosestCurveDataBlock closest = threading::parallel_reduce(
bases.index_range(),
1L,
ClosestCurveDataBlock(),
[&](const IndexRange range, const ClosestCurveDataBlock &init) {
ClosestCurveDataBlock new_closest = init;
for (Base *base : bases.slice(range)) {
Object &curves_ob = *base->object;
Curves &curves_id = *static_cast<Curves *>(curves_ob.data);
std::optional<ed::curves::FindClosestData> new_closest_elem =
ed::curves::closest_elem_find_screen_space(vc,
HooglyBoogly marked this conversation as resolved
Review

This is problematic because we might be mixing selection modes, when we should really be just using the one of the active object (I believe that is what's used to decide what to draw too).

This is problematic because we might be mixing selection modes, when we should really be just using the one of the active object (I believe that is what's used to decide what to draw too).
Review

Good point!

Good point!
curves_ob,
curves_id.geometry.wrap(),
selection_domain,
mval,
new_closest.elem);
if (new_closest_elem) {
new_closest.elem = *new_closest_elem;
new_closest.curves_id = &curves_id;
}
}
return new_closest;
},
[](const ClosestCurveDataBlock &a, const ClosestCurveDataBlock &b) {
return (a.elem.distance < b.elem.distance) ? a : b;
});
HooglyBoogly marked this conversation as resolved
Review

I think I'm misunderstanding this check-- the deselection never seems to run.

I think I'm misunderstanding this check-- the deselection never seems to run.
Review

It looks like the call to ed::curves::has_anything_selected(curves) is always returning false when it should return true in case something is selected.

It looks like the call to `ed::curves::has_anything_selected(curves)` is always returning `false` when it should return `true` in case something is selected.
Review

Using if (ed::curves::has_anything_selected(selection.span)) { on the selection span seems to fix the issue for me. Looks like the other function is not doing what we expect it to.

Using `if (ed::curves::has_anything_selected(selection.span)) {` on the selection span seems to fix the issue for me. Looks like the other function is not doing what we expect it to.
Review

Thanks for checking that. Fixed in ea80e14464

Thanks for checking that. Fixed in ea80e144648905c60540e1d7281c6cdec1f528b6
std::atomic<bool> deselected = false;
if (params.deselect_all || params.sel_op == SEL_OP_SET) {
threading::parallel_for(bases.index_range(), 1L, [&](const IndexRange range) {
for (Base *base : bases.slice(range)) {
Curves &curves_id = *static_cast<Curves *>(base->object->data);
bke::CurvesGeometry &curves = curves_id.geometry.wrap();
if (ed::curves::has_anything_selected(curves)) {
bke::GSpanAttributeWriter selection = ed::curves::ensure_selection_attribute(
curves, selection_domain, CD_PROP_BOOL);
ed::curves::fill_selection_false(selection.span);
selection.finish();
deselected = true;
/* Use #ID_RECALC_GEOMETRY instead of #ID_RECALC_SELECT because it is handled as a
* generic attribute for now. */
DEG_id_tag_update(&curves_id.id, ID_RECALC_GEOMETRY);
WM_event_add_notifier(&C, NC_GEOM | ND_DATA, &curves_id);
}
}
});
}
HooglyBoogly marked this conversation as resolved
Review

Needs a MEM_freeN(bases_ptr).

Needs a `MEM_freeN(bases_ptr)`.
MEM_freeN(bases);
if (!closest.curves_id) {
MEM_freeN(bases_ptr);
return deselected;
}
return changed;
bke::GSpanAttributeWriter selection = ed::curves::ensure_selection_attribute(
closest.curves_id->geometry.wrap(), selection_domain, CD_PROP_BOOL);
ed::curves::apply_selection_operation_at_index(
selection.span, closest.elem.index, params.sel_op);
selection.finish();
/* Use #ID_RECALC_GEOMETRY instead of #ID_RECALC_SELECT because it is handled as a
* generic attribute for now. */
DEG_id_tag_update(&closest.curves_id->id, ID_RECALC_GEOMETRY);
WM_event_add_notifier(&C, NC_GEOM | ND_DATA, closest.curves_id);
MEM_freeN(bases_ptr);
return true;
}
static int view3d_select_exec(bContext *C, wmOperator *op)
@ -3081,7 +3133,8 @@ static int view3d_select_exec(bContext *C, wmOperator *op)
if (obedit && enumerate) {
/* Enumerate makes no sense in edit-mode unless also explicitly picking objects or bones.
* Pass the event through so the event may be handled by loop-select for e.g. see: #100204. */
* Pass the event through so the event may be handled by loop-select for e.g. see: #100204.
*/
if (obedit->type != OB_ARMATURE) {
return OPERATOR_PASS_THROUGH | OPERATOR_CANCELLED;
}