Fix potential crashes in events function edition (#7204)

* Also hide object groups tabs in functions (unless there are groups already) - it's not recommended to use them anymore

Core/GDCore/Extensions/Metadata/ParameterMetadataTools.cpp

@@ -6,15 +6,15 @@
 #include "ParameterMetadataTools.h"
 
 #include "GDCore/Events/Expression.h"
+#include "GDCore/Events/Parsers/ExpressionParser2.h"
+#include "GDCore/Events/Parsers/ExpressionParser2NodePrinter.h"
 #include "GDCore/Project/Object.h"
 #include "GDCore/Project/ObjectsContainer.h"
 #include "GDCore/Project/ObjectsContainersList.h"
 #include "GDCore/Project/ParameterMetadataContainer.h"
 #include "GDCore/Project/Project.h"
 #include "GDCore/String.h"
 #include "InstructionMetadata.h"
-#include "GDCore/Events/Parsers/ExpressionParser2.h"
-#include "GDCore/Events/Parsers/ExpressionParser2NodePrinter.h"
 
 namespace gd {
 const ParameterMetadata ParameterMetadataTools::badParameterMetadata;
@@ -23,42 +23,98 @@ void ParameterMetadataTools::ParametersToObjectsContainer(
     const gd::Project& project,
     const ParameterMetadataContainer& parameters,
     gd::ObjectsContainer& outputObjectsContainer) {
-  outputObjectsContainer.GetObjects().clear();
+  // Keep track of all objects and their behaviors names, so we can remove
+  // those who are in the container but not in the parameters anymore.
+  std::set<gd::String> allObjectNames;
+  std::map<gd::String, std::set<gd::String>> allObjectBehaviorNames;
 
   gd::String lastObjectName;
   for (std::size_t i = 0; i < parameters.GetParametersCount(); ++i) {
     const auto& parameter = parameters.GetParameter(i);
     if (parameter.GetName().empty()) continue;
 
     if (gd::ParameterMetadata::IsObject(parameter.GetType())) {
-      outputObjectsContainer.InsertNewObject(
-          project,
-          parameter.GetExtraInfo(),
-          parameter.GetName(),
-          outputObjectsContainer.GetObjectsCount());
+      const gd::String& objectName = parameter.GetName();
+      const gd::String& objectType = parameter.GetExtraInfo();
+      allObjectNames.insert(objectName);
+
+      // Check if we can keep the existing object.
+      if (outputObjectsContainer.HasObjectNamed(objectName)) {
+        const gd::Object& object = outputObjectsContainer.GetObject(objectName);
+
+        if (object.GetType() != objectType) {
+          // Object type has changed, remove it so it is re-created.
+          outputObjectsContainer.RemoveObject(objectName);
+        }
+      }
+
+      if (outputObjectsContainer.HasObjectNamed(objectName)) {
+        // Keep the existing object - behaviors will be added or removed later.
+      } else {
+        outputObjectsContainer.InsertNewObject(
+            project,
+            parameter.GetExtraInfo(),
+            objectName,
+            outputObjectsContainer.GetObjectsCount());
+      }
 
       // Memorize the last object name. By convention, parameters that require
       // an object (mainly, "objectvar" and "behavior") should be placed after
       // the object in the list of parameters (if possible, just after).
       // Search "lastObjectName" in the codebase for other place where this
       // convention is enforced.
-      lastObjectName = parameter.GetName();
+      lastObjectName = objectName;
     } else if (gd::ParameterMetadata::IsBehavior(parameter.GetType())) {
       if (!lastObjectName.empty()) {
         if (outputObjectsContainer.HasObjectNamed(lastObjectName)) {
-          const gd::Object& object =
-              outputObjectsContainer.GetObject(lastObjectName);
-          gd::String behaviorName = parameter.GetName();
+          const gd::String& behaviorName = parameter.GetName();
+          const gd::String& behaviorType = parameter.GetExtraInfo();
+
+          gd::Object& object = outputObjectsContainer.GetObject(lastObjectName);
+          allObjectBehaviorNames[lastObjectName].insert(behaviorName);
+
+          // Check if we can keep the existing behavior.
+          if (object.HasBehaviorNamed(behaviorName)) {
+            if (object.GetBehavior(behaviorName).GetTypeName() !=
+                behaviorType) {
+              // Behavior type has changed, remove it so it is re-created.
+              object.RemoveBehavior(behaviorName);
+            }
+          }
 
           if (!object.HasBehaviorNamed(behaviorName)) {
-            outputObjectsContainer.GetObject(lastObjectName)
-                .AddNewBehavior(
-                    project, parameter.GetExtraInfo(), behaviorName);
+            object.AddNewBehavior(
+                project, parameter.GetExtraInfo(), behaviorName);
           }
         }
       }
     }
   }
+
+  // Remove objects that are not in the parameters anymore.
+  std::set<gd::String> objectNamesInContainer =
+      outputObjectsContainer.GetAllObjectNames();
+  for (const auto& objectName : objectNamesInContainer) {
+    if (allObjectNames.find(objectName) == allObjectNames.end()) {
+      outputObjectsContainer.RemoveObject(objectName);
+    }
+  }
+
+  // Remove behaviors of objects that are not in the parameters anymore.
+  for (const auto& objectName : allObjectNames) {
+    if (!outputObjectsContainer.HasObjectNamed(objectName)) {
+      // Should not happen.
+      continue;
+    }
+
+    auto& object = outputObjectsContainer.GetObject(objectName);
+    for (const auto& behaviorName : object.GetAllBehaviorNames()) {
+      const auto& allBehaviorNames = allObjectBehaviorNames[objectName];
+      if (allBehaviorNames.find(behaviorName) == allBehaviorNames.end()) {
+        object.RemoveBehavior(behaviorName);
+      }
+    }
+  }
 }
 
 void ParameterMetadataTools::ForEachParameterMatchingSearch(

Core/GDCore/IDE/EventsFunctionTools.cpp

@@ -24,15 +24,16 @@ void EventsFunctionTools::FreeEventsFunctionToObjectsContainer(
     const gd::EventsFunction& eventsFunction,
     gd::ObjectsContainer& outputObjectsContainer) {
   // Functions scope for objects is defined according
-  // to parameters
-  outputObjectsContainer.GetObjects().clear();
-  outputObjectsContainer.GetObjectGroups().Clear();
-
+  // to parameters.
   auto &parameters = eventsFunction.GetParametersForEvents(functionContainer);
   gd::ParameterMetadataTools::ParametersToObjectsContainer(
       project,
       parameters,
       outputObjectsContainer);
+
+  // TODO: in theory we should ensure stability of the groups across calls
+  // to this function. BUT groups in functions should probably have never been
+  // supported, so we're phasing this out in the UI.
   outputObjectsContainer.GetObjectGroups() = eventsFunction.GetObjectGroups();
 }
 

Core/GDCore/Project/ObjectGroup.h

@@ -20,8 +20,8 @@ namespace gd {
 /**
  * \brief Represents an object group.
  *
- * Objects groups do not really contains objects : They are just used in events,
- * so as to create events which can be applied to several objects.
+ * Objects groups do not really contains objects: they are just used in events,
+ * to create events which can be applied to several objects.
  *
  * \ingroup PlatformDefinition
  */

Core/GDCore/Project/ObjectsContainer.cpp

@@ -205,6 +205,14 @@ void ObjectsContainer::MoveObjectFolderOrObjectToAnotherContainerInFolder(
       objectFolderOrObject, newParentFolder, newPosition);
 }
 
+std::set<gd::String> ObjectsContainer::GetAllObjectNames() const {
+  std::set<gd::String> names;
+  for (const auto& object : initialObjects) {
+    names.insert(object->GetName());
+  }
+  return names;
+}
+
 std::vector<const ObjectFolderOrObject*>
 ObjectsContainer::GetAllObjectFolderOrObjects() const {
   std::vector<const ObjectFolderOrObject*> results;

Core/GDCore/Project/ObjectsContainer.h

@@ -7,6 +7,7 @@
 #define GDCORE_OBJECTSCONTAINER_H
 #include <memory>
 #include <vector>
+#include <set>
 #include "GDCore/String.h"
 #include "GDCore/Project/ObjectGroupsContainer.h"
 #include "GDCore/Project/ObjectFolderOrObject.h"
@@ -40,7 +41,7 @@ class GD_CORE_API ObjectsContainer {
    */
   ObjectsContainer();
   virtual ~ObjectsContainer();
-  
+
   ObjectsContainer(const ObjectsContainer&);
   ObjectsContainer& operator=(const ObjectsContainer& rhs);
 
@@ -168,6 +169,8 @@ class GD_CORE_API ObjectsContainer {
   const std::vector<std::unique_ptr<gd::Object> >& GetObjects() const {
     return initialObjects;
   }
+
+  std::set<gd::String> GetAllObjectNames() const;
   ///@}
 
   /**

GDevelop.js/__tests__/Core.js

@@ -4261,15 +4261,27 @@ describe('libGD.js', function () {
         .setExtraInfo('Sprite')
         .setDescription('The second object to be used, a sprite');
 
+      parameters
+        .addNewParameter('MyFirstBehavior')
+        .setType('behavior')
+        .setExtraInfo('SomeBehavior::FirstBehaviorType')
+        .setDescription('The first behavior of the first sprite');
+
+      parameters
+        .addNewParameter('MySecondBehavior')
+        .setType('behavior')
+        .setExtraInfo('SomeBehavior::SecondBehaviorType')
+        .setDescription('The first behavior of the first sprite');
+
       parameters
         .addNewParameter('MySpriteObject2')
         .setType('objectList')
         .setExtraInfo('Sprite')
         .setDescription('The second object to be used, a sprite');
-      expect(parameters.getParametersCount()).toBe(6);
+      expect(parameters.getParametersCount()).toBe(8);
 
       parameters.removeParameter('MySpriteObject2');
-      expect(parameters.getParametersCount()).toBe(5);
+      expect(parameters.getParametersCount()).toBe(7);
 
       objectsContainer = new gd.ObjectsContainer();
       gd.ParameterMetadataTools.parametersToObjectsContainer(
@@ -4278,16 +4290,100 @@ describe('libGD.js', function () {
         objectsContainer
       );
 
+      // Check that object parameters are considered as objects in the objects container.
       expect(objectsContainer.getObjectsCount()).toBe(2);
       expect(objectsContainer.hasObjectNamed('MyObjectWithoutType')).toBe(true);
-      expect(objectsContainer.getObject('MyObjectWithoutType').getType()).toBe(
+      expect(objectsContainer.hasObjectNamed('MySpriteObject')).toBe(true);
+
+      const objectWithoutType = objectsContainer.getObject('MyObjectWithoutType');
+      expect(objectWithoutType.getType()).toBe(
         ''
       );
-      expect(objectsContainer.hasObjectNamed('MySpriteObject')).toBe(true);
+      const mySpriteObject = objectsContainer.getObject('MySpriteObject');
       expect(objectsContainer.getObject('MySpriteObject').getType()).toBe(
         'Sprite'
       );
 
+      // Check that behaviors were also added.
+      expect(objectsContainer.getObject('MySpriteObject').getAllBehaviorNames().toJSArray()).toEqual(
+        ['MyFirstBehavior', 'MySecondBehavior']
+      );
+
+      // Call a second time and verify no changes.
+      gd.ParameterMetadataTools.parametersToObjectsContainer(
+        project,
+        parameters,
+        objectsContainer
+      );
+      expect(objectsContainer.getObjectsCount()).toBe(2);
+      expect(objectsContainer.hasObjectNamed('MyObjectWithoutType')).toBe(true);
+      expect(objectsContainer.hasObjectNamed('MySpriteObject')).toBe(true);
+      expect(objectsContainer.getObject('MySpriteObject').getAllBehaviorNames().toJSArray()).toEqual(
+        ['MyFirstBehavior', 'MySecondBehavior']
+      );
+
+      // Verify that objects are even stable in memory.
+      expect(objectWithoutType).toBe(objectsContainer.getObject('MyObjectWithoutType'));
+      expect(gd.getPointer(objectWithoutType)).toBe(gd.getPointer(objectsContainer.getObject('MyObjectWithoutType')));
+      expect(mySpriteObject).toBe(objectsContainer.getObject('MySpriteObject'));
+      expect(gd.getPointer(mySpriteObject)).toBe(gd.getPointer(objectsContainer.getObject('MySpriteObject')));
+
+      // Change an object type, rename a behavior and add a new object.
+      parameters.getParameter("MyObjectWithoutType").setExtraInfo('Sprite');
+      parameters.getParameter("MySecondBehavior").setName("MyRenamedSecondBehavior");
+      parameters
+        .addNewParameter('MySpriteObject3')
+        .setType('objectList')
+        .setExtraInfo('Sprite')
+        .setDescription('The third object to be used, a sprite');
+
+      // Verify changes are propagated.
+      gd.ParameterMetadataTools.parametersToObjectsContainer(
+        project,
+        parameters,
+        objectsContainer
+      );
+      expect(objectsContainer.getObjectsCount()).toBe(3);
+      expect(objectsContainer.hasObjectNamed('MyObjectWithoutType')).toBe(true);
+      expect(objectsContainer.hasObjectNamed('MySpriteObject')).toBe(true);
+      expect(objectsContainer.hasObjectNamed('MySpriteObject3')).toBe(true);
+      expect(objectsContainer.getObject('MySpriteObject').getAllBehaviorNames().toJSArray()).toEqual(
+        ['MyFirstBehavior', 'MyRenamedSecondBehavior']
+      );
+      const updatedObjectWithoutType = objectsContainer.getObject('MyObjectWithoutType');
+      expect(updatedObjectWithoutType.getType()).toEqual('Sprite');
+      const mySpriteObject3 = objectsContainer.getObject('MySpriteObject3');
+      expect(mySpriteObject3.getType()).toEqual('Sprite');
+
+      // Verify that objects are changed in memory if changed in parameters.
+      expect(objectWithoutType).not.toBe(updatedObjectWithoutType);
+      expect(gd.getPointer(objectWithoutType)).not.toBe(gd.getPointer(updatedObjectWithoutType));
+      expect(mySpriteObject).toBe(objectsContainer.getObject('MySpriteObject'));
+      expect(gd.getPointer(mySpriteObject)).toBe(gd.getPointer(objectsContainer.getObject('MySpriteObject')));
+
+      // Remove an object and check that it is removed from the objects container.
+      parameters.removeParameter('MySpriteObject');
+      parameters.removeParameter('MyFirstBehavior');
+      parameters.removeParameter('MyRenamedSecondBehavior');
+
+      // Verify changes are propagated.
+      gd.ParameterMetadataTools.parametersToObjectsContainer(
+        project,
+        parameters,
+        objectsContainer
+      );
+      expect(objectsContainer.getObjectsCount()).toBe(2);
+      expect(objectsContainer.hasObjectNamed('MyObjectWithoutType')).toBe(true);
+      expect(objectsContainer.hasObjectNamed('MySpriteObject')).toBe(false);
+      expect(objectsContainer.hasObjectNamed('MySpriteObject3')).toBe(true);
+
+      // Verify that objects are still stable in memory.
+      expect(updatedObjectWithoutType).toBe(objectsContainer.getObject('MyObjectWithoutType'));
+      expect(gd.getPointer(updatedObjectWithoutType)).toBe(gd.getPointer(objectsContainer.getObject('MyObjectWithoutType')));
+      expect(mySpriteObject3).toBe(objectsContainer.getObject('MySpriteObject3'));
+      expect(gd.getPointer(mySpriteObject3)).toBe(gd.getPointer(objectsContainer.getObject('MySpriteObject3')));
+
+      objectsContainer.delete();
       eventsFunction.delete();
       project.delete();
     });

newIDE/app/src/EventsFunctionsExtensionEditor/EventsFunctionConfigurationEditor/index.js

@@ -152,6 +152,9 @@ export default class EventsFunctionConfigurationEditor extends React.Component<
       eventsFunctionsExtension,
     } = this.props;
 
+    const hasLegacyFunctionObjectGroups =
+      eventsFunction.getObjectGroups().count() > 0;
+
     return (
       <Column expand useFullHeight noOverflowParent>
         <Line>
@@ -168,11 +171,13 @@ export default class EventsFunctionConfigurationEditor extends React.Component<
                   value: ('parameters': TabNames),
                   label: <Trans>Parameters</Trans>,
                 },
-                {
-                  value: ('groups': TabNames),
-                  label: <Trans>Object groups</Trans>,
-                },
-              ]}
+                hasLegacyFunctionObjectGroups
+                  ? {
+                      value: ('groups': TabNames),
+                      label: <Trans>Object groups</Trans>,
+                    }
+                  : null,
+              ].filter(Boolean)}
             />
           </Column>
         </Line>