Details

Reviewers

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 22795
Build 22795: 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

Merge branch 'master' into attribute-api
progress

Harbormaster completed remote builds in B22795: Diff 53117.Jun 30 2022, 5:27 PM

more refactoring

Harbormaster completed remote builds in B22830: Diff 53183.Jul 2 2022, 4:32 PM

Harbormaster completed remote builds in B22831: Diff 53184.Jul 2 2022, 4:50 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

Merge branch 'master' into attribute-api

Harbormaster completed remote builds in B22890: Diff 53380.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

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
133 ↗	(On Diff #53380)	I like the "Reader" and "Writer" naming, it makes it clearer that the attributes aren't owned here.
221 ↗	(On Diff #53380)	`... or if it is stored as a builtin attribute on a different domain`?
309 ↗	(On Diff #53380)	Any reason not to skip "generally"?
313 ↗	(On Diff #53380)	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).
348–349 ↗	(On Diff #53380)
351–352 ↗	(On Diff #53380)
398–399 ↗	(On Diff #53380)	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.
416 ↗	(On Diff #53380)
419 ↗	(On Diff #53380)	Nice idea with `optional`, I like that
425 ↗	(On Diff #53380)
433 ↗	(On Diff #53380)
465 ↗	(On Diff #53380)
470 ↗	(On Diff #53380)	Doesn't really matter but maybe this is more consistent?
564 ↗	(On Diff #53380)	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.
578–582 ↗	(On Diff #53380)	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.
606 ↗	(On Diff #53380)	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
1049–1053	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
1168–1195	This (`GVMutableAttribute_For_OutputAttribute`) looks unused now
source/blender/geometry/intern/subdivide_curves.cc
86–87 ↗	(On Diff #53380)	Maybe we should pass `AttributeAccessor` by value? Its size is only 16 bytes, just like spans.
source/blender/modifiers/intern/MOD_nodes.cc
999 ↗	(On Diff #53380)	Just to be consistent with elsewhere in the patch

This revision now requires changes to proceed.Jul 8 2022, 1:44 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
221 ↗	(On Diff #53380)	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.
398–399 ↗	(On Diff #53380)	yeah not sure either, won't make that part of the api documentation for now
source/blender/geometry/intern/subdivide_curves.cc
86–87 ↗	(On Diff #53380)	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 ↗	(On Diff #53380)	`domain_size` is consistent with the patch.

Merge branch 'master' into attribute-api
address comments

Harbormaster completed remote builds in B22900: Diff 53408.Jul 8 2022, 12:05 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.

BKE: New geometry attribute API.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 53117

source/blender/blenkernel/BKE_attribute_access.hh

source/blender/blenkernel/BKE_attribute_api.hh

source/blender/blenkernel/BKE_geometry_set.hh

source/blender/blenkernel/intern/attribute_access.cc

source/blender/blenkernel/intern/attribute_access_intern.hh

source/blender/blenkernel/intern/geometry_component_curve.cc

source/blender/blenkernel/intern/geometry_component_curves.cc

source/blender/blenkernel/intern/geometry_component_instances.cc

source/blender/blenkernel/intern/geometry_component_mesh.cc

source/blender/blenkernel/intern/geometry_component_pointcloud.cc

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc

source/blender/geometry/intern/realize_instances.cc

source/blender/nodes/NOD_geometry_exec.hh

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

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

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

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

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

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

BKE: New geometry attribute API.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 53117

source/blender/blenkernel/BKE_attribute_access.hh

source/blender/blenkernel/BKE_attribute_api.hh

source/blender/blenkernel/BKE_geometry_set.hh

source/blender/blenkernel/intern/attribute_access.cc

source/blender/blenkernel/intern/attribute_access_intern.hh

source/blender/blenkernel/intern/geometry_component_curve.cc

source/blender/blenkernel/intern/geometry_component_curves.cc

source/blender/blenkernel/intern/geometry_component_instances.cc

source/blender/blenkernel/intern/geometry_component_mesh.cc

source/blender/blenkernel/intern/geometry_component_pointcloud.cc

source/blender/editors/space_spreadsheet/spreadsheet_data_source_geometry.cc

source/blender/geometry/intern/realize_instances.cc

source/blender/nodes/NOD_geometry_exec.hh

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

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

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

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

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

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

BKE: New geometry attribute API.
ClosedPublic
Actions

Revision Contents
Changeset List