Merge pull request #92 from dcronqvist/duplicate-id-bug

Fix issue where templated objects would get incorrect ID's

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/map-duplicate-object-id-bug.cs

@@ -0,0 +1,131 @@
+namespace DotTiled.Tests;
+
+public partial class TestData
+{
+  public static Map MapDuplicateObjectIdBug(string ext) => new Map
+  {
+    Class = "",
+    Orientation = MapOrientation.Orthogonal,
+    Width = 64,
+    Height = 64,
+    TileWidth = 16,
+    TileHeight = 16,
+    Infinite = true,
+    ParallaxOriginX = 0,
+    ParallaxOriginY = 0,
+    RenderOrder = RenderOrder.RightDown,
+    CompressionLevel = -1,
+    BackgroundColor = new TiledColor { R = 0, G = 0, B = 0, A = 0 },
+    Version = "1.10",
+    TiledVersion = "1.11.2",
+    NextLayerID = 2,
+    NextObjectID = 3,
+    Tilesets = [
+      new Tileset
+      {
+        FirstGID = 1,
+        Source = ext == "tmx" ? "tiles.tsx" : "tiles.tsj",
+        Version = "1.10",
+        TiledVersion = "1.11.2",
+        Name = "Tiles",
+        TileWidth = 16,
+        TileHeight = 16,
+        TileCount = 4,
+        Columns = 2,
+        Grid = new Grid
+        {
+          Orientation = GridOrientation.Orthogonal,
+          Width = 32,
+          Height = 32
+        },
+        Image = new Image
+        {
+          Source = "tiles.png",
+          Width = 32,
+          Height = 32,
+          Format = ImageFormat.Png
+        }
+      }
+    ],
+    Layers = [
+      new TileLayer
+      {
+        ID = 1,
+        Name = "Tile Layer 1",
+        Width = ext == "tmx" ? 64 : 16,
+        Height = ext == "tmx" ? 64 : 16,
+        Data = new Data
+        {
+          Encoding = DataEncoding.Csv,
+          Chunks = new Optional<Chunk[]>([
+            new Chunk
+            {
+              X = 0,
+              Y = 0,
+              Width = 16,
+              Height = 16,
+              GlobalTileIDs = [
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,2,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+                1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1
+              ],
+              FlippingFlags = GetAllNoneFlippingFlags(16 * 16)
+            }
+          ])
+        }
+      },
+      new ObjectLayer
+      {
+        ID = 3,
+        Name = "Object Layer 1",
+        Objects = [
+          new TileObject
+          {
+            ID = 1,
+            Template = ext == "tmx" ? "template.tx" : "template.tj",
+            X = 80,
+            Y = 144,
+
+            GID = 4,
+            Width = 16,
+            Height = 16,
+          },
+          new TileObject
+          {
+            ID = 2,
+            Template = ext == "tmx" ? "template.tx" : "template.tj",
+            X = 48,
+            Y = 144,
+
+            GID = 4,
+            Width = 16,
+            Height = 16,
+          }
+        ]
+      }
+    ]
+  };
+
+  private static FlippingFlags[] GetAllNoneFlippingFlags(int count)
+  {
+    var flippingFlags = new FlippingFlags[count];
+    for (int i = 0; i < count; i++)
+    {
+      flippingFlags[i] = FlippingFlags.None;
+    }
+    return flippingFlags;
+  }
+}

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/map-duplicate-object-id-bug.tmj

@@ -0,0 +1,79 @@
+{ "compressionlevel":-1,
+ "height":64,
+ "infinite":true,
+ "layers":[
+        {
+         "chunks":[
+                {
+                 "data":[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
+                    1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1],
+                 "height":16,
+                 "width":16,
+                 "x":0,
+                 "y":0
+                }],
+         "height":16,
+         "id":1,
+         "name":"Tile Layer 1",
+         "opacity":1,
+         "startx":0,
+         "starty":0,
+         "type":"tilelayer",
+         "visible":true,
+         "width":16,
+         "x":0,
+         "y":0
+        }, 
+        {
+         "draworder":"topdown",
+         "id":3,
+         "name":"Object Layer 1",
+         "objects":[
+                {
+                 "id":1,
+                 "template":"template.tj",
+                 "x":80,
+                 "y":144
+                },
+                {
+                 "id":2,
+                 "template":"template.tj",
+                 "x":48,
+                 "y":144
+                }],
+         "opacity":1,
+         "type":"objectgroup",
+         "visible":true,
+         "x":0,
+         "y":0
+        }],
+ "nextlayerid":2,
+ "nextobjectid":3,
+ "orientation":"orthogonal",
+ "renderorder":"right-down",
+ "tiledversion":"1.11.2",
+ "tileheight":16,
+ "tilesets":[
+        {
+         "firstgid":1,
+         "source":"tiles.tsj"
+        }],
+ "tilewidth":16,
+ "type":"map",
+ "version":"1.10",
+ "width":64
+}

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/map-duplicate-object-id-bug.tmx

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<map version="1.10" tiledversion="1.11.2" orientation="orthogonal" renderorder="right-down" width="64" height="64" tilewidth="16" tileheight="16" infinite="1" nextlayerid="2" nextobjectid="3">
+ <tileset firstgid="1" source="tiles.tsx"/>
+ <layer id="1" name="Tile Layer 1" width="64" height="64">
+  <data encoding="csv">
+   <chunk x="0" y="0" width="16" height="16">
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,2,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1
+</chunk>
+  </data>
+ </layer>
+ <objectgroup id="3" name="Object Layer 1">
+  <object id="1" template="template.tx" x="80" y="144"/>
+  <object id="2" template="template.tx" x="48" y="144"/>
+ </objectgroup>
+</map>

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/template.tj

@@ -0,0 +1,18 @@
+{ "object":
+    {
+     "gid":4,
+     "height":16,
+     "id":2,
+     "name":"",
+     "rotation":0,
+     "type":"",
+     "visible":true,
+     "width":16
+    },
+ "tileset":
+    {
+     "firstgid":1,
+     "source":"tiles.tsj"
+    },
+ "type":"template"
+}

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/template.tx

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<template>
+ <tileset firstgid="1" source="tiles.tsx"/>
+ <object gid="4" width="16" height="16">
+ </object>
+</template>

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/tiles.tsj

@@ -0,0 +1,20 @@
+{ "columns":2,
+ "grid":
+    {
+     "height":32,
+     "orientation":"orthogonal",
+     "width":32
+    },
+ "image":"tiles.png",
+ "imageheight":32,
+ "imagewidth":32,
+ "margin":0,
+ "name":"Tiles",
+ "spacing":0,
+ "tilecount":4,
+ "tiledversion":"1.11.2",
+ "tileheight":16,
+ "tilewidth":16,
+ "type":"tileset",
+ "version":"1.10"
+}

src/DotTiled.Tests/TestData/Maps/map-duplicate-object-id-bug/tiles.tsx

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<tileset version="1.10" tiledversion="1.11.2" name="Tiles" tilewidth="16" tileheight="16" tilecount="4" columns="2">
+ <grid orientation="orthogonal" width="32" height="32"/>
+ <image source="tiles.png" width="32" height="32"/>
+</tileset>

src/DotTiled.Tests/UnitTests/Serialization/LoaderTests.cs

@@ -315,4 +315,39 @@ public class LoaderTests
     // Assert
     DotTiledAssert.AssertProperties(customClassDefinition.Members, result.Properties);
   }
+
+  public static IEnumerable<object[]> Maps => TestData.MapTests;
+
+  [Theory]
+  [MemberData(nameof(Maps))]
+  public void LoadMap_ValidFilesExternalTilesetsAndTemplatesWithCache_ReturnsMapThatEqualsExpected(
+    string testDataFile,
+    Func<string, Map> expectedMap,
+    IReadOnlyCollection<ICustomTypeDefinition> customTypeDefinitions)
+  {
+    // Arrange
+    string[] fileFormats = [".tmx", ".tmj"];
+
+    foreach (var fileFormat in fileFormats)
+    {
+      var testDataFileWithFormat = testDataFile + fileFormat;
+      var resourceReader = Substitute.For<IResourceReader>();
+      resourceReader.Read(Arg.Any<string>()).Returns(callInfo =>
+      {
+        var filePath = callInfo.Arg<string>();
+        return TestData.GetRawStringFor(filePath);
+      });
+
+      var loader = Loader.DefaultWith(
+        resourceReader: resourceReader,
+        customTypeDefinitions: customTypeDefinitions);
+
+      // Act
+      var map = loader.LoadMap(testDataFileWithFormat);
+
+      // Assert
+      Assert.NotNull(map);
+      DotTiledAssert.AssertMap(expectedMap(fileFormat[1..]), map);
+    }
+  }
 }

src/DotTiled.Tests/UnitTests/Serialization/TestData.cs

@@ -34,6 +34,7 @@ public static partial class TestData
   public static IEnumerable<object[]> MapTests =>
   [
     [GetMapPath("default-map"), (string f) => DefaultMap(), Array.Empty<ICustomTypeDefinition>()],
+    [GetMapPath("map-duplicate-object-id-bug"), (string f) => MapDuplicateObjectIdBug(f), Array.Empty<ICustomTypeDefinition>()],
     [GetMapPath("map-with-common-props"), (string f) => MapWithCommonProps(), Array.Empty<ICustomTypeDefinition>()],
     [GetMapPath("map-with-custom-type-props"), (string f) => MapWithCustomTypeProps(), MapWithCustomTypePropsCustomTypeDefinitions()],
     [GetMapPath("map-with-custom-type-props-without-defs"), (string f) => MapWithCustomTypePropsWithoutDefs(), Array.Empty<ICustomTypeDefinition>()],

src/DotTiled/Layers/Objects/EllipseObject.cs

@@ -1,7 +1,25 @@
+using System.Linq;
+
 namespace DotTiled;
 
 /// <summary>
 /// An ellipse object in a map. The existing <see cref="Object.X"/>, <see cref="Object.Y"/>, <see cref="Object.Width"/>,
 /// and <see cref="Object.Height"/> properties are used to determine the size of the ellipse.
 /// </summary>
-public class EllipseObject : Object { }
+public class EllipseObject : Object
+{
+  internal override Object Clone() => new EllipseObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+  };
+}

src/DotTiled/Layers/Objects/Object.cs

@@ -64,4 +64,10 @@ public abstract class Object : HasPropertiesBase
 
   /// <inheritdoc/>
   public override IList<IProperty> GetProperties() => Properties;
+
+  /// <summary>
+  /// Creates a deep copy of the object.
+  /// </summary>
+  /// <returns></returns>
+  internal abstract Object Clone();
 }

src/DotTiled/Layers/Objects/PointObject.cs

@@ -1,7 +1,25 @@
+using System.Linq;
+
 namespace DotTiled;
 
 /// <summary>
 /// A point object in a map. The existing <see cref="Object.X"/> and <see cref="Object.Y"/> properties are used to
 /// determine the position of the point.
 /// </summary>
-public class PointObject : Object { }
+public class PointObject : Object
+{
+  internal override Object Clone() => new PointObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+  };
+}

src/DotTiled/Layers/Objects/PolygonObject.cs

@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.Linq;
 using System.Numerics;
 
 namespace DotTiled;
@@ -14,4 +15,20 @@ public class PolygonObject : Object
   /// <see cref="Object.X"/> and <see cref="Object.Y"/> are used as the origin of the polygon.
   /// </summary>
   public required List<Vector2> Points { get; set; }
+
+  internal override Object Clone() => new PolygonObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+    Points = Points.ToList(),
+  };
 }

src/DotTiled/Layers/Objects/PolylineObject.cs

@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.Linq;
 using System.Numerics;
 
 namespace DotTiled;
@@ -13,4 +14,20 @@ public class PolylineObject : Object
   /// The points that make up the polyline. <see cref="Object.X"/> and <see cref="Object.Y"/> are used as the origin of the polyline.
   /// </summary>
   public required List<Vector2> Points { get; set; }
+
+  internal override Object Clone() => new PolylineObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+    Points = Points.ToList(),
+  };
 }

src/DotTiled/Layers/Objects/RectangleObject.cs

@@ -1,7 +1,25 @@
+using System.Linq;
+
 namespace DotTiled;
 
 /// <summary>
 /// A rectangle object in a map. The existing <see cref="Object.X"/>, <see cref="Object.Y"/>, <see cref="Object.Width"/>,
 /// and <see cref="Object.Height"/> properties are used to determine the size of the rectangle.
 /// </summary>
-public class RectangleObject : Object { }
+public class RectangleObject : Object
+{
+  internal override Object Clone() => new RectangleObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+  };
+}

src/DotTiled/Layers/Objects/TextObject.cs

@@ -1,4 +1,5 @@
 using System.Globalization;
+using System.Linq;
 
 namespace DotTiled;
 
@@ -113,4 +114,32 @@ public class TextObject : Object
   /// The text to be displayed.
   /// </summary>
   public string Text { get; set; } = "";
+
+  internal override Object Clone() => new TextObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+
+    FontFamily = FontFamily,
+    PixelSize = PixelSize,
+    Wrap = Wrap,
+    Color = Color,
+    Bold = Bold,
+    Italic = Italic,
+    Underline = Underline,
+    Strikeout = Strikeout,
+    Kerning = Kerning,
+    HorizontalAlignment = HorizontalAlignment,
+    VerticalAlignment = VerticalAlignment,
+    Text = Text,
+  };
 }

src/DotTiled/Layers/Objects/TileObject.cs

@@ -1,3 +1,5 @@
+using System.Linq;
+
 namespace DotTiled;
 
 /// <summary>
@@ -14,4 +16,21 @@ public class TileObject : Object
   /// The flipping flags for the tile.
   /// </summary>
   public FlippingFlags FlippingFlags { get; set; }
+
+  internal override Object Clone() => new TileObject
+  {
+    ID = ID,
+    Name = Name,
+    Type = Type,
+    X = X,
+    Y = Y,
+    Width = Width,
+    Height = Height,
+    Rotation = Rotation,
+    Visible = Visible,
+    Template = Template,
+    Properties = Properties.Select(p => p.Clone()).ToList(),
+    GID = GID,
+    FlippingFlags = FlippingFlags,
+  };
 }

src/DotTiled/Serialization/Tmj/TmjReaderBase.Data.cs

@@ -19,8 +19,7 @@ public abstract partial class TmjReaderBase
 
   internal static Chunk ReadChunk(JsonElement element, Optional<DataCompression> compression, DataEncoding encoding)
   {
-    var data = ReadDataWithoutChunks(element, compression, encoding);
+    var data = element.GetRequiredPropertyCustom<Data>("data", e => ReadDataWithoutChunks(e, compression, encoding));
-
     var x = element.GetRequiredProperty<int>("x");
     var y = element.GetRequiredProperty<int>("y");
     var width = element.GetRequiredProperty<int>("width");

src/DotTiled/Serialization/Tmj/TmjReaderBase.ObjectLayer.cs

@@ -76,12 +76,13 @@ public abstract partial class TmjReaderBase
     List<Vector2> polygonDefault = null;
     List<Vector2> polylineDefault = null;
     List<IProperty> propertiesDefault = [];
+    Optional<uint> gidDefault = Optional.Empty;
 
     var template = element.GetOptionalProperty<string>("template");
     if (template.HasValue)
     {
       var resolvedTemplate = _externalTemplateResolver(template.Value);
-      var templObj = resolvedTemplate.Object;
+      var templObj = resolvedTemplate.Object.Clone();
 
       idDefault = templObj.ID;
       nameDefault = templObj.Name;
@@ -97,10 +98,11 @@ public abstract partial class TmjReaderBase
       pointDefault = templObj is PointObject;
       polygonDefault = (templObj is PolygonObject polygonObj) ? polygonObj.Points : null;
       polylineDefault = (templObj is PolylineObject polylineObj) ? polylineObj.Points : null;
+      gidDefault = (templObj is TileObject tileObj) ? tileObj.GID : Optional.Empty;
     }
 
     var ellipse = element.GetOptionalProperty<bool>("ellipse").GetValueOr(ellipseDefault);
-    var gid = element.GetOptionalProperty<uint>("gid");
+    var gid = element.GetOptionalProperty<uint>("gid").GetValueOrOptional(gidDefault);
     var height = element.GetOptionalProperty<float>("height").GetValueOr(heightDefault);
     var id = element.GetOptionalProperty<uint>("id").GetValueOrOptional(idDefault);
     var name = element.GetOptionalProperty<string>("name").GetValueOr(nameDefault);

src/DotTiled/Serialization/Tmx/TmxReaderBase.ObjectLayer.cs

@@ -74,7 +74,7 @@ public abstract partial class TmxReaderBase
     var template = _reader.GetOptionalAttribute("template");
     DotTiled.Object obj = null;
     if (template.HasValue)
-      obj = _externalTemplateResolver(template.Value).Object;
+      obj = _externalTemplateResolver(template.Value).Object.Clone();
 
     uint idDefault = obj?.ID.GetValueOr(0) ?? 0;
     string nameDefault = obj?.Name ?? "";