First pass increasing test coverage (#209)

* point tests

* Mesh Tests and removal of unused functions

* Vector tests

* more tests

.editorconfig

@@ -254,8 +254,7 @@ dotnet_diagnostic.ca1506.severity = warning # Avoid excessive class coupling
 dotnet_diagnostic.ca1507.severity = warning # Use nameof in place of string
 dotnet_diagnostic.ca1508.severity = warning # Avoid dead conditional code
 dotnet_diagnostic.ca1509.severity = warning # Invalid entry in code metrics configuration file
-dotnet_diagnostic.ca1861.severity = none # Prefer 'static readonly' fields over constant array arguments if the called method is called repeatedly and is not mutating the passed array (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1861)
-
+dotnet_diagnostic.ca1861.severity = suggestion # Prefer 'static readonly' fields over constant array arguments if the called method is called repeatedly and is not mutating the passed array (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1861)
 
 
 # Performance rules

Speckle.Sdk.sln.DotSettings

@@ -1,2 +1,3 @@
 <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
-	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=QL/@EntryIndexedValue">QL</s:String></wpf:ResourceDictionary>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=QL/@EntryIndexedValue">QL</s:String>
+	<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/Abbreviations/=XYZ/@EntryIndexedValue">XYZ</s:String></wpf:ResourceDictionary>

src/Speckle.Objects/Geometry/Mesh.cs

@@ -1,24 +1,46 @@
+using System.Diagnostics.Contracts;
 using Speckle.Newtonsoft.Json;
 using Speckle.Objects.Other;
+using Speckle.Objects.Utils;
 using Speckle.Sdk;
 using Speckle.Sdk.Common;
 using Speckle.Sdk.Models;
 
 namespace Speckle.Objects.Geometry;
 
+/// <remarks><a href="https://speckle.notion.site/Objects-Geometry-Mesh-9b0bf5ab92bf42f58bf2fe3922d2efca">More docs on notion</a></remarks>
 [SpeckleType("Objects.Geometry.Mesh")]
 public class Mesh : Base, IHasBoundingBox, IHasVolume, IHasArea, ITransformable<Mesh>
 {
+  /// <summary>
+  /// Flat list of vertex data (flat <c>x,y,z,x,y,z...</c> list)
+  /// </summary>
   [DetachProperty, Chunkable(31250)]
   public required List<double> vertices { get; set; }
 
+  /// <summary>
+  /// Flat list of face data<br/>
+  /// Each face starts with the length of the face (e.g. 3 in the case of triangles), followed by that many indices
+  /// </summary>
+  /// <remarks>
+  /// N-gons are supported, but large values of n (> ~50) tend to cause significant performance problems for consumers (e.g. HostApps and <see cref="MeshTriangulationHelper"/>.
+  /// </remarks>
+  /// <example>
+  /// <code>[
+  ///   3, 0, 1, 2, //first face, a triangle (3-gon)
+  ///   4, 1, 2, 3, 4, //second face, a quad (4-gon)
+  ///   6, 4, 5, 6, 7, 8, 9, //third face, an n-gon (6-gon)
+  /// ];</code></example>
   [DetachProperty, Chunkable(62500)]
   public required List<int> faces { get; set; }
 
-  /// <summary> Vertex colors as ARGB <see cref="int"/>s</summary>
+  /// <summary>Vertex colors as ARGB <see cref="int"/>s</summary>
+  /// <remarks>Expected that there are either 1 color per vertex, or an empty <see cref="List{T}"/></remarks>
   [DetachProperty, Chunkable(62500)]
   public List<int> colors { get; set; } = new();
 
+  /// <summary>Flat list of texture coordinates (flat <c>u,v,u,v,u,v...</c> list)</summary>
+  /// <remarks>Expected that there are either 1 texture coordinate per vertex, or an empty <see cref="List{T}"/></remarks>
   [DetachProperty, Chunkable(31250)]
   public List<double> textureCoordinates { get; set; } = new();
 
@@ -85,8 +107,6 @@ public bool TransformTo(Transform transform, out ITransformable transformed)
     return res;
   }
 
-  #region Convenience Methods
-
   [JsonIgnore]
   public int VerticesCount => vertices.Count / 3;
 
@@ -98,6 +118,8 @@ public bool TransformTo(Transform transform, out ITransformable transformed)
   /// </summary>
   /// <param name="index">The index of the vertex</param>
   /// <returns>Vertex as a <see cref="Point"/></returns>
+  /// <remarks>It is usually recommended to instead consume the <see cref="vertices"/> list manually for better performance</remarks>
+  [Pure]
   public Point GetPoint(int index)
   {
     index *= 3;
@@ -106,6 +128,8 @@ public Point GetPoint(int index)
 
   /// <returns><see cref="vertices"/> as list of <see cref="Point"/>s</returns>
   /// <exception cref="SpeckleException">when list is malformed</exception>
+  /// <remarks>It is usually recommended to instead consume the <see cref="vertices"/> list manually for better performance</remarks>
+  [Pure]
   public List<Point> GetPoints()
   {
     if (vertices.Count % 3 != 0)
@@ -129,78 +153,10 @@ public List<Point> GetPoints()
   /// </summary>
   /// <param name="index">The index of the texture coordinate</param>
   /// <returns>Texture coordinate as a <see cref="ValueTuple{T1, T2}"/></returns>
+  [Pure]
   public (double, double) GetTextureCoordinate(int index)
   {
     index *= 2;
     return (textureCoordinates[index], textureCoordinates[index + 1]);
   }
-
-  /// <summary>
-  /// If not already so, this method will align <see cref="Mesh.vertices"/>
-  /// such that a vertex and its corresponding texture coordinates have the same index.
-  /// This alignment is what is expected by most applications.<br/>
-  /// </summary>
-  /// <remarks>
-  /// If the calling application expects
-  /// <code>vertices.count == textureCoordinates.count</code>
-  /// Then this method should be called by the <c>MeshToNative</c> method before parsing <see cref="Mesh.vertices"/> and <see cref="Mesh.faces"/>
-  /// to ensure compatibility with geometry originating from applications that map <see cref="Mesh.vertices"/> to <see cref="Mesh.textureCoordinates"/> using vertex instance index (rather than vertex index)
-  /// <br/>
-  /// <see cref="Mesh.vertices"/>, <see cref="Mesh.colors"/>, and <see cref="faces"/> lists will be modified to contain no shared vertices (vertices shared between polygons)
-  /// </remarks>
-  public void AlignVerticesWithTexCoordsByIndex()
-  {
-    if (textureCoordinates.Count == 0)
-    {
-      return;
-    }
-
-    if (TextureCoordinatesCount == VerticesCount)
-    {
-      return; //Tex-coords already aligned as expected
-    }
-
-    var facesUnique = new List<int>(faces.Count);
-    var verticesUnique = new List<double>(TextureCoordinatesCount * 3);
-    bool hasColors = colors.Count > 0;
-    var colorsUnique = hasColors ? new List<int>(TextureCoordinatesCount) : null;
-
-    int nIndex = 0;
-    while (nIndex < faces.Count)
-    {
-      int n = faces[nIndex];
-      if (n < 3)
-      {
-        n += 3; // 0 -> 3, 1 -> 4
-      }
-
-      if (nIndex + n >= faces.Count)
-      {
-        break; //Malformed face list
-      }
-
-      facesUnique.Add(n);
-      for (int i = 1; i <= n; i++)
-      {
-        int vertIndex = faces[nIndex + i];
-        int newVertIndex = verticesUnique.Count / 3;
-
-        var (x, y, z) = GetPoint(vertIndex);
-        verticesUnique.Add(x);
-        verticesUnique.Add(y);
-        verticesUnique.Add(z);
-
-        colorsUnique?.Add(colors[vertIndex]);
-        facesUnique.Add(newVertIndex);
-      }
-
-      nIndex += n + 1;
-    }
-
-    vertices = verticesUnique;
-    colors = colorsUnique ?? colors;
-    faces = facesUnique;
-  }
-
-  #endregion
 }

src/Speckle.Objects/Geometry/Vector.cs

@@ -86,15 +86,9 @@ public bool TransformTo(Transform transform, out ITransformable transformed)
   /// Returns the coordinates of this <see cref="Vector"/> as a list of numbers
   /// </summary>
   /// <returns>A list of coordinates {x, y, z} </returns>
-  public List<double> ToList()
-  {
-    return new List<double> { x, y, z };
-  }
+  public List<double> ToList() => [x, y, z];
 
-  public Point ToPoint()
-  {
-    return new Point(x, y, z, units, applicationId);
-  }
+  public Point ToPoint() => new(x, y, z, units, applicationId);
 
   /// <summary>
   /// Creates a new vector based on a list of coordinates and the unit they're drawn in.
@@ -176,11 +170,6 @@ public static Vector CrossProduct(Vector u, Vector v)
     return new Vector(x, y, z, units: u.units);
   }
 
-  public static double Angle(Vector u, Vector v)
-  {
-    return Math.Acos(DotProduct(u, v) / (u.Length * v.Length));
-  }
-
   /// <summary>
   /// Compute and return a unit vector from this vector
   /// </summary>
@@ -205,23 +194,6 @@ public Vector Negate()
     return this;
   }
 
-  /// <summary>
-  /// Returns a normalized copy of this vector.
-  /// </summary>
-  /// <returns>A copy of this vector unitized.</returns>
-  public Vector Unit()
-  {
-    return this / Length;
-  }
-
-  /// <summary>
-  /// Constructs a new <see cref="Vector"/> from a <see cref="Point"/>
-  /// </summary>
-  /// <param name="point">The point whose coordinates will be used</param>
-  /// <param name="applicationId">The unique application ID of the object.</param>
-  [Obsolete($"Use {nameof(Point.ToVector)}", true)]
-  public Vector(Point point, string? applicationId = null) { }
-
   /// <summary>
   /// Gets or sets the coordinates of the vector
   /// </summary>

src/Speckle.Sdk/Common/Units.cs

@@ -1,4 +1,5 @@
 using System.Diagnostics.Contracts;
+using Speckle.Sdk.Dependencies;
 
 namespace Speckle.Sdk.Common;
 
@@ -19,7 +20,7 @@ public static class Units
   /// <summary>US Survey foot, now not supported by Speckle, kept privately for backwards compatibility</summary>
   private const string USFeet = "us_ft";
 
-  internal static readonly List<string> SupportedUnits = new()
+  internal static readonly IReadOnlyCollection<string> SupportedUnits = new[]
   {
     Millimeters,
     Centimeters,
@@ -30,7 +31,7 @@ public static class Units
     Yards,
     Miles,
     None,
-  };
+  }.Freeze();
 
   /// <param name="unit"></param>
   /// <returns><see langword="true"/> if <paramref name="unit"/> is a recognised/supported unit string, otherwise <see langword="false"/></returns>

tests/Speckle.Objects.Tests.Unit/Geometry/MeshTests.cs

@@ -6,22 +6,7 @@ namespace Speckle.Objects.Tests.Unit.Geometry;
 
 public class MeshTests
 {
-  private static readonly Mesh[] TestCaseSource = { CreateBlenderStylePolygon(), CreateRhinoStylePolygon() };
-
-  [Theory]
-  [MemberData(nameof(GetTestCaseSource))]
-  public void CanAlignVertices(Mesh inPolygon)
-  {
-    inPolygon.AlignVerticesWithTexCoordsByIndex();
-
-    inPolygon.VerticesCount.Should().Be(inPolygon.TextureCoordinatesCount);
-
-    var expectedPolygon = CreateRhinoStylePolygon();
-
-    inPolygon.vertices.Should().BeEquivalentTo(expectedPolygon.vertices);
-    inPolygon.faces.Should().BeEquivalentTo(expectedPolygon.faces);
-    inPolygon.textureCoordinates.Should().BeEquivalentTo(expectedPolygon.textureCoordinates);
-  }
+  private static readonly Mesh[] TestCaseSource = { CreateRhinoStylePolygon(), CreateEmpty() };
 
   public static IEnumerable<object[]> GetTestCaseSource() => TestCaseSource.Select(mesh => new object[] { mesh });
 
@@ -36,14 +21,51 @@ private static Mesh CreateRhinoStylePolygon()
     };
   }
 
-  private static Mesh CreateBlenderStylePolygon()
+  private static Mesh CreateEmpty()
   {
     return new Mesh
     {
-      vertices = new List<double> { 0, 0, 0, 0, 0, 1, 1, 0, 1, 1, 0, 0 },
-      faces = new List<int> { 3, 0, 1, 2, 3, 0, 2, 3 },
-      textureCoordinates = new List<double> { 0, 0, 0, 1, 1, 1, 0, 0, 1, 1, 1, 0 },
+      vertices = [],
+      faces = [],
+      textureCoordinates = [],
       units = Units.Meters,
     };
   }
+
+  [Theory]
+  [MemberData(nameof(GetTestCaseSource))]
+  public void GetTextureCoordinate_ReturnsCorrectUVValue(Mesh testCase)
+  {
+    for (int i = 0, j = 0; i < testCase.textureCoordinates.Count; i += 2, j++)
+    {
+      var (u, v) = testCase.GetTextureCoordinate(j);
+
+      u.Should().Be(testCase.textureCoordinates[i]);
+      v.Should().Be(testCase.textureCoordinates[i + 1]);
+    }
+
+    Assert.Throws<ArgumentOutOfRangeException>(() => testCase.GetTextureCoordinate(testCase.textureCoordinates.Count));
+  }
+
+  [Theory]
+  [MemberData(nameof(GetTestCaseSource))]
+  public void GetPoints_ReturnsVerticesAsPoints(Mesh testCase)
+  {
+    testCase.VerticesCount.Should().Be(testCase.vertices.Count / 3);
+
+    var getPoints = testCase.GetPoints();
+    var getPoint = Enumerable.Range(0, testCase.VerticesCount).Select(i => testCase.GetPoint(i));
+
+    //Test each point has correct units
+    getPoints.Select(x => x.units).Should().AllBe(testCase.units).And.HaveCount(testCase.VerticesCount);
+    getPoints.Select(x => x.units).Should().AllBe(testCase.units).And.HaveCount(testCase.VerticesCount);
+
+    //Convert back to flat list
+    var expected = testCase.vertices;
+    var getPointsList = getPoints.SelectMany(x => x.ToList());
+    var getPointList = getPoint.SelectMany(x => x.ToList());
+
+    getPointsList.Should().BeEquivalentTo(expected);
+    getPointList.Should().BeEquivalentTo(expected);
+  }
 }