Page MenuHome

Curves: Support boolean attribute selection type, other simplifications
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Sep 25 2022, 6:03 AM.

Details

Summary

Use a single ".selection" attribute instead of multiple. This
attribute can be on either the point or curve domain and can have
either boolean or float type. Some tools create boolean selections,
other tools create float selections. Some tools "upgrade" the
attribute from boolean to float.

Edit mode tools that create selections from scratch can create boolean
selections, but edit mode should generally be able to handle both
selection types. Sculpt mode should be able to read boolean selections,
but can set the type to float.

Theoretically we could just always use floats to store selections,
but the type-agnosticism doesn't cost too much given the existing tools
for dealing with it, and it should allow other optimizations in the
future, like different ways to store boolean attributes.

Notes:

  • In the future, the implementation of float_selection_ensure can be replaced by a generic "convert attribute" function.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Sep 25 2022, 6:03 AM
Hans Goudey (HooglyBoogly) created this revision.
Jacques Lucke (JacquesLucke) requested changes to this revision.Nov 12 2022, 3:14 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/attribute_access.cc
61 ↗(On Diff #57530)

Leftover?

source/blender/draw/engines/overlay/overlay_sculpt_curves.cc
36

Not sure we can or should rely on automatic domain interpolation here. Would be nice if it were explicit how float selections are converted to booleans.

source/blender/editors/curves/intern/curves_ops.cc
815

I feel like this kind of devirtualization is wrong here. Mainly because in the case where it is a single value, the loop is not necessary.

818

Should start using atomics properly when writing to the same variable from different threads.

source/blender/editors/curves/intern/curves_selection.cc
25

typo (1is)

source/blender/editors/include/ED_curves_sculpt.h
19

looks like this section can be removed now

This revision now requires changes to proceed.Nov 12 2022, 3:14 PM
Hans Goudey (HooglyBoogly) marked 6 inline comments as done.
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/draw/engines/overlay/overlay_sculpt_curves.cc
36

(from chat conversation for reference)

It's just nice to be able to read it on any domain as a regular attribute, since that simplifies a lot of code.
We could use attribute meta-data to specialize this behavior if we needed to, while continuing to use the generic attribute system.

source/blender/editors/curves/intern/curves_ops.cc
818

I generalized this a bit to bool contains(const VArray<bool> &varray, const bool value). I thought of putting it in BLI_array_utils.hh, but maybe best to wait until it's needed elsewhere.

  • Merge master
  • Add versioning

Fix diff (file was missing)

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Don't have to look at it again after the mentioned things are fixed.

source/blender/editors/curves/intern/curves_ops.cc
812

This should be compared to value.

837

The return value of std::find can't be used that way. You have to compare it to chunk.end().
That said, you should probably only search in the part of chunk that is actually used, otherwise you'll get wrong results. Having this code complexity for the non-single-or-span case is probably not necessary now anyway? Can this case ever happen?

source/blender/editors/curves/intern/curves_selection.cc
34

Is this the wrong way around?

This revision is now accepted and ready to land.Jan 2 2023, 7:26 PM
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.Jan 4 2023, 4:52 AM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/editors/curves/intern/curves_ops.cc
837

Eek, thanks for finding these problems. I wish std::find and other similar functions would just return a null pointer. The way they are has prevented me from using them a few times, I just forgot here.