From 968073b04346fa8b4134f9e9cfc4fecb774a2ac1 Mon Sep 17 00:00:00 2001
From: Alejandro Perea <alexpro820@gmail.com>
Date: Wed, 26 Jan 2022 20:55:08 +0100
Subject: [PATCH] Contain all layer types in an enum (#131)

* Contain all layer types in an enum

* Implement `as_xlayer` for `LayerType`

* Make id optional

* Rename `ty` to `layer_type`

* Remove `layer_index`

* Move `as_x` functions to tests

* Fix build
---
 CHANGELOG.md   |   6 ++
 src/layers.rs  | 191 +++++++++++++++++++++++++++++++++----------------
 src/map.rs     |  24 ++-----
 src/objects.rs |  65 +----------------
 src/tile.rs    |   8 +--
 tests/lib.rs   |  65 +++++++++++------
 6 files changed, 190 insertions(+), 169 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cef98f9..5b57687 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - MIT license file.
 
 ### Changed
+- `Layer` has been renamed to `TileLayer`, and the original `Layer` structure is now used
+  for common data from all layer types.
+- `Map` now has a single `layers` member which contains layers of all types in order.
+- Layer members that are common between types (i.e. `id`, `name`, `visible`, `opacity`, `offset_x`,
+  `offset_y` and `properties`) have been moved into `Layer`.
+- `ObjectGroup` has been renamed to `ObjectLayer`.
 - `parse_file`, `parse` -> `Map::parse_file` with optional path.
 - `parse_with_path` -> `Map::parse_reader`.
 - `parse_tileset` -> `Tileset::parse`.
diff --git a/src/layers.rs b/src/layers.rs
index 2f801e2..faa5f60 100644
--- a/src/layers.rs
+++ b/src/layers.rs
@@ -5,7 +5,8 @@ use xml::{attribute::OwnedAttribute, EventReader};
 use crate::{
     error::TiledError,
     image::Image,
-    properties::{parse_properties, Properties},
+    objects::Object,
+    properties::{parse_properties, Color, Properties},
     util::*,
 };
 
@@ -42,33 +43,42 @@ impl LayerTile {
     }
 }
 
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Clone, PartialEq, Debug)]
+pub enum LayerType {
+    TileLayer(TileLayer),
+    ObjectLayer(ObjectLayer),
+    ImageLayer(ImageLayer),
+    // TODO: Support group layers
+}
+
+#[derive(Clone, Copy)]
+pub(crate) enum LayerTag {
+    TileLayer,
+    ObjectLayer,
+    ImageLayer,
+}
+
+#[derive(Clone, PartialEq, Debug)]
 pub struct Layer {
     pub name: String,
-    pub width: u32,
-    pub height: u32,
-    pub opacity: f32,
+    pub id: u32,
     pub visible: bool,
     pub offset_x: f32,
     pub offset_y: f32,
-    /// The tiles are arranged in rows. Each tile is a number which can be used
-    ///  to find which tileset it belongs to and can then be rendered.
-    pub tiles: LayerData,
+    pub opacity: f32,
     pub properties: Properties,
-    pub layer_index: u32,
-    /// The ID of the layer, as shown in the editor.
-    /// Layer ID stays the same even if layers are reordered or modified in the editor.
-    pub id: u32,
+    pub layer_type: LayerType,
 }
 
 impl Layer {
     pub(crate) fn new<R: Read>(
         parser: &mut EventReader<R>,
         attrs: Vec<OwnedAttribute>,
-        layer_index: u32,
+        tag: LayerTag,
         infinite: bool,
-    ) -> Result<Layer, TiledError> {
-        let ((o, v, ox, oy, n, id), (w, h)) = get_attrs!(
+        path_relative_to: Option<&Path>,
+    ) -> Result<Self, TiledError> {
+        let ((opacity, visible, offset_x, offset_y, name, id), ()) = get_attrs!(
             attrs,
             optionals: [
                 ("opacity", opacity, |v:String| v.parse().ok()),
@@ -78,6 +88,59 @@ impl Layer {
                 ("name", name, |v| Some(v)),
                 ("id", id, |v:String| v.parse().ok()),
             ],
+            required: [
+            ],
+
+            TiledError::MalformedAttributes("layer parsing error, no id attribute found".to_string())
+        );
+
+        let (ty, properties) = match tag {
+            LayerTag::TileLayer => {
+                let (ty, properties) = TileLayer::new(parser, attrs, infinite)?;
+                (LayerType::TileLayer(ty), properties)
+            }
+            LayerTag::ObjectLayer => {
+                let (ty, properties) = ObjectLayer::new(parser, attrs)?;
+                (LayerType::ObjectLayer(ty), properties)
+            }
+            LayerTag::ImageLayer => {
+                let (ty, properties) = ImageLayer::new(parser, path_relative_to)?;
+                (LayerType::ImageLayer(ty), properties)
+            }
+        };
+
+        Ok(Self {
+            visible: visible.unwrap_or(true),
+            offset_x: offset_x.unwrap_or(0.0),
+            offset_y: offset_y.unwrap_or(0.0),
+            opacity: opacity.unwrap_or(1.0),
+            name: name.unwrap_or_default(),
+            id: id.unwrap_or(0),
+            properties,
+            layer_type: ty,
+        })
+    }
+}
+
+#[derive(Debug, PartialEq, Clone)]
+pub struct TileLayer {
+    pub width: u32,
+    pub height: u32,
+    /// The tiles are arranged in rows. Each tile is a number which can be used
+    ///  to find which tileset it belongs to and can then be rendered.
+    pub tiles: LayerData,
+}
+
+impl TileLayer {
+    pub(crate) fn new<R: Read>(
+        parser: &mut EventReader<R>,
+        attrs: Vec<OwnedAttribute>,
+        infinite: bool,
+    ) -> Result<(TileLayer, Properties), TiledError> {
+        let ((), (w, h)) = get_attrs!(
+            attrs,
+            optionals: [
+            ],
             required: [
                 ("width", width, |v: String| v.parse().ok()),
                 ("height", height, |v: String| v.parse().ok()),
@@ -101,19 +164,14 @@ impl Layer {
             },
         });
 
-        Ok(Layer {
-            name: n.unwrap_or(String::new()),
-            width: w,
-            height: h,
-            opacity: o.unwrap_or(1.0),
-            visible: v.unwrap_or(true),
-            offset_x: ox.unwrap_or(0.0),
-            offset_y: oy.unwrap_or(0.0),
-            tiles: tiles,
-            properties: properties,
-            layer_index,
-            id: id.unwrap_or(0),
-        })
+        Ok((
+            TileLayer {
+                width: w,
+                height: h,
+                tiles: tiles,
+            },
+            properties,
+        ))
     }
 }
 
@@ -125,45 +183,56 @@ pub enum LayerData {
 
 #[derive(Debug, PartialEq, Clone)]
 pub struct ImageLayer {
-    pub name: String,
-    pub opacity: f32,
-    pub visible: bool,
-    pub offset_x: f32,
-    pub offset_y: f32,
     pub image: Option<Image>,
-    pub properties: Properties,
-    pub layer_index: u32,
-    /// The ID of the layer, as shown in the editor.
-    /// Layer ID stays the same even if layers are reordered or modified in the editor.
-    pub id: u32,
 }
 
 impl ImageLayer {
     pub(crate) fn new<R: Read>(
         parser: &mut EventReader<R>,
-        attrs: Vec<OwnedAttribute>,
-        layer_index: u32,
         path_relative_to: Option<&Path>,
-    ) -> Result<ImageLayer, TiledError> {
-        let ((o, v, ox, oy, n, id), ()) = get_attrs!(
+    ) -> Result<(ImageLayer, Properties), TiledError> {
+        let mut image: Option<Image> = None;
+        let mut properties = HashMap::new();
+
+        parse_tag!(parser, "imagelayer", {
+            "image" => |attrs| {
+                image = Some(Image::new(parser, attrs, path_relative_to.ok_or(TiledError::SourceRequired{object_to_parse: "Image".to_string()})?)?);
+                Ok(())
+            },
+            "properties" => |_| {
+                properties = parse_properties(parser)?;
+                Ok(())
+            },
+        });
+        Ok((ImageLayer { image }, properties))
+    }
+}
+
+#[derive(Debug, PartialEq, Clone)]
+pub struct ObjectLayer {
+    pub objects: Vec<Object>,
+    pub colour: Option<Color>,
+}
+
+impl ObjectLayer {
+    pub(crate) fn new<R: Read>(
+        parser: &mut EventReader<R>,
+        attrs: Vec<OwnedAttribute>,
+    ) -> Result<(ObjectLayer, Properties), TiledError> {
+        let (c, ()) = get_attrs!(
             attrs,
             optionals: [
-                ("opacity", opacity, |v:String| v.parse().ok()),
-                ("visible", visible, |v:String| v.parse().ok().map(|x:i32| x == 1)),
-                ("offsetx", offset_x, |v:String| v.parse().ok()),
-                ("offsety", offset_y, |v:String| v.parse().ok()),
-                ("name", name, |v| Some(v)),
-                ("id", id, |v:String| v.parse().ok()),
+                ("color", colour, |v:String| v.parse().ok()),
             ],
             required: [],
             // this error should never happen since there are no required attrs
-            TiledError::MalformedAttributes("image layer parsing error".to_string())
+            TiledError::MalformedAttributes("object group parsing error".to_string())
         );
+        let mut objects = Vec::new();
         let mut properties = HashMap::new();
-        let mut image: Option<Image> = None;
-        parse_tag!(parser, "imagelayer", {
-            "image" => |attrs| {
-                image = Some(Image::new(parser, attrs, path_relative_to.ok_or(TiledError::SourceRequired{object_to_parse: "Image".to_string()})?)?);
+        parse_tag!(parser, "objectgroup", {
+            "object" => |attrs| {
+                objects.push(Object::new(parser, attrs)?);
                 Ok(())
             },
             "properties" => |_| {
@@ -171,17 +240,13 @@ impl ImageLayer {
                 Ok(())
             },
         });
-        Ok(ImageLayer {
-            name: n.unwrap_or(String::new()),
-            opacity: o.unwrap_or(1.0),
-            visible: v.unwrap_or(true),
-            offset_x: ox.unwrap_or(0.0),
-            offset_y: oy.unwrap_or(0.0),
-            image,
+        Ok((
+            ObjectLayer {
+                objects: objects,
+                colour: c,
+            },
             properties,
-            layer_index,
-            id: id.unwrap_or(0),
-        })
+        ))
     }
 }
 
diff --git a/src/map.rs b/src/map.rs
index 52081e3..1cee014 100644
--- a/src/map.rs
+++ b/src/map.rs
@@ -4,8 +4,7 @@ use xml::{attribute::OwnedAttribute, reader::XmlEvent, EventReader};
 
 use crate::{
     error::{ParseTileError, TiledError},
-    layers::{ImageLayer, Layer},
-    objects::ObjectGroup,
+    layers::{Layer, LayerTag},
     properties::{parse_properties, Color, Properties},
     tileset::Tileset,
     util::{get_attrs, parse_tag},
@@ -27,10 +26,8 @@ pub struct Map {
     pub tile_height: u32,
     /// The tilesets present in this map.
     pub tilesets: Vec<Tileset>,
-    /// The tile layers present in this map.
+    /// The layers present in this map.
     pub layers: Vec<Layer>,
-    pub image_layers: Vec<ImageLayer>,
-    pub object_groups: Vec<ObjectGroup>,
     /// The custom properties of this map.
     pub properties: Properties,
     /// The background color of this map, if any.
@@ -98,27 +95,23 @@ impl Map {
             TiledError::MalformedAttributes("map must have a version, width and height with correct types".to_string())
         );
 
+        let infinite = infinite.unwrap_or(false);
         let source_path = map_path.and_then(|p| p.parent());
 
         let mut tilesets = Vec::new();
         let mut layers = Vec::new();
-        let mut image_layers = Vec::new();
         let mut properties = HashMap::new();
-        let mut object_groups = Vec::new();
-        let mut layer_index = 0;
         parse_tag!(parser, "map", {
             "tileset" => |attrs| {
                 tilesets.push(Tileset::parse_xml(parser, attrs, source_path)?);
                 Ok(())
             },
             "layer" => |attrs| {
-                layers.push(Layer::new(parser, attrs, layer_index, infinite.unwrap_or(false))?);
-                layer_index += 1;
+                layers.push(Layer::new(parser, attrs, LayerTag::TileLayer, infinite, source_path)?);
                 Ok(())
             },
             "imagelayer" => |attrs| {
-                image_layers.push(ImageLayer::new(parser, attrs, layer_index, source_path)?);
-                layer_index += 1;
+                layers.push(Layer::new(parser, attrs, LayerTag::ImageLayer, infinite, source_path)?);
                 Ok(())
             },
             "properties" => |_| {
@@ -126,8 +119,7 @@ impl Map {
                 Ok(())
             },
             "objectgroup" => |attrs| {
-                object_groups.push(ObjectGroup::new(parser, attrs, Some(layer_index))?);
-                layer_index += 1;
+                layers.push(Layer::new(parser, attrs, LayerTag::ObjectLayer, infinite, source_path)?);
                 Ok(())
             },
         });
@@ -140,11 +132,9 @@ impl Map {
             tile_height: th,
             tilesets,
             layers,
-            image_layers,
-            object_groups,
             properties,
             background_color: c,
-            infinite: infinite.unwrap_or(false),
+            infinite,
         })
     }
 
diff --git a/src/objects.rs b/src/objects.rs
index 9f4a993..6569f33 100644
--- a/src/objects.rs
+++ b/src/objects.rs
@@ -4,71 +4,10 @@ use xml::{attribute::OwnedAttribute, EventReader};
 
 use crate::{
     error::TiledError,
-    properties::{parse_properties, Color, Properties},
+    properties::{parse_properties, Properties},
     util::{get_attrs, parse_tag},
 };
 
-#[derive(Debug, PartialEq, Clone)]
-pub struct ObjectGroup {
-    pub name: String,
-    pub opacity: f32,
-    pub visible: bool,
-    pub objects: Vec<Object>,
-    pub colour: Option<Color>,
-    /**
-     * Layer index is not preset for tile collision boxes
-     */
-    pub layer_index: Option<u32>,
-    pub properties: Properties,
-    /// The ID of the layer, as shown in the editor.
-    /// Layer ID stays the same even if layers are reordered or modified in the editor.
-    pub id: u32,
-}
-
-impl ObjectGroup {
-    pub(crate) fn new<R: Read>(
-        parser: &mut EventReader<R>,
-        attrs: Vec<OwnedAttribute>,
-        layer_index: Option<u32>,
-    ) -> Result<ObjectGroup, TiledError> {
-        let ((o, v, c, n, id), ()) = get_attrs!(
-            attrs,
-            optionals: [
-                ("opacity", opacity, |v:String| v.parse().ok()),
-                ("visible", visible, |v:String| v.parse().ok().map(|x:i32| x == 1)),
-                ("color", colour, |v:String| v.parse().ok()),
-                ("name", name, |v:String| Some(v)),
-                ("id", id, |v:String| v.parse().ok()),
-            ],
-            required: [],
-            // this error should never happen since there are no required attrs
-            TiledError::MalformedAttributes("object group parsing error".to_string())
-        );
-        let mut objects = Vec::new();
-        let mut properties = HashMap::new();
-        parse_tag!(parser, "objectgroup", {
-            "object" => |attrs| {
-                objects.push(Object::new(parser, attrs)?);
-                Ok(())
-            },
-            "properties" => |_| {
-                properties = parse_properties(parser)?;
-                Ok(())
-            },
-        });
-        Ok(ObjectGroup {
-            name: n.unwrap_or(String::new()),
-            opacity: o.unwrap_or(1.0),
-            visible: v.unwrap_or(true),
-            objects: objects,
-            colour: c,
-            layer_index,
-            properties,
-            id: id.unwrap_or(0),
-        })
-    }
-}
-
 #[derive(Debug, PartialEq, Clone)]
 pub enum ObjectShape {
     Rect { width: f32, height: f32 },
@@ -95,7 +34,7 @@ pub struct Object {
 }
 
 impl Object {
-    fn new<R: Read>(
+    pub(crate) fn new<R: Read>(
         parser: &mut EventReader<R>,
         attrs: Vec<OwnedAttribute>,
     ) -> Result<Object, TiledError> {
diff --git a/src/tile.rs b/src/tile.rs
index 4d14d2f..a6d623b 100644
--- a/src/tile.rs
+++ b/src/tile.rs
@@ -6,7 +6,7 @@ use crate::{
     animation::Frame,
     error::TiledError,
     image::Image,
-    objects::ObjectGroup,
+    layers::ObjectLayer,
     properties::{parse_properties, Properties},
     util::{get_attrs, parse_animation, parse_tag},
 };
@@ -16,7 +16,7 @@ pub struct Tile {
     pub id: u32,
     pub image: Option<Image>,
     pub properties: Properties,
-    pub objectgroup: Option<ObjectGroup>,
+    pub collision: Option<ObjectLayer>,
     pub animation: Option<Vec<Frame>>,
     pub tile_type: Option<String>,
     pub probability: f32,
@@ -54,7 +54,7 @@ impl Tile {
                 Ok(())
             },
             "objectgroup" => |attrs| {
-                objectgroup = Some(ObjectGroup::new(parser, attrs, None)?);
+                objectgroup = Some(ObjectLayer::new(parser, attrs)?.0);
                 Ok(())
             },
             "animation" => |_| {
@@ -66,7 +66,7 @@ impl Tile {
             id,
             image,
             properties,
-            objectgroup,
+            collision: objectgroup,
             animation,
             tile_type,
             probability: probability.unwrap_or(1.0),
diff --git a/tests/lib.rs b/tests/lib.rs
index 411996a..46bb347 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -1,8 +1,21 @@
 use std::path::Path;
 use std::{fs::File, path::PathBuf};
-use tiled::{
-    TiledError, LayerData, Map, PropertyValue, Tileset,
-};
+use tiled::{LayerData, Map, PropertyValue, TiledError, Tileset};
+use tiled::{LayerType, ObjectLayer, TileLayer};
+
+fn as_tile_layer(layer: &LayerType) -> &TileLayer {
+    match layer {
+        LayerType::TileLayer(x) => x,
+        _ => panic!("Not a tile layer"),
+    }
+}
+
+fn as_object_layer(layer: &LayerType) -> &ObjectLayer {
+    match layer {
+        LayerType::ObjectLayer(x) => x,
+        _ => panic!("Not an object layer"),
+    }
+}
 
 fn parse_map_without_source(p: impl AsRef<Path>) -> Result<Map, TiledError> {
     let file = File::open(p).unwrap();
@@ -21,7 +34,7 @@ fn test_gzip_and_zlib_encoded_and_raw_are_the_same() {
     assert_eq!(z, c);
     assert_eq!(z, zstd);
 
-    if let LayerData::Finite(tiles) = &c.layers[0].tiles {
+    if let LayerData::Finite(tiles) = &as_tile_layer(&c.layers[0].layer_type).tiles {
         assert_eq!(tiles.len(), 100 * 100);
         assert_eq!(tiles[0].gid, 35);
         assert_eq!(tiles[100].gid, 17);
@@ -29,7 +42,7 @@ fn test_gzip_and_zlib_encoded_and_raw_are_the_same() {
         assert_eq!(tiles[200 + 1].gid, 17);
         assert!(tiles[9900..9999].iter().map(|t| t.gid).all(|g| g == 0));
     } else {
-        assert!(false, "It is wrongly recognised as an infinite map");
+        panic!("It is wrongly recognised as an infinite map");
     }
 }
 
@@ -66,7 +79,7 @@ fn test_just_tileset() {
 fn test_infinite_tileset() {
     let r = Map::parse_file("assets/tiled_base64_zlib_infinite.tmx").unwrap();
 
-    if let LayerData::Infinite(chunks) = &r.layers[0].tiles {
+    if let LayerData::Infinite(chunks) = &as_tile_layer(&r.layers[0].layer_type).tiles {
         assert_eq!(chunks.len(), 4);
 
         assert_eq!(chunks[&(0, 0)].width, 32);
@@ -82,23 +95,31 @@ fn test_infinite_tileset() {
 #[test]
 fn test_image_layers() {
     let r = Map::parse_file("assets/tiled_image_layers.tmx").unwrap();
-    assert_eq!(r.image_layers.len(), 2);
+    assert_eq!(r.layers.len(), 2);
+    let mut image_layers = r.layers.iter().map(|x| {
+        if let LayerType::ImageLayer(img) = &x.layer_type {
+            (img, x)
+        } else {
+            panic!("Found layer that isn't an image layer")
+        }
+    });
     {
-        let first = &r.image_layers[0];
-        assert_eq!(first.name, "Image Layer 1");
+        let first = image_layers.next().unwrap();
+        assert_eq!(first.1.name, "Image Layer 1");
         assert!(
-            first.image.is_none(),
+            first.0.image.is_none(),
             "{}'s image should be None",
-            first.name
+            first.1.name
         );
     }
     {
-        let second = &r.image_layers[1];
-        assert_eq!(second.name, "Image Layer 2");
+        let second = image_layers.next().unwrap();
+        assert_eq!(second.1.name, "Image Layer 2");
         let image = second
+            .0
             .image
             .as_ref()
-            .expect(&format!("{}'s image shouldn't be None", second.name));
+            .expect(&format!("{}'s image shouldn't be None", second.1.name));
         assert_eq!(image.source, PathBuf::from("assets/tilesheet.png"));
         assert_eq!(image.width, 448);
         assert_eq!(image.height, 192);
@@ -133,9 +154,8 @@ fn test_layer_property() {
 #[test]
 fn test_object_group_property() {
     let r = Map::parse_file("assets/tiled_object_groups.tmx").unwrap();
-    let prop_value: bool = if let Some(&PropertyValue::BoolValue(ref v)) = r.object_groups[0]
-        .properties
-        .get("an object group property")
+    let prop_value: bool = if let Some(&PropertyValue::BoolValue(ref v)) =
+        r.layers[1].properties.get("an object group property")
     {
         *v
     } else {
@@ -160,7 +180,7 @@ fn test_tileset_property() {
 fn test_flipped_gid() {
     let r = Map::parse_file("assets/tiled_flipped.tmx").unwrap();
 
-    if let LayerData::Finite(tiles) = &r.layers[0].tiles {
+    if let LayerData::Finite(tiles) = &as_tile_layer(&r.layers[0].layer_type).tiles {
         let t1 = tiles[0];
         let t2 = tiles[1];
         let t3 = tiles[2];
@@ -188,7 +208,7 @@ fn test_flipped_gid() {
 #[test]
 fn test_ldk_export() {
     let r = Map::parse_file("assets/ldk_tiled_export.tmx").unwrap();
-    if let LayerData::Finite(tiles) = &r.layers[0].tiles {
+    if let LayerData::Finite(tiles) = &as_tile_layer(&r.layers[0].layer_type).tiles {
         assert_eq!(tiles.len(), 8 * 8);
         assert_eq!(tiles[0].gid, 0);
         assert_eq!(tiles[8].gid, 1);
@@ -200,9 +220,10 @@ fn test_ldk_export() {
 #[test]
 fn test_object_property() {
     let r = parse_map_without_source(&Path::new("assets/tiled_object_property.tmx")).unwrap();
-    let prop_value = if let Some(PropertyValue::ObjectValue(v)) = r.object_groups[0].objects[0]
-        .properties
-        .get("object property")
+    let prop_value = if let Some(PropertyValue::ObjectValue(v)) =
+        as_object_layer(&r.layers[1].layer_type).objects[0]
+            .properties
+            .get("object property")
     {
         *v
     } else {
-- 
GitLab