Fix scene variables "persistentUuid" field in JSON being regenerated or deleted on save (#8837)

Only show in developer changelog

Core/GDCore/Project/Variable.cpp

@@ -356,6 +356,17 @@ Variable& Variable::ResetPersistentUuid() {
   return *this;
 }
 
+Variable& Variable::EnsurePersistentUuid() {
+  if (persistentUuid.empty()) persistentUuid = UUID::MakeUuid4();
+  for (auto& it : children) {
+    it.second->EnsurePersistentUuid();
+  }
+  for (auto& it : childrenArray) {
+    it->EnsurePersistentUuid();
+  }
+  return *this;
+}
+
 Variable& Variable::ClearPersistentUuid() {
   persistentUuid = "";
   for (auto& it : children) {

Core/GDCore/Project/Variable.h

@@ -382,6 +382,14 @@ class GD_CORE_API Variable {
    */
   Variable& ResetPersistentUuid();
 
+  /**
+   * \brief Set the persistent UUID if it is not set already, and do the
+   * same for all the children - contrary to `ResetPersistentUuid`, existing
+   * UUIDs are preserved (so that they stay stable across serializations,
+   * avoiding useless changes in the project file).
+   */
+  Variable& EnsurePersistentUuid();
+
   /**
    * \brief Remove the persistent UUID - when the variable no
    * longer needs to be recognized between serializations.

Core/GDCore/Project/VariablesContainer.cpp

@@ -230,6 +230,15 @@ VariablesContainer& VariablesContainer::ResetPersistentUuid() {
   return *this;
 }
 
+VariablesContainer& VariablesContainer::EnsurePersistentUuids() {
+  if (persistentUuid.empty()) persistentUuid = UUID::MakeUuid4();
+  for (auto& variable : variables) {
+    variable.second->EnsurePersistentUuid();
+  }
+
+  return *this;
+}
+
 VariablesContainer& VariablesContainer::ClearPersistentUuid() {
   persistentUuid = "";
   for (auto& variable : variables) {

Core/GDCore/Project/VariablesContainer.h

@@ -187,6 +187,14 @@ class GD_CORE_API VariablesContainer {
    */
   VariablesContainer& ResetPersistentUuid();
 
+  /**
+   * \brief Set the persistent UUID of the container and all its variables
+   * if they are not set already - contrary to `ResetPersistentUuid`,
+   * existing UUIDs are preserved (so that they stay stable across
+   * serializations, avoiding useless changes in the project file).
+   */
+  VariablesContainer& EnsurePersistentUuids();
+
   /**
    * \brief Remove the persistent UUID - when the variables no
    * longer need to be recognized between serializations.

Core/tests/Variable.cpp

@@ -136,4 +136,39 @@ TEST_CASE("Variable", "[common][variables]") {
 
     REQUIRE(variable != otherVariable);
   }
+  SECTION("EnsurePersistentUuid sets missing UUIDs, recursively") {
+    gd::Variable variable;
+    variable.GetChild("MyChild").SetValue(123);
+    variable.GetChild("MyStructureChild").GetChild("MyGrandChild").SetValue(1);
+
+    REQUIRE(variable.GetPersistentUuid() == "");
+
+    variable.EnsurePersistentUuid();
+
+    REQUIRE(variable.GetPersistentUuid() != "");
+    REQUIRE(variable.GetChild("MyChild").GetPersistentUuid() != "");
+    REQUIRE(variable.GetChild("MyStructureChild")
+                .GetChild("MyGrandChild")
+                .GetPersistentUuid() != "");
+  }
+  SECTION("EnsurePersistentUuid preserves existing UUIDs") {
+    gd::Variable variable;
+    variable.GetChild("MyChild").SetValue(123);
+    variable.ResetPersistentUuid();
+
+    const gd::String uuid = variable.GetPersistentUuid();
+    const gd::String childUuid =
+        variable.GetChild("MyChild").GetPersistentUuid();
+    REQUIRE(uuid != "");
+    REQUIRE(childUuid != "");
+
+    // A new child, without a UUID, is added.
+    variable.GetChild("MyNewChild").SetValue(456);
+
+    variable.EnsurePersistentUuid();
+
+    REQUIRE(variable.GetPersistentUuid() == uuid);
+    REQUIRE(variable.GetChild("MyChild").GetPersistentUuid() == childUuid);
+    REQUIRE(variable.GetChild("MyNewChild").GetPersistentUuid() != "");
+  }
 }

Core/tests/VariablesContainer.cpp

@@ -78,4 +78,35 @@ TEST_CASE("VariablesContainer", "[common][variables]") {
     REQUIRE(mixedValuesVariable.HasMixedValues() == true);
     REQUIRE(mixedTypesVariable.GetType() == gd::Variable::Type::MixedTypes);
   }
+  SECTION("EnsurePersistentUuids sets missing UUIDs and preserves existing ones") {
+    gd::VariablesContainer container;
+    container.InsertNew("Variable1", 0).SetString("Hello World");
+
+    REQUIRE(container.GetPersistentUuid() == "");
+    REQUIRE(container.Get("Variable1").GetPersistentUuid() == "");
+
+    container.EnsurePersistentUuids();
+
+    const gd::String containerUuid = container.GetPersistentUuid();
+    const gd::String variable1Uuid =
+        container.Get("Variable1").GetPersistentUuid();
+    REQUIRE(containerUuid != "");
+    REQUIRE(variable1Uuid != "");
+
+    // A new variable, without a UUID, is added.
+    container.InsertNew("Variable2", 1).SetValue(42);
+    REQUIRE(container.Get("Variable2").GetPersistentUuid() == "");
+
+    container.EnsurePersistentUuids();
+
+    // Existing UUIDs are preserved, the new variable got one.
+    REQUIRE(container.GetPersistentUuid() == containerUuid);
+    REQUIRE(container.Get("Variable1").GetPersistentUuid() == variable1Uuid);
+    REQUIRE(container.Get("Variable2").GetPersistentUuid() != "");
+
+    // Contrary to ResetPersistentUuid, which regenerates everything.
+    container.ResetPersistentUuid();
+    REQUIRE(container.GetPersistentUuid() != containerUuid);
+    REQUIRE(container.Get("Variable1").GetPersistentUuid() != variable1Uuid);
+  }
 }

GDJS/Runtime/types/project-data.d.ts

@@ -165,6 +165,12 @@ declare type VariableData = Readonly<{
   children?: VariableData[];
   /** The type of the variable. Defaults to number. */
   type?: VariableType;
+  /**
+   * A unique identifier, stable across saves of the project, used by the
+   * editor to track the variable (for refactoring after renames...).
+   * Unused by the game engine.
+   */
+  persistentUuid?: string;
 }>;
 
 /** A variable child of a container. Those always have a name. */

GDevelop.js/Bindings/Bindings.idl

@@ -299,7 +299,9 @@ interface Variable {
     void SerializeTo([Ref] SerializerElement element);
     void UnserializeFrom([Const, Ref] SerializerElement element);
     [Ref] Variable ResetPersistentUuid();
+    [Ref] Variable EnsurePersistentUuid();
     [Ref] Variable ClearPersistentUuid();
+    [Const, Ref] DOMString GetPersistentUuid();
 };
 
 enum VariablesContainer_SourceType {
@@ -337,7 +339,9 @@ interface VariablesContainer {
     void SerializeTo([Ref] SerializerElement element);
     void UnserializeFrom([Const, Ref] SerializerElement element);
     [Ref] VariablesContainer ResetPersistentUuid();
+    [Ref] VariablesContainer EnsurePersistentUuids();
     [Ref] VariablesContainer ClearPersistentUuid();
+    [Const, Ref] DOMString GetPersistentUuid();
 };
 
 interface VariablesContainersList {

GDevelop.js/__tests__/Core.js

@@ -1084,6 +1084,25 @@ describe('libGD.js', function () {
       expect(container.getNameAt(1)).toBe('Variable');
       expect(container.getNameAt(2)).toBe('ThirdVariable');
 
+      container.delete();
+    });
+    it('can ensure persistent UUIDs are set, while preserving existing ones', function () {
+      let container = new gd.VariablesContainer();
+      container.insertNew('Variable', 0).setValue(4);
+
+      container.ensurePersistentUuids();
+      const variableUuid = container.get('Variable').getPersistentUuid();
+      expect(variableUuid).toBeTruthy();
+
+      container.insertNew('SecondVariable', 1).setValue(5);
+      expect(container.get('SecondVariable').getPersistentUuid()).toBe('');
+
+      container.ensurePersistentUuids();
+
+      // The existing UUID is preserved, the new variable got one.
+      expect(container.get('Variable').getPersistentUuid()).toBe(variableUuid);
+      expect(container.get('SecondVariable').getPersistentUuid()).toBeTruthy();
+
       container.delete();
     });
   });

GDevelop.js/types.d.ts

@@ -370,7 +370,9 @@ export class Variable extends EmscriptenObject {
   serializeTo(element: SerializerElement): void;
   unserializeFrom(element: SerializerElement): void;
   resetPersistentUuid(): Variable;
+  ensurePersistentUuid(): Variable;
   clearPersistentUuid(): Variable;
+  getPersistentUuid(): string;
 }
 
 export class VariablesContainer extends EmscriptenObject {
@@ -393,7 +395,9 @@ export class VariablesContainer extends EmscriptenObject {
   serializeTo(element: SerializerElement): void;
   unserializeFrom(element: SerializerElement): void;
   resetPersistentUuid(): VariablesContainer;
+  ensurePersistentUuids(): VariablesContainer;
   clearPersistentUuid(): VariablesContainer;
+  getPersistentUuid(): string;
 }
 
 export class VariablesContainersList extends EmscriptenObject {

GDevelop.js/types/gdvariable.js

@@ -40,7 +40,9 @@ declare class gdVariable {
   serializeTo(element: gdSerializerElement): void;
   unserializeFrom(element: gdSerializerElement): void;
   resetPersistentUuid(): gdVariable;
+  ensurePersistentUuid(): gdVariable;
   clearPersistentUuid(): gdVariable;
+  getPersistentUuid(): string;
   delete(): void;
   ptr: number;
 };

GDevelop.js/types/gdvariablescontainer.js

@@ -28,7 +28,9 @@ declare class gdVariablesContainer {
   serializeTo(element: gdSerializerElement): void;
   unserializeFrom(element: gdSerializerElement): void;
   resetPersistentUuid(): gdVariablesContainer;
+  ensurePersistentUuids(): gdVariablesContainer;
   clearPersistentUuid(): gdVariablesContainer;
+  getPersistentUuid(): string;
   delete(): void;
   ptr: number;
 };

newIDE/app/src/ObjectEditor/ObjectEditorDialog.js

@@ -125,17 +125,17 @@ const InnerDialog = (props: InnerDialogProps) => {
   const [objectName, setObjectName] = React.useState(props.objectName);
   const forceUpdate = useForceUpdate();
 
-  // Reset variable UUIDs for changeset tracking. This must happen before
-  // the cancelable editor hook serializes the object, so that both the
-  // serialized "original" state and the in-memory "new" state share
+  // Ensure variable UUIDs are set for changeset tracking. This must happen
+  // before the cancelable editor hook serializes the object, so that both
+  // the serialized "original" state and the in-memory "new" state share
   // the same UUIDs when changes are applied.
-  // We only reset variable UUIDs (not the object's own UUID).
-  // This can be removed once we decide to persist variable UUIDs in the project file.
-  // (and make sure they are properly reset when a variable is added/copied/pasted/etc).
-  const variableUuidsResetRef = React.useRef(false);
-  if (!variableUuidsResetRef.current) {
-    object.getVariables().resetPersistentUuid();
-    variableUuidsResetRef.current = true;
+  // Variables persistent UUIDs are persisted in the project file, so they
+  // must be kept stable: only set them for variables not having one yet.
+  // We only touch variable UUIDs (not the object's own UUID).
+  const variableUuidsEnsuredRef = React.useRef(false);
+  if (!variableUuidsEnsuredRef.current) {
+    object.getVariables().ensurePersistentUuids();
+    variableUuidsEnsuredRef.current = true;
   }
 
   const {
@@ -146,14 +146,7 @@ const InnerDialog = (props: InnerDialogProps) => {
   } = useSerializableObjectCancelableEditor({
     serializableObject: object,
     useProjectToUnserialize: project,
-    onCancel: React.useCallback(
-      () => {
-        // Clear variable UUIDs to avoid them being persisted in the project file.
-        object.getVariables().clearPersistentUuid();
-        onCancel();
-      },
-      [object, onCancel]
-    ),
+    onCancel,
   });
 
   const [hasResourceChanged, setResourceChanged] = React.useState<boolean>(
@@ -216,9 +209,6 @@ const InnerDialog = (props: InnerDialogProps) => {
       );
     }
 
-    // Clear variable UUIDs to avoid them being persisted in the project file.
-    object.getVariables().clearPersistentUuid();
-
     // Do the renaming *after* applying changes, as "withSerializableObject"
     // HOC will unserialize the object to apply modifications, which will
     // override the name.

newIDE/app/src/ObjectGroupEditor/EditedObjectGroupEditorDialog.js

@@ -77,7 +77,12 @@ const EditedObjectGroupEditorDialog = ({
   } = useSerializableObjectCancelableEditor({
     serializableObject: groupVariablesContainer,
     onCancel: () => {},
-    resetThenClearPersistentUuid: true,
+    // The merged container is a temporary object, but its variables keep the
+    // persistent UUIDs of the variables of the first object of the group -
+    // they must be preserved (only set for variables not having one yet), as
+    // they can be copied to the objects of the group when applying changes,
+    // are persisted in the project file and so must stay stable.
+    ensurePersistentUuids: true,
   });
 
   const apply = async () => {
@@ -114,7 +119,6 @@ const EditedObjectGroupEditorDialog = ({
         );
       }
     }
-    groupVariablesContainer.clearPersistentUuid();
   };
 
   const removeObject = React.useCallback(

newIDE/app/src/Utils/SerializableObjectCancelableEditor.js

@@ -12,14 +12,15 @@ type Props = {|
   onCancel: () => void | Promise<void>,
 
   /**
-   * In the future, most serializable objects will be able to have
-   * persistent UUID to identify them uniquely. In the meantime, some
-   * UUIDs are used to check for changes in a serialized object, but must
-   * not be persisted in the project file. In this case, this will
-   * reset the UUIDs (which are probabably not set) and clear them when cancelled.
-   * If you must manually clear them if changes are applied.
+   * Persistent UUIDs are used to track variables when applying refactoring
+   * after changes - and they are also persisted in the project file, so they
+   * must be kept stable to avoid useless changes in it. This will set the
+   * UUIDs of the object (and its sub elements, like variables) not having
+   * one yet, while preserving the existing ones. Nothing is cleared when
+   * cancelled (the original UUIDs are restored with the rest of the
+   * serialized state).
    */
-  resetThenClearPersistentUuid?: boolean,
+  ensurePersistentUuids?: boolean,
 |};
 
 const changesBeforeShowingWarning = 1;
@@ -33,7 +34,7 @@ export const useSerializableObjectCancelableEditor = ({
   serializableObject,
   useProjectToUnserialize,
   onCancel,
-  resetThenClearPersistentUuid,
+  ensurePersistentUuids,
 }: Props): {
   getOriginalContentSerializedElement: () => gdSerializerElement,
   hasUnsavedChanges: () => boolean,
@@ -59,8 +60,7 @@ export const useSerializableObjectCancelableEditor = ({
       // effect fires (in case the parent points to a destroyed object).
       if (!exceptionallyGuardAgainstDeadObject(serializableObject)) return;
 
-      if (resetThenClearPersistentUuid)
-        serializableObject.resetPersistentUuid();
+      if (ensurePersistentUuids) serializableObject.ensurePersistentUuids();
 
       serializedElementRef.current = new gd.SerializerElement();
       serializableObject.serializeTo(serializedElementRef.current);
@@ -72,7 +72,7 @@ export const useSerializableObjectCancelableEditor = ({
         }
       };
     },
-    [serializableObject, resetThenClearPersistentUuid]
+    [serializableObject, ensurePersistentUuids]
   );
 
   const getOriginalContentSerializedElement = React.useCallback(() => {
@@ -130,9 +130,6 @@ export const useSerializableObjectCancelableEditor = ({
         );
       }
 
-      if (resetThenClearPersistentUuid)
-        serializableObject.clearPersistentUuid();
-
       onCancel();
     },
     [
@@ -141,7 +138,6 @@ export const useSerializableObjectCancelableEditor = ({
       onCancel,
       showConfirmation,
       backdropClickBehavior,
-      resetThenClearPersistentUuid,
     ]
   );
 
@@ -159,14 +155,15 @@ type SerializableObjectsCancelableEditorProps = {|
   onCancel: () => void | Promise<void>,
 
   /**
-   * In the future, most serializable objects will be able to have
-   * persistent UUID to identify them uniquely. In the meantime, some
-   * UUIDs are used to check for changes in a serialized object, but must
-   * not be persisted in the project file. In this case, this will
-   * reset the UUIDs (which are probabably not set) and clear them when cancelled.
-   * If you must manually clear them if changes are applied.
+   * Persistent UUIDs are used to track variables when applying refactoring
+   * after changes - and they are also persisted in the project file, so they
+   * must be kept stable to avoid useless changes in it. This will set the
+   * UUIDs of the objects (and their sub elements, like variables) not having
+   * one yet, while preserving the existing ones. Nothing is cleared when
+   * cancelled (the original UUIDs are restored with the rest of the
+   * serialized state).
    */
-  resetThenClearPersistentUuid?: boolean,
+  ensurePersistentUuids?: boolean,
 |};
 
 /**
@@ -178,7 +175,7 @@ export const useSerializableObjectsCancelableEditor = ({
   serializableObjects,
   useProjectToUnserialize,
   onCancel,
-  resetThenClearPersistentUuid,
+  ensurePersistentUuids,
 }: SerializableObjectsCancelableEditorProps): {
   getOriginalContentSerializedElements: () => Map<string, gdSerializerElement>,
   hasUnsavedChanges: () => boolean,
@@ -210,8 +207,8 @@ export const useSerializableObjectsCancelableEditor = ({
       // effect fires (in case the parent points to a destroyed object).
       if (!exceptionallyGuardAgainstDeadObject(serializableObject)) continue;
 
-      if (resetThenClearPersistentUuid) {
-        serializableObject.resetPersistentUuid();
+      if (ensurePersistentUuids) {
+        serializableObject.ensurePersistentUuids();
       }
       const serializedElement = new gd.SerializerElement();
       serializableObject.serializeTo(serializedElement);
@@ -287,10 +284,6 @@ export const useSerializableObjectsCancelableEditor = ({
         } else {
           serializableObject.unserializeFrom(serializedElement);
         }
-
-        if (resetThenClearPersistentUuid) {
-          serializableObject.clearPersistentUuid();
-        }
       }
 
       onCancel();
@@ -301,7 +294,6 @@ export const useSerializableObjectsCancelableEditor = ({
       showConfirmation,
       serializableObjects,
       useProjectToUnserialize,
-      resetThenClearPersistentUuid,
     ]
   );
 

newIDE/app/src/VariablesList/ObjectGroupVariablesDialog.js

@@ -69,7 +69,12 @@ const ObjectGroupVariablesDialog = ({
   } = useSerializableObjectCancelableEditor({
     serializableObject: groupVariablesContainer,
     onCancel: () => {},
-    resetThenClearPersistentUuid: true,
+    // The merged container is a temporary object, but its variables keep the
+    // persistent UUIDs of the variables of the first object of the group -
+    // they must be preserved (only set for variables not having one yet), as
+    // they can be copied to the objects of the group when applying changes,
+    // are persisted in the project file and so must stay stable.
+    ensurePersistentUuids: true,
   });
 
   const apply = async () => {
@@ -112,7 +117,6 @@ const ObjectGroupVariablesDialog = ({
         );
       }
     }
-    groupVariablesContainer.clearPersistentUuid();
   };
 
   const lastSelectedVariableNodeId = React.useRef<string | null>(null);

newIDE/app/src/VariablesList/VariablesEditorDialog.js

@@ -97,7 +97,9 @@ const VariablesEditorDialog = ({
   } = useSerializableObjectsCancelableEditor({
     serializableObjects,
     onCancel,
-    resetThenClearPersistentUuid: true,
+    // Variables persistent UUIDs are persisted in the project file, so they
+    // must be kept stable: only set them for variables not having one yet.
+    ensurePersistentUuids: true,
   });
 
   const lastSelectedVariableNodeId = React.useRef<string | null>(null);
@@ -187,7 +189,6 @@ const VariablesEditorDialog = ({
             );
           }
         }
-        variablesContainer.clearPersistentUuid();
       }
       const tab = tabs.find(({ id }) => id === currentTab);
       if (tab) {

newIDE/app/src/VariablesList/useVariablesContainerRefactoring.js

@@ -152,10 +152,12 @@ const useVariablesContainerRefactoring = ({
       console.error('Error applying variable refactoring:', error);
     }
 
-    // Take a fresh snapshot for the next cycle.
+    // Take a fresh snapshot for the next cycle. Only ensure UUIDs are set
+    // (for newly added variables) - existing UUIDs are preserved, as they
+    // are persisted in the project file and must stay stable to avoid
+    // useless changes in it.
     snapshot.delete();
-    variablesContainer.clearPersistentUuid();
-    variablesContainer.resetPersistentUuid();
+    variablesContainer.ensurePersistentUuids();
     const newSnapshot = new gd.SerializerElement();
     variablesContainer.serializeTo(newSnapshot);
     snapshotRef.current = newSnapshot;
@@ -166,7 +168,10 @@ const useVariablesContainerRefactoring = ({
       if (!enabled) return;
 
       // Setup: snapshot the current state of the variables container.
-      variablesContainer.resetPersistentUuid();
+      // Only ensure UUIDs are set (for variables that don't have one yet) -
+      // existing UUIDs are preserved, as they are persisted in the project
+      // file and must stay stable to avoid useless changes in it.
+      variablesContainer.ensurePersistentUuids();
       const snapshot = new gd.SerializerElement();
       variablesContainer.serializeTo(snapshot);
       snapshotRef.current = snapshot;
@@ -180,14 +185,6 @@ const useVariablesContainerRefactoring = ({
           timerRef.current = null;
         }
 
-        // Try to clear persistent UUIDs so they are not persisted in the
-        // project file. This may fail if the object was already deleted.
-        try {
-          variablesContainer.clearPersistentUuid();
-        } catch (error) {
-          // The container might already be deleted.
-        }
-
         // Free the snapshot C++ memory.
         if (snapshotRef.current) {
           snapshotRef.current.delete();