From 6dca66d606edd595b7ebfa188cc5ac965b7fe5d2 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Mon, 11 Aug 2025 10:03:59 +0200 Subject: [PATCH] Fix: Instances: ensure unique instance ids The previous code to get unique instance ids did not actually ensure 100% uniqueness. Now the code does and hence the "almost" from the name is removed. In practice, this change likely does not have any impact because the fallback case is very unlikely to be reached. Pull Request: https://projects.blender.org/blender/blender/pulls/144324 --- source/blender/blenkernel/BKE_instances.hh | 15 ++++++------ source/blender/blenkernel/intern/instances.cc | 23 +++++++++++-------- .../blender/blenkernel/intern/object_dupli.cc | 4 ++-- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/source/blender/blenkernel/BKE_instances.hh b/source/blender/blenkernel/BKE_instances.hh index 9cb441e7f93..6dc6f50d05a 100644 --- a/source/blender/blenkernel/BKE_instances.hh +++ b/source/blender/blenkernel/BKE_instances.hh @@ -126,11 +126,9 @@ class Instances { */ mutable SharedCache> reference_user_counts_; - /* These almost unique ids are generated based on the `id` attribute, which might not contain - * unique ids at all. They are *almost* unique, because under certain very unlikely - * circumstances, they are not unique. Code using these ids should not crash when they are not - * unique but can generally expect them to be unique. */ - mutable SharedCache> almost_unique_ids_cache_; + /* These unique ids are generated based on the `id` attribute, which might not contain + * unique ids at all. */ + mutable SharedCache> unique_ids_cache_; public: Instances(); @@ -199,9 +197,10 @@ class Instances { */ void remove(const IndexMask &mask, const AttributeFilter &attribute_filter); /** - * Get an id for every instance. These can be used e.g. motion blur. + * Get an id for every instance. These can be used e.g. motion blur. This is based on the "id" + * attribute but makes sure that the ids are actually unique. */ - Span almost_unique_ids() const; + Span unique_ids() const; /** * Get cached user counts for every reference. @@ -225,7 +224,7 @@ class Instances { void tag_reference_handles_changed() { reference_user_counts_.tag_dirty(); - almost_unique_ids_cache_.tag_dirty(); + unique_ids_cache_.tag_dirty(); } }; diff --git a/source/blender/blenkernel/intern/instances.cc b/source/blender/blenkernel/intern/instances.cc index 5d9f1fb7fc7..0a589216a43 100644 --- a/source/blender/blenkernel/intern/instances.cc +++ b/source/blender/blenkernel/intern/instances.cc @@ -153,7 +153,7 @@ Instances::Instances(Instances &&other) instances_num_(other.instances_num_), attributes_(std::move(other.attributes_)), reference_user_counts_(std::move(other.reference_user_counts_)), - almost_unique_ids_cache_(std::move(other.almost_unique_ids_cache_)) + unique_ids_cache_(std::move(other.unique_ids_cache_)) { } @@ -162,7 +162,7 @@ Instances::Instances(const Instances &other) instances_num_(other.instances_num_), attributes_(other.attributes_), reference_user_counts_(other.reference_user_counts_), - almost_unique_ids_cache_(other.almost_unique_ids_cache_) + unique_ids_cache_(other.unique_ids_cache_) { } @@ -466,10 +466,15 @@ static Array generate_unique_instance_ids(Span original_ids) break; } if (iteration == max_iteration) { - /* It seems to be very unlikely that we ever run into this case (assuming there are less - * than 2^30 instances). However, if that happens, it's better to use an id that is not - * unique than to be stuck in an infinite loop. */ - unique_ids[instance_index] = original_id; + /* The likelyhood of running into this case is very low even if there is a huge number of + * instances. For correctness, it's still good to systematically find an unused id instead + * of purely relying on randomness. */ + for (const int generated_id : IndexRange(INT32_MAX)) { + if (used_unique_ids.add(generated_id)) { + unique_ids[instance_index] = generated_id; + break; + } + } break; } } @@ -495,9 +500,9 @@ Span Instances::reference_user_counts() const return reference_user_counts_.data(); } -Span Instances::almost_unique_ids() const +Span Instances::unique_ids() const { - almost_unique_ids_cache_.ensure([&](Array &r_data) { + unique_ids_cache_.ensure([&](Array &r_data) { const VArraySpan instance_ids = *this->attributes().lookup("id"); if (instance_ids.is_empty()) { r_data.reinitialize(instances_num_); @@ -506,7 +511,7 @@ Span Instances::almost_unique_ids() const } r_data = generate_unique_instance_ids(instance_ids); }); - return almost_unique_ids_cache_.data(); + return unique_ids_cache_.data(); } static float3 get_transform_position(const float4x4 &transform) diff --git a/source/blender/blenkernel/intern/object_dupli.cc b/source/blender/blenkernel/intern/object_dupli.cc index 2f3e4fe1b9a..46f9182b798 100644 --- a/source/blender/blenkernel/intern/object_dupli.cc +++ b/source/blender/blenkernel/intern/object_dupli.cc @@ -986,12 +986,12 @@ static void make_duplis_geometry_set_impl(const DupliContext *ctx, Span instance_offset_matrices = instances->transforms(); Span reference_handles = instances->reference_handles(); - Span almost_unique_ids = instances->almost_unique_ids(); + Span unique_ids = instances->unique_ids(); Span references = instances->references(); for (int64_t i : instance_offset_matrices.index_range()) { const InstanceReference &reference = references[reference_handles[i]]; - const int id = almost_unique_ids[i]; + const int id = unique_ids[i]; const DupliContext *ctx_for_instance = instances_ctx; /* Set the #preview_instance_index when necessary. */