Fix: Memory leak writing to builtin attribute from modifier

If the attribute already existed, but had a different domain or data type
than the user tried to write to it with (i.e. writing to `position` with
a float), then the data from field evaluation wasn't freed.

This restructures the geometry nodes modifier attribute writing process
to solve that problem and remove some of the nested if statements
that made the process confusing.

One case that still doesn't work is writing to a builtin attribute from
a different domain. If `OutputAttribute` gets that feature then that
could be supported here too.

Differential Revision: https://developer.blender.org/D14706

1 files changed, 26 insertions, 22 deletions

diff --git a/source/blender/modifiers/intern/MOD_nodes.cc b/source/blender/modifiers/intern/MOD_nodes.cc
index b904a23eb58..85b6cf6a2cd 100644
--- a/source/blender/modifiers/intern/MOD_nodes.cc
+++ b/source/blender/modifiers/intern/MOD_nodes.cc
@@ -998,30 +998,34 @@ static void store_computed_output_attributes(
     const CustomDataType data_type = blender::bke::cpp_type_to_custom_data_type(store.data.type());
     const std::optional<AttributeMetaData> meta_data = component.attribute_get_meta_data(
         store.name);
-    if (meta_data.has_value() && meta_data->domain == store.domain &&
-        meta_data->data_type == data_type) {
-      /* Copy the data into an existing attribute. */
-      blender::bke::WriteAttributeLookup write_attribute = component.attribute_try_get_for_write(
-          store.name);
-      if (write_attribute) {
-        write_attribute.varray.set_all(store.data.data());
-        if (write_attribute.tag_modified_fn) {
-          write_attribute.tag_modified_fn();
-        }
-      }
-      store.data.type().destruct_n(store.data.data(), store.data.size());
-      MEM_freeN(store.data.data());
+
+    /* Attempt to remove the attribute if it already exists but the domain and type don't match.
+     * Removing the attribute won't succeed if it is built in and non-removable. */
+    if (meta_data.has_value() &&
+        (meta_data->domain != store.domain || meta_data->data_type != data_type)) {
+      component.attribute_try_delete(store.name);
     }
-    else {
-      /* Replace the existing attribute with the new data. */
-      if (meta_data.has_value()) {
-        component.attribute_try_delete(store.name);
-      }
-      component.attribute_try_create(store.name,
-                                     store.domain,
-                                     blender::bke::cpp_type_to_custom_data_type(store.data.type()),
-                                     AttributeInitMove(store.data.data()));
+
+    /* Try to create the attribute reusing the stored buffer. This will only succeed if the
+     * attribute didn't exist before, or if it existed but was removed above. */
+    if (component.attribute_try_create(
+            store.name,
+            store.domain,
+            blender::bke::cpp_type_to_custom_data_type(store.data.type()),
+            AttributeInitMove(store.data.data()))) {
+      continue;
+    }
+
+    OutputAttribute attribute = component.attribute_try_get_for_output_only(
+        store.name, store.domain, data_type);
+    if (attribute) {
+      attribute.varray().set_all(store.data.data());
+      attribute.save();
     }
+
+    /* We were unable to reuse the data, so it must be destructed and freed. */
+    store.data.type().destruct_n(store.data.data(), store.data.size());
+    MEM_freeN(store.data.data());
   }
 }