[Bf-blender-cvs] [f100fd4609d] node-add-asset-menu: Fix memory leaks

[Hans Goudey]

Commit: f100fd4609d15a09ae8331c4b26382a53fd33cec
Author: Hans Goudey
Date:   Thu Oct 6 13:57:10 2022 -0500
Branches: node-add-asset-menu
https://developer.blender.org/rBf100fd4609d15a09ae8331c4b26382a53fd33cec

Fix memory leaks

The storage is owned by the node editor until the asset system is more complete

===================================================================

M	source/blender/editors/space_node/add_menu_assets.cc
M	source/blender/editors/space_node/node_intern.hh
M	source/blender/editors/space_node/space_node.cc

===================================================================

diff --git a/source/blender/editors/space_node/add_menu_assets.cc b/source/blender/editors/space_node/add_menu_assets.cc
index cf72f00bf79..d20eb635bd3 100644
--- a/source/blender/editors/space_node/add_menu_assets.cc
+++ b/source/blender/editors/space_node/add_menu_assets.cc
@@ -38,6 +38,7 @@ struct LibraryCatalog {
 struct AssetItemTree {
   bke::AssetCatalogTree catalogs;
   MultiValueMap<bke::AssetCatalogPath, LibraryAsset> assets_per_path;
+  Map<const bke::AssetCatalogTreeItem *, bke::AssetCatalogPath> full_catalog_per_tree_item;
 };
 
 static AssetItemTree build_catalog_tree(const bContext &C, const bNodeTree &node_tree)
@@ -100,13 +101,28 @@ static AssetItemTree build_catalog_tree(const bContext &C, const bNodeTree &node
     }
   });
 
-  return {std::move(catalogs_with_node_assets), std::move(assets_per_path)};
+  /* Build another map storing full asset paths for each tree item, in order to have stable
+   * pointers to asset catalog paths to use for context pointers. This is necessary because
+   * #bke::AssetCatalogTreeItem doesn't store its full path directly. */
+  Map<const bke::AssetCatalogTreeItem *, bke::AssetCatalogPath> full_catalog_per_tree_item;
+  catalogs_with_node_assets.foreach_item([&](bke::AssetCatalogTreeItem &item) {
+    full_catalog_per_tree_item.add_new(&item, item.catalog_path());
+  });
+
+  return {std::move(catalogs_with_node_assets),
+          std::move(assets_per_path),
+          std::move(full_catalog_per_tree_item)};
 }
 
 static void node_add_catalog_assets_draw(const bContext *C, Menu *menu)
 {
   bScreen &screen = *CTX_wm_screen(C);
   const SpaceNode &snode = *CTX_wm_space_node(C);
+  if (!snode.runtime->assets_for_menu) {
+    BLI_assert_unreachable();
+    return;
+  }
+  AssetItemTree &tree = *snode.runtime->assets_for_menu;
   const bNodeTree *edit_tree = snode.edittree;
   if (!edit_tree) {
     return;
@@ -119,7 +135,6 @@ static void node_add_catalog_assets_draw(const bContext *C, Menu *menu)
   const bke::AssetCatalogPath &menu_path = *static_cast<const bke::AssetCatalogPath *>(
       menu_path_ptr.data);
 
-  AssetItemTree tree = build_catalog_tree(*C, *edit_tree);
   const Span<LibraryAsset> asset_items = tree.assets_per_path.lookup(menu_path);
   bke::AssetCatalogTreeItem *catalog_item = tree.catalogs.find_item(menu_path);
   BLI_assert(catalog_item != nullptr);
@@ -137,18 +152,18 @@ static void node_add_catalog_assets_draw(const bContext *C, Menu *menu)
         &screen.id, &RNA_FileSelectEntry, const_cast<FileDirEntry *>(item.handle.file_data)};
     uiLayoutSetContextPointer(row, "active_file", &file);
 
-    PointerRNA library_ptr{
-        &screen.id, &RNA_AssetLibraryReference, new AssetLibraryReference(item.library_ref)};
+    PointerRNA library_ptr{&screen.id,
+                           &RNA_AssetLibraryReference,
+                           const_cast<AssetLibraryReference *>(&item.library_ref)};
     uiLayoutSetContextPointer(row, "asset_library_ref", &library_ptr);
 
     uiItemO(row, ED_asset_handle_get_name(&item.handle), ICON_NONE, "NODE_OT_add_group_asset");
   }
 
   catalog_item->foreach_child([&](bke::AssetCatalogTreeItem &child_item) {
-    const bke::AssetCatalogPath path = child_item.catalog_path();
-
-    /* TODO: Where should this memory live? */
-    PointerRNA path_ptr{&screen.id, &RNA_AssetCatalogPath, new bke::AssetCatalogPath(path)};
+    const bke::AssetCatalogPath &path = tree.full_catalog_per_tree_item.lookup(&child_item);
+    PointerRNA path_ptr{
+        &screen.id, &RNA_AssetCatalogPath, const_cast<bke::AssetCatalogPath *>(&path)};
     uiLayout *row = uiLayoutRow(layout, false);
     uiLayoutSetContextPointer(row, "asset_catalog_path", &path_ptr);
     uiItemM(row, "NODE_MT_node_add_catalog_assets", path.name().c_str(), ICON_NONE);
@@ -158,10 +173,13 @@ static void node_add_catalog_assets_draw(const bContext *C, Menu *menu)
 static void add_root_catalogs_draw(const bContext *C, Menu *menu)
 {
   bScreen &screen = *CTX_wm_screen(C);
-  const SpaceNode &snode = *CTX_wm_space_node(C);
+  SpaceNode &snode = *CTX_wm_space_node(C);
   const bNodeTree *edit_tree = snode.edittree;
 
-  AssetItemTree tree = build_catalog_tree(*C, *edit_tree);
+  snode.runtime->assets_for_menu.reset();
+  snode.runtime->assets_for_menu = std::make_shared<AssetItemTree>(
+      build_catalog_tree(*C, *edit_tree));
+  AssetItemTree &tree = *snode.runtime->assets_for_menu;
   if (tree.catalogs.is_empty()) {
     return;
   }
@@ -170,10 +188,9 @@ static void add_root_catalogs_draw(const bContext *C, Menu *menu)
   uiItemS(layout);
 
   tree.catalogs.foreach_root_item([&](bke::AssetCatalogTreeItem &item) {
-    const bke::AssetCatalogPath path = item.catalog_path();
-
-    /* TODO: Where should this memory live? */
-    PointerRNA path_ptr{&screen.id, &RNA_AssetCatalogPath, new bke::AssetCatalogPath(path)};
+    const bke::AssetCatalogPath &path = tree.full_catalog_per_tree_item.lookup(&item);
+    PointerRNA path_ptr{
+        &screen.id, &RNA_AssetCatalogPath, const_cast<bke::AssetCatalogPath *>(&path)};
     uiLayout *row = uiLayoutRow(layout, false);
     uiLayoutSetContextPointer(row, "asset_catalog_path", &path_ptr);
     uiItemM(row, "NODE_MT_node_add_catalog_assets", path.name().c_str(), ICON_NONE);
diff --git a/source/blender/editors/space_node/node_intern.hh b/source/blender/editors/space_node/node_intern.hh
index f4d992f754e..fe32109ed96 100644
--- a/source/blender/editors/space_node/node_intern.hh
+++ b/source/blender/editors/space_node/node_intern.hh
@@ -38,6 +38,8 @@ extern const char *node_context_dir[];
 
 namespace blender::ed::space_node {
 
+struct AssetItemTree;
+
 /** Temporary data used in node link drag modal operator. */
 struct bNodeLinkDrag {
   /** Links dragged by the operator. */
@@ -96,6 +98,15 @@ struct SpaceNode_Runtime {
   /* XXX hack for translate_attach op-macros to pass data from transform op to insert_offset op */
   /** Temporary data for node insert offset (in UI called Auto-offset). */
   struct NodeInsertOfsData *iofsd;
+
+  /**
+   * Temporary data for node add menu in order to provide longer-term storage for context pointers.
+   * Recreated every time the root menu is opened. In the future this will be replaced with an "all
+   * libraries" cache in the asset system itself.
+   *
+   * Stored with a shared pointer so that it can be forward declared.
+   */
+  std::shared_ptr<AssetItemTree> assets_for_menu;
 };
 
 enum NodeResizeDirection {
diff --git a/source/blender/editors/space_node/space_node.cc b/source/blender/editors/space_node/space_node.cc
index 9ae5b846fd0..5754e77399f 100644
--- a/source/blender/editors/space_node/space_node.cc
+++ b/source/blender/editors/space_node/space_node.cc
@@ -300,7 +300,7 @@ static void node_free(SpaceLink *sl)
 
   if (snode->runtime) {
     snode->runtime->linkdrag.reset();
-    MEM_freeN(snode->runtime);
+    MEM_delete(snode->runtime);
   }
 }