From ff4d5b6f04d42a3b222a29aae8b313991aa99ea8 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Mon, 16 Oct 2023 18:41:07 +0200 Subject: [PATCH] Geometry Nodes: don't use CustomDataAttributes in Instances This was the last use of `CustomDataAttributes`. It's not very useful nowadays because we have a lower level (`CustomData`) and higher level (`AttributeAccessor`) API. As part of this I removed the ability to `reserve` instances which was useful when adding instances one by one. Generally, such code should eventually be rewritten to handle more instances at once, instead of handling them one by one. This will lead to better performance and is more in line with how other geometry types (like mesh) are handled. This also adds the ability to move and assign `Instances` just like we can move and assign `CurvesGeometry`. --- source/blender/blenkernel/BKE_customdata.h | 5 +- source/blender/blenkernel/BKE_instances.hh | 14 ++- .../blender/blenkernel/intern/customdata.cc | 28 +++++- source/blender/blenkernel/intern/instances.cc | 99 ++++++++++++------- .../geometry/intern/join_geometries.cc | 1 - .../geometry/intern/realize_instances.cc | 1 - .../nodes/node_geo_collection_info.cc | 1 - 7 files changed, 97 insertions(+), 52 deletions(-) diff --git a/source/blender/blenkernel/BKE_customdata.h b/source/blender/blenkernel/BKE_customdata.h index d9b89c0515d..f7a654bb3ba 100644 --- a/source/blender/blenkernel/BKE_customdata.h +++ b/source/blender/blenkernel/BKE_customdata.h @@ -197,7 +197,10 @@ bool CustomData_merge_layout(const struct CustomData *source, * the #CD_CONSTRUCT behavior, so trivial types must be initialized by the caller. After being * resized, the #CustomData does not contain any referenced layers. */ -void CustomData_realloc(struct CustomData *data, int old_size, int new_size); +void CustomData_realloc(struct CustomData *data, + int old_size, + int new_size, + eCDAllocType alloctype = CD_CONSTRUCT); /** * BMesh version of CustomData_merge_layout; merges the layouts of source and `dest`, diff --git a/source/blender/blenkernel/BKE_instances.hh b/source/blender/blenkernel/BKE_instances.hh index 5876319404d..3b77ea01682 100644 --- a/source/blender/blenkernel/BKE_instances.hh +++ b/source/blender/blenkernel/BKE_instances.hh @@ -102,13 +102,17 @@ class Instances { mutable std::mutex almost_unique_ids_mutex_; mutable blender::Array almost_unique_ids_; - CustomDataAttributes attributes_; + CustomData attributes_; public: - Instances() = default; + Instances(); + Instances(Instances &&other); Instances(const Instances &other); + ~Instances(); + + Instances &operator=(const Instances &other); + Instances &operator=(Instances &&other); - void reserve(int min_capacity); /** * Resize the transform, handles, and attributes to the specified capacity. * @@ -258,12 +262,12 @@ inline const GeometrySet &InstanceReference::geometry_set() const inline CustomData &Instances::custom_data_attributes() { - return attributes_.data; + return attributes_; } inline const CustomData &Instances::custom_data_attributes() const { - return attributes_.data; + return attributes_; } /** \} */ diff --git a/source/blender/blenkernel/intern/customdata.cc b/source/blender/blenkernel/intern/customdata.cc index e8a5e1e9b12..952fce49b0a 100644 --- a/source/blender/blenkernel/intern/customdata.cc +++ b/source/blender/blenkernel/intern/customdata.cc @@ -2434,7 +2434,10 @@ void CustomData_ensure_layers_are_mutable(struct CustomData *data, int totelem) } } -void CustomData_realloc(CustomData *data, const int old_size, const int new_size) +void CustomData_realloc(CustomData *data, + const int old_size, + const int new_size, + const eCDAllocType alloctype) { BLI_assert(new_size >= 0); for (int i = 0; i < data->totlayer; i++) { @@ -2467,10 +2470,25 @@ void CustomData_realloc(CustomData *data, const int old_size, const int new_size } if (new_size > old_size) { - /* Initialize new values for non-trivial types. */ - if (typeInfo->construct) { - const int new_elements_num = new_size - old_size; - typeInfo->construct(POINTER_OFFSET(layer->data, old_size_in_bytes), new_elements_num); + const int new_elements_num = new_size - old_size; + void *new_elements_begin = POINTER_OFFSET(layer->data, old_size_in_bytes); + switch (alloctype) { + case CD_CONSTRUCT: { + /* Initialize new values for non-trivial types. */ + if (typeInfo->construct) { + typeInfo->construct(new_elements_begin, new_elements_num); + } + break; + } + case CD_SET_DEFAULT: { + if (typeInfo->set_default_value) { + typeInfo->set_default_value(new_elements_begin, new_elements_num); + } + else { + memset(new_elements_begin, 0, typeInfo->size * new_elements_num); + } + break; + } } } } diff --git a/source/blender/blenkernel/intern/instances.cc b/source/blender/blenkernel/intern/instances.cc index fe4d4974ae7..86d9c7eb7b9 100644 --- a/source/blender/blenkernel/intern/instances.cc +++ b/source/blender/blenkernel/intern/instances.cc @@ -43,36 +43,71 @@ bool operator==(const InstanceReference &a, const InstanceReference &b) return a.type_ == b.type_ && a.data_ == b.data_; } +Instances::Instances() +{ + CustomData_reset(&attributes_); +} + +Instances::Instances(Instances &&other) + : references_(std::move(other.references_)), + reference_handles_(std::move(other.reference_handles_)), + transforms_(std::move(other.transforms_)), + almost_unique_ids_(std::move(other.almost_unique_ids_)), + attributes_(other.attributes_) +{ + CustomData_reset(&other.attributes_); +} + Instances::Instances(const Instances &other) : references_(other.references_), reference_handles_(other.reference_handles_), transforms_(other.transforms_), - almost_unique_ids_(other.almost_unique_ids_), - attributes_(other.attributes_) + almost_unique_ids_(other.almost_unique_ids_) { + CustomData_copy(&other.attributes_, &attributes_, CD_MASK_ALL, other.instances_num()); } -void Instances::reserve(int min_capacity) +Instances::~Instances() { - reference_handles_.reserve(min_capacity); - transforms_.reserve(min_capacity); - attributes_.reallocate(min_capacity); + CustomData_free(&attributes_, this->instances_num()); +} + +Instances &Instances::operator=(const Instances &other) +{ + if (this == &other) { + return *this; + } + std::destroy_at(this); + new (this) Instances(other); + return *this; +} + +Instances &Instances::operator=(Instances &&other) +{ + if (this == &other) { + return *this; + } + std::destroy_at(this); + new (this) Instances(std::move(other)); + return *this; } void Instances::resize(int capacity) { + const int old_size = this->instances_num(); reference_handles_.resize(capacity); transforms_.resize(capacity); - attributes_.reallocate(capacity); + CustomData_realloc(&attributes_, old_size, capacity, CD_SET_DEFAULT); } void Instances::add_instance(const int instance_handle, const float4x4 &transform) { BLI_assert(instance_handle >= 0); BLI_assert(instance_handle < references_.size()); + const int old_size = this->instances_num(); reference_handles_.append(instance_handle); transforms_.append(transform); - attributes_.reallocate(this->instances_num()); + CustomData_realloc(&attributes_, old_size, transforms_.size()); } Span Instances::reference_handles() const @@ -138,37 +173,25 @@ void Instances::remove(const IndexMask &mask, return; } - const Span old_handles = this->reference_handles(); - Vector new_handles(mask.size()); - array_utils::gather(old_handles, mask, new_handles.as_mutable_span()); - reference_handles_ = std::move(new_handles); + const int new_size = mask.size(); - const Span old_tansforms = this->transforms(); - Vector new_transforms(mask.size()); - array_utils::gather(old_tansforms, mask, new_transforms.as_mutable_span()); - transforms_ = std::move(new_transforms); + Instances new_instances; + new_instances.references_ = std::move(references_); + new_instances.reference_handles_.resize(new_size); + new_instances.transforms_.resize(new_size); + array_utils::gather( + reference_handles_.as_span(), mask, new_instances.reference_handles_.as_mutable_span()); + array_utils::gather(transforms_.as_span(), mask, new_instances.transforms_.as_mutable_span()); - const bke::CustomDataAttributes &src_attributes = attributes_; + gather_attributes(this->attributes(), + ATTR_DOMAIN_INSTANCE, + propagation_info, + {"position"}, + mask, + new_instances.attributes_for_write()); - bke::CustomDataAttributes dst_attributes; - dst_attributes.reallocate(mask.size()); + *this = std::move(new_instances); - src_attributes.foreach_attribute( - [&](const bke::AttributeIDRef &id, const bke::AttributeMetaData &meta_data) { - if (id.is_anonymous() && !propagation_info.propagate(id.anonymous_id())) { - return true; - } - - GSpan src = *src_attributes.get_for_read(id); - dst_attributes.create(id, meta_data.data_type); - GMutableSpan dst = *dst_attributes.get_for_write(id); - array_utils::gather(src, mask, dst); - - return true; - }, - ATTR_DOMAIN_INSTANCE); - - attributes_ = std::move(dst_attributes); this->remove_unused_references(); } @@ -334,9 +357,9 @@ static Array generate_unique_instance_ids(Span original_ids) Span Instances::almost_unique_ids() const { std::lock_guard lock(almost_unique_ids_mutex_); - std::optional instance_ids_gspan = attributes_.get_for_read("id"); - if (instance_ids_gspan) { - Span instance_ids = instance_ids_gspan->typed(); + bke::AttributeReader instance_ids_attribute = this->attributes().lookup("id"); + if (instance_ids_attribute) { + Span instance_ids = instance_ids_attribute.varray.get_internal_span(); if (almost_unique_ids_.size() != instance_ids.size()) { almost_unique_ids_ = generate_unique_instance_ids(instance_ids); } diff --git a/source/blender/geometry/intern/join_geometries.cc b/source/blender/geometry/intern/join_geometries.cc index 5169a51f4d2..02f769e9465 100644 --- a/source/blender/geometry/intern/join_geometries.cc +++ b/source/blender/geometry/intern/join_geometries.cc @@ -105,7 +105,6 @@ static void join_instances(const Span src_components, static_cast(*src_component); tot_instances += src_instance_component.get()->instances_num(); } - dst_instances->reserve(tot_instances); for (const GeometryComponent *src_component : src_components) { const bke::InstancesComponent &src_instance_component = diff --git a/source/blender/geometry/intern/realize_instances.cc b/source/blender/geometry/intern/realize_instances.cc index 6da3050d321..216d6e3b7fa 100644 --- a/source/blender/geometry/intern/realize_instances.cc +++ b/source/blender/geometry/intern/realize_instances.cc @@ -32,7 +32,6 @@ using blender::bke::AttributeIDRef; using blender::bke::AttributeKind; using blender::bke::AttributeMetaData; using blender::bke::custom_data_type_to_cpp_type; -using blender::bke::CustomDataAttributes; using blender::bke::GSpanAttributeWriter; using blender::bke::InstanceReference; using blender::bke::Instances; diff --git a/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc b/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc index aab60120cb5..fd8288b2e59 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_collection_info.cc @@ -89,7 +89,6 @@ static void node_geo_exec(GeoNodeExecParams params) children_objects.append(collection_object->ob); } - instances->reserve(children_collections.size() + children_objects.size()); Vector entries; entries.reserve(children_collections.size() + children_objects.size());