Page MenuHome

BKE: New geometry attribute API.
ClosedPublic

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

Details

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 22660
Build 22660: 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.
  • Merge branch 'master' into attribute-api
  • progress
  • Merge branch 'master' into attribute-api
  • move to BKE_attribute.hh header
  • improve comments
  • cleanup
  • Merge branch 'master' into attribute-api
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)
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
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
1040–1044

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
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
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.

Jacques Lucke (JacquesLucke) marked 4 inline comments as done.
  • Merge branch 'master' into attribute-api
  • address comments
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