Page MenuHome
Differential D15280

BKE: New geometry attribute API.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Jun 23 2022, 5:43 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Commits
rBb876ce2a4a46: Geometry Nodes: new geometry attribute API
Summary

Currently there are two attribute APIs. The first, defined in BKE_attribute.h is accessible from RNA and C code, The second is implemented with GeometryComponent, accessible in C++ code. The second is widely used, but only being accessible through the geometry set API makes it awkward to use, and even impossible for types that don't correspond directly to a geometry component type like CurvesGeometry (or even BMesh).

This patch adds a new attribute API, designed to replace the GeometryComponent attribute API now, and to eventually replace the other one. The basic structure provides the user with an AttributeAccessor class, which has a pointer to data and a pointer to an implementation of functions that access and change attributes. This implementation satisfies a few requirements.

  • Const correctness: Correctness is non-trivial. Attributes cannot be changed without mutable data, and one cannot break it by simply copying the accessor.
  • Stack storage: The accessor type is small enough to be stored on the stack and can be returned by value.
  • Avoid complex inheritance: One could imagine all geometry types inheriting from some "Attribute provider base class". However, this doesn't work well with the C-like way these types are used elsewhere in Blender.

In the end, attribute access is still implemented with the pattern of "providers", defined in the geometry component files. Now these can be defined elsewhere as well though.

Diff Detail

Repository
rB Blender
Branch
attribute-api
Build Status
Buildable 22900
Build 22900: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Jun 23 2022, 5:43 PM
Jacques Lucke (JacquesLucke) created this revision.
Harbormaster completed remote builds in B22660: Diff 52836.Jun 23 2022, 5:43 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jun 29 2022, 1:03 AM
Hans Goudey (HooglyBoogly) mentioned this in D15310: Make renaming mesh customdata layers via the python api go through BKE_id_attribute_rename() for attribute layers..Jun 29 2022, 5:13 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 53117.Jun 30 2022, 5:27 PM
  • Merge branch 'master' into attribute-api
  • progress
Harbormaster completed remote builds in B22795: Diff 53117.Jun 30 2022, 5:27 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 53183.Jul 2 2022, 4:32 PM
  • more refactoring
Harbormaster completed remote builds in B22830: Diff 53183.Jul 2 2022, 4:32 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 53184.Jul 2 2022, 4:50 PM
  • fix
Harbormaster completed remote builds in B22831: Diff 53184.Jul 2 2022, 4:50 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 53379.Jul 7 2022, 6:47 PM
  • Merge branch 'master' into attribute-api
  • move to BKE_attribute.hh header
  • improve comments
  • cleanup
Harbormaster completed remote builds in B22889: Diff 53379.Jul 7 2022, 6:47 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 53380.Jul 7 2022, 7:28 PM
  • Merge branch 'master' into attribute-api
Harbormaster completed remote builds in B22890: Diff 53380.Jul 7 2022, 7:28 PM
Jacques Lucke (JacquesLucke) added a comment.Jul 7 2022, 7:28 PM

https://builder.blender.org/admin/#/builders/18/builds/523

Jacques Lucke (JacquesLucke) retitled this revision from BKE: New attribute API (WIP). to BKE: New geometry attribute API..Jul 7 2022, 7:32 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) added a reviewer: Hans Goudey (HooglyBoogly).
Jacob Merrill (blueprintrandom) awarded a token.Jul 7 2022, 7:35 PM
Hans Goudey (HooglyBoogly) requested changes to this revision.Jul 8 2022, 1:44 AM

Overall this looks great! I'm excited for this to land. And glad it will be in 3.3, which might make backports easier. The inlines are mostly about naming and comments.

I have two general comments though:

  • I'm concerned about the keep_existing_values argument. I find this much more confusing and less visible than the existing "write_only" naming. With boolean arguments we need to remember the default, and remember whether true or false corresponds to initialization/copying. I could see it being much easier to accidentally depend on that with the approach in this patch.
  • Reading the patch, the AttributesRef naming you suggested in chat seemed a bit more reasonable. I think I still very slightly prefer "Accessor", but "Ref" would also be fine with me, and is a little shorter and consistent with StringRef, AttributeIDRef, and others. It seems to fit in slightly less well with AttributeWriter and AttributeReader though.
source/blender/blenkernel/BKE_attribute.hh
134

I like the "Reader" and "Writer" naming, it makes it clearer that the attributes aren't owned here.

222

... or if it is stored as a builtin attribute on a different domain?

310

Any reason not to skip "generally"?

314

Finishing the comment with something like which helps to... would be nice, to help convince people the design makes sense (which is part of the comment's job I suppose).

349–350
352–353
399–400

Do you want to include that builtin attributes have a constant domain and type?
I sort of forget where we ended up the last time we talked about it, whether it was just because of our current code or actually our long-term goal.

417
420

Nice idea with optional, I like that

426
434
466
471

Doesn't really matter but maybe this is more consistent?

565

Being used to OutputAttribute, this behavior is a bit confusing, but I think after doing this refactor you have a much better idea of what behavior is actually used in most places.

579–583

The cost is two more functions and some more boilerplate, but I do think for_write_only is probably a bit friendlier than keep_existing_values.
It's also much more visible in the caller, which sees _only instead of false, which has a lot more meaning.
If you agree, I'm happy to help splitting them up if you're not up to it.

607

It's not clear why keep_existing_values is false by default here and true by default above. They should probably be consistent.

source/blender/blenkernel/BKE_geometry_set.hh
829–833

In lieu of bke::Mesh.attributes() I'd propose the naming blender::bke::mesh_attributes(const Mesh &mesh) and blender::bke::pointcloud_attributes(const PointCloud &pointcloud)`
These should also take references as arguments IMO. They are just defined in C++ code, we might as well make use of that. I haven't found names like that to be controversial in other code.

source/blender/blenkernel/intern/attribute_access.cc
1167–1194

This (GVMutableAttribute_For_OutputAttribute) looks unused now

source/blender/geometry/intern/subdivide_curves.cc
86–87

Maybe we should pass AttributeAccessor by value? Its size is only 16 bytes, just like spans.

source/blender/modifiers/intern/MOD_nodes.cc
999

Just to be consistent with elsewhere in the patch

This revision now requires changes to proceed.Jul 8 2022, 1:44 AM
Jacques Lucke (JacquesLucke) marked 17 inline comments as done.Jul 8 2022, 11:52 AM

AttributeAccessor is fine for me as well. If you prefer that, we can use it. I don't have a strong preference.

source/blender/blenkernel/BKE_attribute.hh
222

No, that behavior does not exist anymore. If that's really the expected behavior, it has to be implemented on the call site now. I found this behavior to be unnecessary in general. We very rarely to never write to attributes on the wrong domain. If we do, that should probably be fixed.

399–400

yeah not sure either, won't make that part of the api documentation for now

source/blender/geometry/intern/subdivide_curves.cc
86–87

We could, but I think it's fine to pass by reference. As a general rule, it's better to pass things by reference instead of by value. Wouldn't make a special rule for attribute accessors, because there is no benefit.
For Span and StringRef there can actually be a performance benefit when it's passed by value instead of reference, because then there is one less pointer dereference on element access. This is true here as well, but the effect is negligible.

source/blender/modifiers/intern/MOD_nodes.cc
999

domain_size is consistent with the patch.

Jacques Lucke (JacquesLucke) updated this revision to Diff 53408.Jul 8 2022, 12:05 PM
Jacques Lucke (JacquesLucke) marked 4 inline comments as done.
  • Merge branch 'master' into attribute-api
  • address comments
Harbormaster completed remote builds in B22900: Diff 53408.Jul 8 2022, 12:05 PM
Hans Goudey (HooglyBoogly) accepted this revision.EditedJul 8 2022, 3:53 PM

This looks good to me now. Nice job :)

This revision is now accepted and ready to land.Jul 8 2022, 3:53 PM
Closed by commit rBb876ce2a4a46: Geometry Nodes: new geometry attribute API (authored by Jacques Lucke (JacquesLucke)). · Explain WhyJul 8 2022, 4:17 PM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) added a commit: rBb876ce2a4a46: Geometry Nodes: new geometry attribute API.

Large Diff

This large diff affects 108 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
814 lines
5 lines
260 lines
10 lines
4 lines
2 lines
intern/
2 lines
856 lines
266 lines
125 lines
11 lines
72 lines
230 lines
191 lines
101 lines
212 lines
102 lines
41 lines
4 lines
7 lines
2 lines
draw/
intern/
15 lines
4 lines
editors/
curves/
intern/
22 lines
geometry/
28 lines
object/
4 lines
9 lines
sculpt_paint/
9 lines
10 lines
9 lines
48 lines
space_spreadsheet/
33 lines
2 lines
geometry/
intern/
12 lines
22 lines
27 lines
268 lines
25 lines
36 lines
58 lines
io/
wavefront_obj/
exporter/
5 lines
modifiers/
intern/
37 lines
nodes/
15 lines
geometry/
nodes/
26 lines
17 lines
20 lines
16 lines
14 lines
18 lines
4 lines
12 lines
4 lines
4 lines
13 lines
4 lines
42 lines
57 lines
11 lines
111 lines
4 lines
64 lines
2 lines
4 lines
17 lines
12 lines
4 lines
2 lines
4 lines
2 lines
2 lines
4 lines
5 lines
2 lines
4 lines
8 lines
12 lines
16 lines
2 lines
6 lines
42 lines
15 lines
12 lines
2 lines
26 lines
13 lines
13 lines
7 lines
4 lines
4 lines
10 lines
17 lines
18 lines
26 lines
17 lines
17 lines
52 lines
18 lines
18 lines
18 lines
25 lines
18 lines
14 lines
10 lines
4 lines
6 lines
6 lines
intern/
12 lines

Diff 53408

View Options

source/blender/blenkernel/BKE_attribute.hh

Loading...
View Options

source/blender/blenkernel/BKE_attribute_access.hh

View Options

source/blender/blenkernel/BKE_curves.hh

View Options

source/blender/blenkernel/BKE_geometry_set.hh

Loading...
View Options

source/blender/blenkernel/BKE_mesh_sample.hh

View Options

source/blender/blenkernel/BKE_spline.hh

View Options

source/blender/blenkernel/CMakeLists.txt

View Options

source/blender/blenkernel/intern/attribute.cc

View Options

source/blender/blenkernel/intern/attribute_access.cc

Loading...
View Options

source/blender/blenkernel/intern/attribute_access_intern.hh

View Options

source/blender/blenkernel/intern/curve_eval.cc

View Options

source/blender/blenkernel/intern/curve_legacy_convert.cc

View Options

source/blender/blenkernel/intern/curve_to_mesh_convert.cc

View Options

source/blender/blenkernel/intern/geometry_component_curve.cc

View Options

source/blender/blenkernel/intern/geometry_component_curves.cc

View Options

source/blender/blenkernel/intern/geometry_component_instances.cc

View Options

source/blender/blenkernel/intern/geometry_component_mesh.cc

View Options

source/blender/blenkernel/intern/geometry_component_pointcloud.cc

View Options

source/blender/blenkernel/intern/geometry_set.cc

View Options

source/blender/blenkernel/intern/mesh_convert.cc

View Options

source/blender/blenkernel/intern/mesh_sample.cc

View Options

source/blender/blenkernel/intern/spline_base.cc

View Options

source/blender/draw/intern/draw_cache_impl_curves.cc

View Options

source/blender/draw/intern/draw_curves.cc

View Options

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

View Options

source/blender/editors/geometry/geometry_attributes.cc

View Options

source/blender/editors/object/object_add.cc

View Options

source/blender/editors/object/object_modifier.cc

View Options

source/blender/editors/sculpt_paint/curves_sculpt_add.cc

View Options

source/blender/editors/sculpt_paint/curves_sculpt_density.cc

View Options

source/blender/editors/sculpt_paint/curves_sculpt_slide.cc

View Options

source/blender/editors/sculpt_paint/paint_vertex_color_ops.cc

View Options

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc

View Options

source/blender/editors/space_spreadsheet/spreadsheet_dataset_draw.cc

View Options

source/blender/geometry/intern/mesh_primitive_cuboid.cc

View Options

source/blender/geometry/intern/mesh_to_curve_convert.cc

View Options

source/blender/geometry/intern/point_merge_by_distance.cc

View Options

source/blender/geometry/intern/realize_instances.cc

View Options

source/blender/geometry/intern/resample_curves.cc

View Options

source/blender/geometry/intern/set_curve_type.cc

View Options

source/blender/geometry/intern/subdivide_curves.cc

Loading...
View Options

source/blender/io/wavefront_obj/exporter/obj_export_file_writer.cc

View Options

source/blender/modifiers/intern/MOD_nodes.cc

Loading...
View Options

source/blender/nodes/NOD_geometry_exec.hh

View Options

source/blender/nodes/geometry/nodes/node_geo_accumulate_field.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_attribute_capture.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_attribute_domain_size.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_attribute_statistic.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_boolean.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_convex_hull.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_fillet.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_primitive_star.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_reverse.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_set_handle_type.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_curve_trim.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_delete_geometry.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_distribute_points_on_faces.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_dual_mesh.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_duplicate_elements.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_edge_split.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_extrude_mesh.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_field_at_index.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_field_on_domain.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_flip_faces.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_curve_handles.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_edge_angle.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_edge_neighbors.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_edge_vertices.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_face_area.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_face_is_planar.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_face_neighbors.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_mesh_island.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_spline_length.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_input_tangent.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_instances_to_points.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_join_geometry.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_material_selection.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_grid.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_uv_sphere.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_to_curve.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_mesh_to_points.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_points.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_points_to_vertices.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_points_to_volume.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_raycast.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_remove_attribute.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_curve_handles.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_curve_radius.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_curve_tilt.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_id.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_material_index.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_point_radius.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_position.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_shade_smooth.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_spline_cyclic.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_set_spline_resolution.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_store_named_attribute.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_string_to_curves.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_subdivision_surface.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_transfer_attribute.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_triangulate.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_uv_pack_islands.cc

View Options

source/blender/nodes/geometry/nodes/node_geo_uv_unwrap.cc

View Options

source/blender/nodes/intern/geometry_nodes_eval_log.cc