ID: Fix failing test cases.

This fixes failing test cases when using `make test`.
See {D13615} for more information.

The fix will perform the id remapping one item at a time. Although not
really nice, this isn't a bottleneck.

The failing test cases is because space_node stores pointers multiple
times and didn't update all pointers. It was not clear why it didn't do
it, but changing the behavior more to the previous behavior fixes the
issue at hand.

I prefer to remove the double storage of the node tree pointers (in
snode and path) to reduce pointer management complexity.

1 files changed, 64 insertions, 38 deletions

diff --git a/source/blender/editors/space_node/space_node.cc b/source/blender/editors/space_node/space_node.cc
index 8f465ffbf80..00fd328b2ed 100644
--- a/source/blender/editors/space_node/space_node.cc
+++ b/source/blender/editors/space_node/space_node.cc
@@ -897,66 +897,92 @@ static void node_widgets()
   WM_gizmogrouptype_append_and_link(gzmap_type, NODE_GGT_backdrop_corner_pin);
 }
 
-static void node_id_remap(ScrArea *UNUSED(area),
-                          SpaceLink *slink,
-                          const struct IDRemapper *mappings)
+static void node_id_remap_cb(ID *old_id, ID *new_id, void *user_data)
 {
-  SpaceNode *snode = (SpaceNode *)slink;
+  SpaceNode *snode = static_cast<SpaceNode *>(user_data);
 
-  if (ELEM(BKE_id_remapper_apply(mappings, &snode->id, ID_REMAP_APPLY_DEFAULT),
-           ID_REMAP_RESULT_SOURCE_REMAPPED,
-           ID_REMAP_RESULT_SOURCE_UNASSIGNED)) {
+  if (snode->id == old_id) {
     /* nasty DNA logic for SpaceNode:
      * ideally should be handled by editor code, but would be bad level call
      */
     BLI_freelistN(&snode->treepath);
 
     /* XXX Untested in case new_id != nullptr... */
+    snode->id = new_id;
     snode->from = nullptr;
     snode->nodetree = nullptr;
     snode->edittree = nullptr;
   }
-  if (BKE_id_remapper_apply(mappings, &snode->from, ID_REMAP_APPLY_DEFAULT) ==
-      ID_REMAP_RESULT_SOURCE_UNASSIGNED) {
-    snode->flag &= ~SNODE_PIN;
+  else if (GS(old_id->name) == ID_OB) {
+    if (snode->from == old_id) {
+      if (new_id == nullptr) {
+        snode->flag &= ~SNODE_PIN;
+      }
+      snode->from = new_id;
+    }
   }
-  BKE_id_remapper_apply(mappings, (ID **)&snode->gpd, ID_REMAP_APPLY_UPDATE_REFCOUNT);
-
-  if (!BKE_id_remapper_has_mapping_for(mappings, FILTER_ID_NT)) {
-    return;
+  else if (GS(old_id->name) == ID_GD) {
+    if ((ID *)snode->gpd == old_id) {
+      snode->gpd = (bGPdata *)new_id;
+      id_us_min(old_id);
+      id_us_plus(new_id);
+    }
   }
+  else if (GS(old_id->name) == ID_NT) {
+    bNodeTreePath *path, *path_next;
 
-  bNodeTreePath *path, *path_next;
-  for (path = (bNodeTreePath *)snode->treepath.first; path; path = path->next) {
-    BKE_id_remapper_apply(mappings, (ID **)&path->nodetree, ID_REMAP_APPLY_ENSURE_REAL);
-    if (path == snode->treepath.first) {
-      /* first nodetree in path is same as snode->nodetree */
-      snode->nodetree = path->nodetree;
-    }
-    if (path->nodetree == nullptr) {
-      break;
+    for (path = (bNodeTreePath *)snode->treepath.first; path; path = path->next) {
+      if ((ID *)path->nodetree == old_id) {
+        path->nodetree = (bNodeTree *)new_id;
+        id_us_ensure_real(new_id);
+      }
+      if (path == snode->treepath.first) {
+        /* first nodetree in path is same as snode->nodetree */
+        snode->nodetree = path->nodetree;
+      }
+      if (path->nodetree == nullptr) {
+        break;
+      }
     }
-  }
 
-  /* remaining path entries are invalid, remove */
-  for (; path; path = path_next) {
-    path_next = path->next;
+    /* remaining path entries are invalid, remove */
+    for (; path; path = path_next) {
+      path_next = path->next;
 
-    BLI_remlink(&snode->treepath, path);
-    MEM_freeN(path);
-  }
+      BLI_remlink(&snode->treepath, path);
+      MEM_freeN(path);
+    }
 
-  /* edittree is just the last in the path,
-   * set this directly since the path may have been shortened above */
-  if (snode->treepath.last) {
-    path = (bNodeTreePath *)snode->treepath.last;
-    snode->edittree = path->nodetree;
-  }
-  else {
-    snode->edittree = nullptr;
+    /* edittree is just the last in the path,
+     * set this directly since the path may have been shortened above */
+    if (snode->treepath.last) {
+      path = (bNodeTreePath *)snode->treepath.last;
+      snode->edittree = path->nodetree;
+    }
+    else {
+      snode->edittree = nullptr;
+    }
   }
 }
 
+static void node_id_remap(ScrArea *UNUSED(area),
+                          SpaceLink *slink,
+                          const struct IDRemapper *mappings)
+{
+  /* Although we should be able to perform all the mappings in a single go this lead to issues when
+   * running the python test cases. Somehow the nodetree/edittree weren't updated to the new
+   * pointers that generated a SEGFAULT.
+   *
+   * To move forward we should perhaps remove snode->edittree and snode->nodetree as they are just
+   * copies of pointers. All usages should be calling a function that will receive the appropriate
+   * instance.
+   *
+   * We could also move a remap address at a time to ise the IDRemapper as that should get closer
+   * to cleaner code. See {D13615} for more information about this topic.
+   */
+  BKE_id_remapper_iter(mappings, node_id_remap_cb, slink);
+}
+
 static int node_space_subtype_get(ScrArea *area)
 {
   SpaceNode *snode = (SpaceNode *)area->spacedata.first;