Depsgraph: Use UUID to match modifiers

Solves possible pointer-based comparison fiasco.

Another nice outcome of this is that topology cache will now be
preserved throughout the undo system. For example, undo of object
transform will not require topology cache to be re-created.

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

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_modifier.cc

@@ -23,29 +23,15 @@
 
 #include "intern/eval/deg_eval_runtime_backup_modifier.h"
 
+#include "DNA_modifier_types.h"
+
 namespace blender {
 namespace deg {
 
-ModifierDataBackupID::ModifierDataBackupID(const Depsgraph * /*depsgraph*/)
+ModifierDataBackup::ModifierDataBackup(ModifierData *modifier_data)
-    : ModifierDataBackupID(nullptr, eModifierType_None)
+    : type(static_cast<ModifierType>(modifier_data->type)), runtime(modifier_data->runtime)
 {
 }
 
-ModifierDataBackupID::ModifierDataBackupID(ModifierData *modifier_data, ModifierType type)
-    : modifier_data(modifier_data), type(type)
-{
-}
-
-bool operator==(const ModifierDataBackupID &a, const ModifierDataBackupID &b)
-{
-  return a.modifier_data == b.modifier_data && a.type == b.type;
-}
-
-uint64_t ModifierDataBackupID::hash() const
-{
-  uintptr_t ptr = (uintptr_t)modifier_data;
-  return (ptr >> 4) ^ (uintptr_t)type;
-}
-
 }  // namespace deg
 }  // namespace blender

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_modifier.h

@@ -25,38 +25,18 @@
 
 #include "BKE_modifier.h"
 
-#include "intern/depsgraph_type.h"
-
 struct ModifierData;
 
 namespace blender {
 namespace deg {
 
-struct Depsgraph;
+class ModifierDataBackup {
-
-/* Identifier used to match modifiers to backup/restore their runtime data.
- * Identification is happening using original modifier data pointer and the
- * modifier type.
- * It is not enough to only pointer, since it's possible to have a situation
- * when modifier is removed and a new one added, and due to memory allocation
- * policy they might have same pointer.
- * By adding type into matching we are at least ensuring that modifier will not
- * try to interpret runtime data created by another modifier type. */
-class ModifierDataBackupID {
  public:
-  ModifierDataBackupID(const Depsgraph *depsgraph);
+  explicit ModifierDataBackup(ModifierData *modifier_data);
-  ModifierDataBackupID(ModifierData *modifier_data, ModifierType type);
 
-  friend bool operator==(const ModifierDataBackupID &a, const ModifierDataBackupID &b);
-
-  uint64_t hash() const;
-
-  ModifierData *modifier_data;
   ModifierType type;
+  void *runtime;
 };
 
-/* Storage for backed up runtime modifier data. */
-typedef Map<ModifierDataBackupID, void *> ModifierRuntimeDataBackup;
-
 }  // namespace deg
 }  // namespace blender

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_object.cc

@@ -62,21 +62,18 @@ void ObjectRuntimeBackup::init_from_object(Object *object)
   backup_pose_channel_runtime_data(object);
 }
 
-inline ModifierDataBackupID create_modifier_data_id(const ModifierData *modifier_data)
-{
-  return ModifierDataBackupID(modifier_data->orig_modifier_data,
-                              static_cast<ModifierType>(modifier_data->type));
-}
-
 void ObjectRuntimeBackup::backup_modifier_runtime_data(Object *object)
 {
   LISTBASE_FOREACH (ModifierData *, modifier_data, &object->modifiers) {
     if (modifier_data->runtime == nullptr) {
       continue;
     }
+
+    const SessionUUID &session_uuid = modifier_data->session_uuid;
+    BLI_assert(BLI_session_uuid_is_generated(&session_uuid));
+
     BLI_assert(modifier_data->orig_modifier_data != nullptr);
-    ModifierDataBackupID modifier_data_id = create_modifier_data_id(modifier_data);
+    modifier_runtime_data.add(session_uuid, ModifierDataBackup(modifier_data));
-    modifier_runtime_data.add(modifier_data_id, modifier_data->runtime);
     modifier_data->runtime = nullptr;
   }
 }
@@ -153,17 +150,17 @@ void ObjectRuntimeBackup::restore_modifier_runtime_data(Object *object)
 {
   LISTBASE_FOREACH (ModifierData *, modifier_data, &object->modifiers) {
     BLI_assert(modifier_data->orig_modifier_data != nullptr);
-    ModifierDataBackupID modifier_data_id = create_modifier_data_id(modifier_data);
+    const SessionUUID &session_uuid = modifier_data->session_uuid;
-    void *runtime = modifier_runtime_data.pop_default(modifier_data_id, nullptr);
+    optional<ModifierDataBackup> backup = modifier_runtime_data.pop_try(session_uuid);
-    if (runtime != nullptr) {
+    if (backup.has_value()) {
-      modifier_data->runtime = runtime;
+      modifier_data->runtime = backup->runtime;
     }
   }
 
-  for (ModifierRuntimeDataBackup::Item item : modifier_runtime_data.items()) {
+  for (ModifierDataBackup &backup : modifier_runtime_data.values()) {
-    const ModifierTypeInfo *modifier_type_info = BKE_modifier_get_info(item.key.type);
+    const ModifierTypeInfo *modifier_type_info = BKE_modifier_get_info(backup.type);
     BLI_assert(modifier_type_info != nullptr);
-    modifier_type_info->freeRuntimeData(item.value);
+    modifier_type_info->freeRuntimeData(backup.runtime);
   }
 }
 

source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_object.h

@@ -36,6 +36,8 @@ struct Object;
 namespace blender {
 namespace deg {
 
+struct Depsgraph;
+
 class ObjectRuntimeBackup {
  public:
   ObjectRuntimeBackup(const Depsgraph *depsgraph);
@@ -56,7 +58,7 @@ class ObjectRuntimeBackup {
   Object_Runtime runtime;
   short base_flag;
   unsigned short base_local_view_bits;
-  ModifierRuntimeDataBackup modifier_runtime_data;
+  Map<SessionUUID, ModifierDataBackup> modifier_runtime_data;
   Map<SessionUUID, bPoseChannel_Runtime> pose_channel_runtime_data;
 };