Page MenuHome

Curves edit mode: support point selection
Changes PlannedPublic

Authored by Kévin Dietrich (kevindietrich) on Mar 14 2022, 1:10 PM.

Details

Summary

This adds support for selecting Curves points in edit mode.

Selection happens one of two ways:

  • either X-ray is enabled in which case selection is done by projecting points on the screen and choosing which one is the closest to the mouse cursor
  • X-ray is disabled in which case selection is done by looking up the selection draw buffer from the Select ID draw engine

For the second case, an point index vertex buffer is added to
the HairBatchCache and filled when requested.

A flags attribute is added to the Curves point which records
if a point is selected or not. More flags can be added, although
for now we only use 8 bits per point. This attribute is then
used as a basis for creating a edit data draw buffer which will
draw the points (color) differently based on their selection
status.

The last selected point is also recorded which is used to define
the active point.

Diff Detail

Repository
rB Blender
Branch
edit_curves_selection (branched from master)
Build Status
Buildable 21022
Build 21022: arc lint + arc unit

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Mar 14 2022, 1:10 PM
Kévin Dietrich (kevindietrich) created this revision.

I think selection should be a boolean attribute rather than a flag in an integer. This gives us options for storing it differently in the future if we want. It simplifies code because we can use generic utilities that work for all boolean attributes, instead of writing specific code for a flag.

Further, we might have to use a different way of identifying the selection layer than a generic attribute name. Currently, if someone makes an attribute called flags, thing will break.
I mentioned this problem a bit in T95965: Mesh Struct of Arrays Refactor (Edit-Data Attributes section), since we'll have the same problem for meshes in the future.
I would propose using something similar to AnonymousAttributeID, but called something like StaticAttributeID that could be used as a const global variable to identify edit mode selection layers.

From a drawing point of view, this looks fine.

This revision is now accepted and ready to land.Mar 16 2022, 8:10 AM

I wonder what would be the advantages of using this selection draw buffer solution.
If I'm not mistaken, the current selection solution for hairs is to store the depth buffer and project each point of the strand checking occlusion with the depth buffer.
One advantage I see of the method with depth buffer is that it supports cases where more than one point is in the same pixel. And you can also deduplicate the code with X-ray selection.

Hans Goudey (HooglyBoogly) requested changes to this revision.Mar 16 2022, 2:23 PM
This revision now requires changes to proceed.Mar 16 2022, 2:23 PM

Rebase with master.

Added a StaticAttributeID to reference the selection layer.
The implementation is a bit summary, there are some open-ended
questions:

  • do we use reference count like AnonymousAttributeID?
  • how do we generate unique names so that future usage do not worry about name collisions? If it is declared by value, we need the definition of the structure. Maybe this should be only visible to C++ code, so a constructor can be used to initialize it. C code will only have accessed to the type declaration, and maybe some function to access the name.

Perhaps this should be its own patch, I kept it here for
example usage, but can be separated to refine desing and
implementation.

I agree that it probably makes sense to move the attribute ID changes to a separate patch.

  • I'm not sure what purpose a reference count would serve in this case.
  • Ideally name collisions wouldn't be a problem, since CustomData can check whether the layer has a "static ID" and avoid using the name in that case.

I left some other comments inline in the meantime.

source/blender/draw/engines/select/select_draw_utils.c
36–43

const BoundBox *

source/blender/draw/intern/draw_cache_impl_curves.cc
188

It probably makes sense to have special cases for point_selection.is_single()

207

"color" for point ids sounds a bit weird, is that right?

source/blender/draw/intern/draw_hair_private.h
49–50

I made D14576 to start splitting these changes into separate code so we don't have to worry about causing regressions in drawing hair particle systems, and so we can more easily make changes.

source/blender/editors/curves/intern/edit_curves_select.cc
32–35

I think it's a bit preferable to put most of this file in the blender::ed::curves namespace, and just leave the ED_* C API in the global namespace.

274

const

I was thinking more about what makes the attributes that you call static attributes different from other attributes. The main thing we want to achieve is that procedural processing does not depend on these attributes, right?
So maybe we can just make them builtin attributes (like position) with the additional constraint that the user can't access the data in a procedural context.
Propagation of such attributes should still work. Also outside of procedural contexts, the data should be accessible like normal named attributes (e.g. with the Python api).

More specifically, these "ui attributes" (not sure if that's a better name, maybe it is) are exactly like built-in attributes with these differences:

  • Cannot be accessed with the Named Attribute, Remove Attribute and Store Named Attribute nodes.
  • Cannot be used as input/output in the geometry nodes modifier.
  • Is excluded when exporting geometry.
  • Is excluded in attribute lists.

That makes them essentially invisible except from the Python API where they behave the same as other builtin attributes.

That only works if my initial assumption is true of course, maybe it's not.

This is related to T96848 and T97174.

[EDIT]
Thinking about it a bit more, maybe it's wrong to say ui attributes are built-in attributes in general. the problem with that is that there may be a dynamic number of ui attributes when e.g. a ui attribute is attached to every uv layer. Maybe such layers could just be tagged with some CD_FLAG_UI flag. They can be both, built-in and normal attributes.

Also see T97452 for a more complete proposal on how to move forward with the selection attribute.

Kévin Dietrich (kevindietrich) marked 5 inline comments as done.
  • Update VBO name (following changes in master).
  • Remove static attribute, use ".selection_point_bool" as suggested.
Kévin Dietrich (kevindietrich) planned changes to this revision.Jul 16 2022, 1:17 PM

Marking as plan changes as this now requires proper handling of evaluated vs. original data since modifier support has been added to Curves between the last two revisions of this differential.