fix: Instantiate() behavior for Prefab and Entity references

sources/engine/Stride.Engine.Tests/TestEntity.cs

@@ -6,6 +6,7 @@
 using System.Linq;
 using Xunit;
 using Stride.Core;
+using Stride.Core.Annotations;
 using Stride.Engine.Design;
 using Stride.Rendering;
 
@@ -129,8 +130,6 @@ public void TestMultipleComponents()
         [Fact]
         public void TestEntityAndPrefabClone()
         {
-            Prefab prefab = null;
-
             var entity = new Entity("Parent");
             var childEntity = new Entity("Child");
             entity.AddChild(childEntity);
@@ -141,17 +140,25 @@ public void TestEntityAndPrefabClone()
 
             var newEntity = entity.Clone();
 
-            // NOTE: THE CODE AFTER THIS IS EXECUTED TWO TIMES
-            // 1st time: newEntity = entity.Clone();
-            // 2nd time: newEntity = prefab.Instantiate()[0];
-            check_new_Entity:
+            CheckEntity(newEntity);
+
+            // Check prefab cloning
+            var prefab = new Prefab();
+            prefab.Entities.Add(entity);
+            var newEntities = prefab.Instantiate();
+            Assert.Single(newEntities);
+
+            CheckEntity(newEntities[0]);
+            return;
+
+            void CheckEntity([NotNull] Entity e)
             {
-                Assert.Single(newEntity.Transform.Children);
-                var newChildEntity = newEntity.Transform.Children[0].Entity;
+                Assert.Single(e.Transform.Children);
+                var newChildEntity = e.Transform.Children[0].Entity;
                 Assert.Equal("Child", newChildEntity.Name);
 
-                Assert.NotNull(newEntity.Get<CustomEntityComponent>());
-                var newCustom = newEntity.Get<CustomEntityComponent>();
+                Assert.NotNull(e.Get<CustomEntityComponent>());
+                var newCustom = e.Get<CustomEntityComponent>();
 
                 // Make sure that the old component and the new component are different
                 Assert.NotEqual(custom, newCustom);
@@ -162,19 +169,37 @@ public void TestEntityAndPrefabClone()
                 // Verify that objects references outside the Entity/Component hierarchy are not cloned (shared)
                 Assert.Equal(custom.CustomObject, newCustom.CustomObject);
             }
+        }
 
-            // Woot, ugly test using a goto, avoid factorizing code in a delegate method, ugly but effective, goto FTW
-            if (prefab == null)
+        [Fact]
+        public void TestCloningBehavior()
+        {
+            var externalEntity = new Entity();
+            var sourceEntity = new Entity();
+            var sourceComponent = new EntityComponentWithPrefab
             {
-                // Check prefab cloning
-                prefab = new Prefab();
-                prefab.Entities.Add(entity);
-                var newEntities = prefab.Instantiate();
-                Assert.Single(newEntities);
-
-                newEntity = newEntities[0];
-                goto check_new_Entity;
-            }
+                Prefab = new Prefab(),
+                ExternalEntityRef = externalEntity,
+                // Not yet supported, see commented out ExternalComponentRef declaration further and PR #2914
+                //ExternalComponentRef = externalEntity.Transform
+            };
+            sourceComponent.Prefab.Entities.Add(sourceEntity);
+            sourceEntity.Add(sourceComponent);
+
+            var clonedComponent = sourceComponent.Prefab.Instantiate()[0].Get<EntityComponentWithPrefab>();
+
+            // Validate that cloning did clone the entity
+            Assert.NotEqual(clonedComponent.Entity, sourceComponent.Entity);
+
+            // References to prefabs should not be deep cloned as they are content types
+            Assert.Equal(clonedComponent.Prefab, sourceComponent.Prefab);
+
+            // References to entities outside this one's hierarchy should not clone the entity referenced, it should point to the same reference
+            Assert.Equal(clonedComponent.ExternalEntityRef, sourceComponent.ExternalEntityRef);
+
+            // References to entity component outside this one's hierarchy should not clone the component referenced, it should point to the same reference
+            // Not yet supported, see commented out ExternalComponentRef declaration further and PR #2914
+            /*Assert.Equal(clonedComponent.ExternalComponentRef, sourceComponent.ExternalComponentRef);*/
         }
 
         private class DelegateEntityComponentNotify : EntityManager
@@ -270,4 +295,20 @@ public abstract class CustomEntityComponentBase : EntityComponent
     {
         public Action<CustomEntityComponentEventArgs> Changed;
     }
+
+    [DataContract]
+    public class EntityComponentWithPrefab : EntityComponent
+    {
+        public required Prefab Prefab { get; set; }
+        public required Entity ExternalEntityRef { get; set; }
+
+        /* TODO:
+         * References to entity component outside of a prefab's hierarchy should not clone the component referenced, it should point to the same reference.
+         * More work is required on that front, particularly with EntityComponent which does not have a specific serializer,
+         * we would need one that derive from DataContentSerializerWithReuse to properly filter external references.
+         * The serializer for TransformComponent derives from a simple DataSerializer for example.
+         * See also PR #2914
+         */
+        /*public required TransformComponent ExternalComponentRef { get; set; }*/
+    }
 }

sources/engine/Stride.Engine/Engine/Design/CloneSerializer.cs

@@ -37,6 +37,9 @@ public override void PreSerialize(ref T obj, ArchiveMode mode, SerializationStre
                 }
                 else
                 {
+                    var dataSerializer = cloneContext.EntitySerializerSelector.GetSerializer<T>();
+                    if (dataSerializer != this)
+                        dataSerializer.PreSerialize(ref obj, mode, stream);
                     cloneContext.SerializedObjects.Add(obj);
                 }
             }
@@ -58,7 +61,9 @@ public override void PreSerialize(ref T obj, ArchiveMode mode, SerializationStre
                 }
                 else
                 {
-                    base.PreSerialize(ref obj, mode, stream);
+                    var dataSerializer = cloneContext.EntitySerializerSelector.GetSerializer<T>();
+                    if (dataSerializer != this)
+                        dataSerializer.PreSerialize(ref obj, mode, stream);
                     cloneContext.SerializedObjects.Add(obj);
                 }
             }
@@ -75,7 +80,8 @@ public override void Serialize(ref T obj, ArchiveMode mode, SerializationStream
 
                 // Serialize object
                 //stream.Context.Set(EntitySerializer.InsideEntityComponentProperty, false);
-                dataSerializer.Serialize(ref obj, mode, stream);
+                if (dataSerializer != this)
+                    dataSerializer.Serialize(ref obj, mode, stream);
 
                 if (obj is EntityComponent)
                 {

sources/engine/Stride.Engine/Engine/Design/EntityCloner.cs

@@ -29,10 +29,13 @@ namespace Stride.Engine.Design
     [DataSerializerGlobal(typeof(CloneSerializer<OfflineRasterizedSpriteFont>), Profile = "Clone")]
     [DataSerializerGlobal(typeof(CloneSerializer<RuntimeRasterizedSpriteFont>), Profile = "Clone")]
     [DataSerializerGlobal(typeof(CloneSerializer<SignedDistanceFieldSpriteFont>), Profile = "Clone")]
+    [DataSerializerGlobal(typeof(CloneSerializer<Prefab>), Profile = "Clone")]
+    [DataSerializerGlobal(typeof(CloneSerializer<Entity>), Profile = "Clone")]
     public class EntityCloner
     {
         private static readonly CloneContext cloneContext = new CloneContext();
         private static SerializerSelector cloneSerializerSelector = null;
+        private static SerializerSelector entitySerializerSelector = null;
         public static readonly PropertyKey<CloneContext> CloneContextProperty = new PropertyKey<CloneContext>("CloneContext", typeof(EntityCloner));
 
         // CloneObject TLS used to clone entities, so that we don't create one everytime we clone
@@ -45,13 +48,13 @@ private static HashSet<object> ClonedObjects()
         }
 
         /// <summary>
-        /// Clones the specified prefab.
-        /// <see cref="Entity"/>, children <see cref="Entity"/> and their <see cref="EntityComponent"/> will be cloned.
+        /// Instantiates the content of the prefab provided.
+        /// <see cref="Prefab.Entities"/>, children <see cref="Entity"/> and their <see cref="EntityComponent"/> will be cloned.
         /// Other assets will be shared.
         /// </summary>
-        /// <param name="prefab">The prefab to clone.</param>
-        /// <returns>A cloned prefab</returns>
-        public static Prefab Clone(Prefab prefab)
+        /// <param name="prefab">The prefab to instantiate.</param>
+        /// <returns>A clone of this prefab's <see cref="Prefab.Entities"/></returns>
+        public static List<Entity> Instantiate(Prefab prefab)
         {
             if (prefab == null) throw new ArgumentNullException(nameof(prefab));
             var clonedObjects = ClonedObjects();
@@ -61,7 +64,8 @@ public static Prefab Clone(Prefab prefab)
                 {
                     CollectEntityTreeHelper(entity, clonedObjects);
                 }
-                return Clone(clonedObjects, null, prefab);
+
+                return Clone(clonedObjects, null, prefab.Entities);
             }
             finally
             {
@@ -127,17 +131,16 @@ internal static void CollectEntityTreeHelper(Entity entity, HashSet<object> enti
         /// <returns>The cloned object.</returns>
         private static T Clone<T>(HashSet<object> clonedObjects, TryGetValueFunction<object, object> mappedObjects, T entity) where T : class
         {
-            if (cloneSerializerSelector == null)
-            {
-                cloneSerializerSelector = new SerializerSelector(true, false, "Default", "Clone");
-            }
+            cloneSerializerSelector ??= new SerializerSelector(true, false, "Default", "Clone");
+
+            entitySerializerSelector ??= new SerializerSelector(true, false, "Default");
 
             // Initialize CloneContext
             lock (cloneContext)
             {
                 try
                 {
-                    cloneContext.EntitySerializerSelector = cloneSerializerSelector;
+                    cloneContext.EntitySerializerSelector = entitySerializerSelector;
 
                     cloneContext.ClonedObjects = clonedObjects;
                     cloneContext.MappedObjects = mappedObjects;

sources/engine/Stride.Engine/Engine/Prefab.cs

@@ -29,8 +29,7 @@ public sealed class Prefab
         /// <returns>A collection of entities extracted from the prefab</returns>
         public List<Entity> Instantiate()
         {
-            var newPrefab = EntityCloner.Clone(this);
-            return newPrefab.Entities;
+            return EntityCloner.Instantiate(this);
         }
     }
 }