From f8dd03d3dd1b2d7f0ade7c209092212098c75cb4 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 1 Dec 2021 09:27:27 -0500 Subject: Cleanup: Store instances id attribute with other attributes Now that we can store any dynamic attribute on the instances component, we don't need the special case for `id`, it can just be handled by the generic attribute storage. Mostly this just allows removing a bunch of redundant code. I had to add a null check for `update_custom_data_pointers` because the instances component doesn't have any pointers to inside of custom data. Differential Revision: https://developer.blender.org/D13430 --- source/blender/blenkernel/BKE_geometry_set.hh | 12 -- .../blender/blenkernel/intern/attribute_access.cc | 20 ++- .../intern/geometry_component_instances.cc | 150 +++++---------------- .../nodes/legacy/node_geo_legacy_point_instance.cc | 6 +- .../geometry/nodes/node_geo_instance_on_points.cc | 11 -- 5 files changed, 57 insertions(+), 142 deletions(-) (limited to 'source') diff --git a/source/blender/blenkernel/BKE_geometry_set.hh b/source/blender/blenkernel/BKE_geometry_set.hh index 35e66908d54..ead62827104 100644 --- a/source/blender/blenkernel/BKE_geometry_set.hh +++ b/source/blender/blenkernel/BKE_geometry_set.hh @@ -636,13 +636,6 @@ class InstancesComponent : public GeometryComponent { blender::Vector instance_reference_handles_; /** Transformation of the instances. */ blender::Vector instance_transforms_; - /** - * IDs of the instances. They are used for consistency over multiple frames for things like - * motion blur. Proper stable ID data that actually helps when rendering can only be generated - * in some situations, so this vector is allowed to be empty, in which case the index of each - * instance will be used for the final ID. - */ - blender::Vector instance_ids_; /* These almost unique ids are generated based on `ids_`, which might not contain unique ids at * all. They are *almost* unique, because under certain very unlikely circumstances, they are not @@ -676,11 +669,6 @@ class InstancesComponent : public GeometryComponent { blender::MutableSpan instance_reference_handles(); blender::MutableSpan instance_transforms(); blender::Span instance_transforms() const; - blender::MutableSpan instance_ids(); - blender::Span instance_ids() const; - - blender::MutableSpan instance_ids_ensure(); - void instance_ids_clear(); int instances_amount() const; int references_amount() const; diff --git a/source/blender/blenkernel/intern/attribute_access.cc b/source/blender/blenkernel/intern/attribute_access.cc index 902e08f1b28..5663f6b8756 100644 --- a/source/blender/blenkernel/intern/attribute_access.cc +++ b/source/blender/blenkernel/intern/attribute_access.cc @@ -400,7 +400,9 @@ WriteAttributeLookup BuiltinCustomDataLayerProvider::try_get_for_write( } if (data != new_data) { - custom_data_access_.update_custom_data_pointers(component); + if (custom_data_access_.update_custom_data_pointers) { + custom_data_access_.update_custom_data_pointers(component); + } data = new_data; } @@ -441,7 +443,9 @@ bool BuiltinCustomDataLayerProvider::try_delete(GeometryComponent &component) co const bool delete_success = CustomData_free_layer( custom_data, stored_type_, domain_size, layer_index); if (delete_success) { - custom_data_access_.update_custom_data_pointers(component); + if (custom_data_access_.update_custom_data_pointers) { + custom_data_access_.update_custom_data_pointers(component); + } } return delete_success; } @@ -476,7 +480,9 @@ bool BuiltinCustomDataLayerProvider::try_create(GeometryComponent &component, *custom_data, stored_type_, domain_size, initializer); } if (success) { - custom_data_access_.update_custom_data_pointers(component); + if (custom_data_access_.update_custom_data_pointers) { + custom_data_access_.update_custom_data_pointers(component); + } } return success; } @@ -644,7 +650,9 @@ WriteAttributeLookup NamedLegacyCustomDataProvider::try_get_for_write( void *data_new = CustomData_duplicate_referenced_layer_named( custom_data, stored_type_, layer.name, domain_size); if (data_old != data_new) { - custom_data_access_.update_custom_data_pointers(component); + if (custom_data_access_.update_custom_data_pointers) { + custom_data_access_.update_custom_data_pointers(component); + } } return {as_write_attribute_(layer.data, domain_size), domain_}; } @@ -666,7 +674,9 @@ bool NamedLegacyCustomDataProvider::try_delete(GeometryComponent &component, if (custom_data_layer_matches_attribute_id(layer, attribute_id)) { const int domain_size = component.attribute_domain_size(domain_); CustomData_free_layer(custom_data, stored_type_, domain_size, i); - custom_data_access_.update_custom_data_pointers(component); + if (custom_data_access_.update_custom_data_pointers) { + custom_data_access_.update_custom_data_pointers(component); + } return true; } } diff --git a/source/blender/blenkernel/intern/geometry_component_instances.cc b/source/blender/blenkernel/intern/geometry_component_instances.cc index 29c28a875c1..aa80a2aacd2 100644 --- a/source/blender/blenkernel/intern/geometry_component_instances.cc +++ b/source/blender/blenkernel/intern/geometry_component_instances.cc @@ -37,6 +37,7 @@ using blender::MutableSpan; using blender::Set; using blender::Span; using blender::VectorSet; +using blender::fn::GSpan; /* -------------------------------------------------------------------- */ /** \name Geometry Component Implementation @@ -51,7 +52,6 @@ GeometryComponent *InstancesComponent::copy() const InstancesComponent *new_component = new InstancesComponent(); new_component->instance_reference_handles_ = instance_reference_handles_; new_component->instance_transforms_ = instance_transforms_; - new_component->instance_ids_ = instance_ids_; new_component->references_ = references_; return new_component; } @@ -60,9 +60,6 @@ void InstancesComponent::reserve(int min_capacity) { instance_reference_handles_.reserve(min_capacity); instance_transforms_.reserve(min_capacity); - if (!instance_ids_.is_empty()) { - this->instance_ids_ensure(); - } attributes_.reallocate(min_capacity); } @@ -76,9 +73,6 @@ void InstancesComponent::resize(int capacity) { instance_reference_handles_.resize(capacity); instance_transforms_.resize(capacity); - if (!instance_ids_.is_empty()) { - this->instance_ids_ensure(); - } attributes_.reallocate(capacity); } @@ -86,9 +80,7 @@ void InstancesComponent::clear() { instance_reference_handles_.clear(); instance_transforms_.clear(); - instance_ids_.clear(); attributes_.clear(); - references_.clear(); } @@ -98,9 +90,6 @@ void InstancesComponent::add_instance(const int instance_handle, const float4x4 BLI_assert(instance_handle < references_.size()); instance_reference_handles_.append(instance_handle); instance_transforms_.append(transform); - if (!instance_ids_.is_empty()) { - this->instance_ids_ensure(); - } attributes_.reallocate(this->instances_amount()); } @@ -123,31 +112,6 @@ blender::Span InstancesComponent::instance_transforms() const return instance_transforms_; } -blender::MutableSpan InstancesComponent::instance_ids() -{ - return instance_ids_; -} -blender::Span InstancesComponent::instance_ids() const -{ - return instance_ids_; -} - -/** - * Make sure the ID storage size matches the number of instances. By directly resizing the - * component's vectors internally, it is possible to be in a situation where the IDs are not - * empty but they do not have the correct size; this function resolves that. - */ -blender::MutableSpan InstancesComponent::instance_ids_ensure() -{ - instance_ids_.append_n_times(0, this->instances_amount() - instance_ids_.size()); - return instance_ids_; -} - -void InstancesComponent::instance_ids_clear() -{ - instance_ids_.clear_and_make_inline(); -} - /** * With write access to the instances component, the data in the instanced geometry sets can be * changed. This is a function on the component rather than each reference to ensure `const` @@ -351,15 +315,17 @@ static blender::Array generate_unique_instance_ids(Span original_ids) blender::Span InstancesComponent::almost_unique_ids() const { std::lock_guard lock(almost_unique_ids_mutex_); - if (instance_ids().is_empty()) { - almost_unique_ids_.reinitialize(this->instances_amount()); - for (const int i : almost_unique_ids_.index_range()) { - almost_unique_ids_[i] = i; + std::optional instance_ids_gspan = attributes_.get_for_read("id"); + if (instance_ids_gspan) { + Span instance_ids = instance_ids_gspan->typed(); + if (almost_unique_ids_.size() != instance_ids.size()) { + almost_unique_ids_ = generate_unique_instance_ids(instance_ids); } } else { - if (almost_unique_ids_.size() != instance_ids_.size()) { - almost_unique_ids_ = generate_unique_instance_ids(instance_ids_); + almost_unique_ids_.reinitialize(this->instances_amount()); + for (const int i : almost_unique_ids_.index_range()) { + almost_unique_ids_[i] = i; } } return almost_unique_ids_; @@ -438,81 +404,21 @@ class InstancePositionAttributeProvider final : public BuiltinAttributeProvider } }; -class InstanceIDAttributeProvider final : public BuiltinAttributeProvider { - public: - InstanceIDAttributeProvider() - : BuiltinAttributeProvider( - "id", ATTR_DOMAIN_INSTANCE, CD_PROP_INT32, Creatable, Writable, Deletable) - { - } - - GVArray try_get_for_read(const GeometryComponent &component) const final - { - const InstancesComponent &instances = static_cast(component); - if (instances.instance_ids().is_empty()) { - return {}; - } - return VArray::ForSpan(instances.instance_ids()); - } - - WriteAttributeLookup try_get_for_write(GeometryComponent &component) const final - { - InstancesComponent &instances = static_cast(component); - if (instances.instance_ids().is_empty()) { - return {}; - } - return {VMutableArray::ForSpan(instances.instance_ids()), domain_}; - } - - bool try_delete(GeometryComponent &component) const final - { - InstancesComponent &instances = static_cast(component); - if (instances.instance_ids().is_empty()) { - return false; - } - instances.instance_ids_clear(); - return true; - } - - bool try_create(GeometryComponent &component, const AttributeInit &initializer) const final - { - InstancesComponent &instances = static_cast(component); - if (instances.instances_amount() == 0) { - return false; - } - MutableSpan ids = instances.instance_ids_ensure(); - switch (initializer.type) { - case AttributeInit::Type::Default: { - ids.fill(0); - break; - } - case AttributeInit::Type::VArray: { - const GVArray &varray = static_cast(initializer).varray; - varray.materialize_to_uninitialized(varray.index_range(), ids.data()); - break; - } - case AttributeInit::Type::MoveArray: { - void *source_data = static_cast(initializer).data; - ids.copy_from({static_cast(source_data), instances.instances_amount()}); - MEM_freeN(source_data); - break; - } - } - return true; - } +template +static GVArray make_array_read_attribute(const void *data, const int domain_size) +{ + return VArray::ForSpan(Span((const T *)data, domain_size)); +} - bool exists(const GeometryComponent &component) const final - { - const InstancesComponent &instances = static_cast(component); - return !instances.instance_ids().is_empty(); - } -}; +template +static GVMutableArray make_array_write_attribute(void *data, const int domain_size) +{ + return VMutableArray::ForSpan(MutableSpan((T *)data, domain_size)); +} static ComponentAttributeProviders create_attribute_providers_for_instances() { static InstancePositionAttributeProvider position; - static InstanceIDAttributeProvider id; - static CustomDataAccessInfo instance_custom_data_access = { [](GeometryComponent &component) -> CustomData * { InstancesComponent &inst = static_cast(component); @@ -524,6 +430,24 @@ static ComponentAttributeProviders create_attribute_providers_for_instances() }, nullptr}; + /** + * IDs of the instances. They are used for consistency over multiple frames for things like + * motion blur. Proper stable ID data that actually helps when rendering can only be generated + * in some situations, so this vector is allowed to be empty, in which case the index of each + * instance will be used for the final ID. + */ + static BuiltinCustomDataLayerProvider id("id", + ATTR_DOMAIN_INSTANCE, + CD_PROP_INT32, + CD_PROP_INT32, + BuiltinAttributeProvider::Creatable, + BuiltinAttributeProvider::Writable, + BuiltinAttributeProvider::Deletable, + instance_custom_data_access, + make_array_read_attribute, + make_array_write_attribute, + nullptr); + static CustomDataAttributeProvider instance_custom_data(ATTR_DOMAIN_INSTANCE, instance_custom_data_access); diff --git a/source/blender/nodes/geometry/nodes/legacy/node_geo_legacy_point_instance.cc b/source/blender/nodes/geometry/nodes/legacy/node_geo_legacy_point_instance.cc index be8294cda90..ef35bef96aa 100644 --- a/source/blender/nodes/geometry/nodes/legacy/node_geo_legacy_point_instance.cc +++ b/source/blender/nodes/geometry/nodes/legacy/node_geo_legacy_point_instance.cc @@ -186,7 +186,9 @@ static void add_instances_from_component(InstancesComponent &instances, instances.resize(start_len + domain_size); MutableSpan handles = instances.instance_reference_handles().slice(start_len, domain_size); MutableSpan transforms = instances.instance_transforms().slice(start_len, domain_size); - MutableSpan instance_ids = instances.instance_ids_ensure().slice(start_len, domain_size); + OutputAttribute_Typed instance_id_attribute = + instances.attribute_try_get_for_output_only("id", ATTR_DOMAIN_INSTANCE); + MutableSpan instance_ids = instance_id_attribute.as_span(); /* Skip all of the randomness handling if there is only a single possible instance * (anything except for collection mode with "Whole Collection" turned off). */ @@ -213,6 +215,8 @@ static void add_instances_from_component(InstancesComponent &instances, } }); } + + instance_id_attribute.save(); } static void node_geo_exec(GeoNodeExecParams params) diff --git a/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc b/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc index 637c2c59da6..4e75f1f60d9 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc @@ -158,17 +158,6 @@ static void add_instances_from_component( } }); - VArray ids = src_component - .attribute_try_get_for_read("id", ATTR_DOMAIN_POINT, CD_PROP_INT32) - .typed(); - if (ids) { - VArray_Span ids_span{ids}; - MutableSpan dst_ids = dst_component.instance_ids_ensure(); - for (const int64_t i : selection.index_range()) { - dst_ids[i] = ids_span[selection[i]]; - } - } - if (pick_instance.is_single()) { if (pick_instance.get_internal_single()) { if (instance.has_realized_data()) { -- cgit v1.2.3