Topography.Trimmed

[andrewheumann]

BACKGROUND:

• There was a request from some customers to support trimming a topography with a profile. The existing Trim method wasn't exactly what was needed, because it:
  • left behind a solid rather than just the top surface
  • didn't support voids within the projected shape
  • mutated the existing topography instead of creating a new mesh
    DESCRIPTION:
• adds Topography.Trimmed, which returns a new Topopgraphy trimmed by a Profile
• adds Topography.TopMesh which returns just the upwards facing portion of the topo without the sides
• Updates Topography.Trim to support Profile instead of Polygon
• Fixes an issue where cloning a mesh would result in incorrect vertex-triangle relationships, due to an oversight in one Triangle constructor

TESTING:

• Added a test which tests all new methods:

FUTURE WORK:

• calling GetNakedBoundaries results in some really funky shapes with these meshes... there's still something weird about the mesh topology after a CSG operation.

REQUIRED:

• All changes are up to date in CHANGELOG.md.

---

This change is 

[anthonie-kramer]

Just a comment about deep copies, and a question about the updated constructor

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)

---

Elements/src/Topography.cs line 226 at r1 (raw file):

        /// Construct a new Topography by duplicating another one.
        /// </summary>
        public Topography(Topography topography)

My preference when making a copy of an object is just to create a deep copy which is safer and doesn't require maintenance... Not as performant as your constructor code below, but thought I would mention it.

Code snippet:

        public Topography CreateDeepCopy(Topography topography)
        {
            return JsonConvert.DeserializeObject<Topography>(JsonConvert.SerializeObject(topography));
        }

---

Elements/src/Geometry/Triangle.cs line 34 at r1 (raw file):

            foreach (var vert in vertices)
            {
                if (!vert.Triangles.Contains(this))

I'm trying to understand how this works... Since this is currently being created, how could vert.Triangles.Contains(this)?

Unless we are overriding the Equals method... but then I think we would be creating duplicate objects that have different references?

[andrewheumann]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)

---

Elements/src/Topography.cs line 226 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

My preference when making a copy of an object is just to create a deep copy which is safer and doesn't require maintenance... Not as performant as your constructor code below, but thought I would mention it.

Everywhere else we do Copy constructors in Elements we do it expressly, like this. (Grid1d, Box, Mesh, etc). Serialize/Deserialize feels like a hack to me — I can see how it would be safe, but especially since serialized Topography objects can be quite large, I feel like it's maybe not appropriate here.

---

Elements/src/Geometry/Triangle.cs line 34 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

I'm trying to understand how this works... Since this is currently being created, how could vert.Triangles.Contains(this)?

Unless we are overriding the Equals method... but then I think we would be creating duplicate objects that have different references?

This is a good catch — I guess it's not possible for the vertex to already have this triangle when accessed via this pathway. removed!

[anthonie-kramer]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

---

Elements/src/Topography.cs line 226 at r1 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

Everywhere else we do Copy constructors in Elements we do it expressly, like this. (Grid1d, Box, Mesh, etc). Serialize/Deserialize feels like a hack to me — I can see how it would be safe, but especially since serialized Topography objects can be quite large, I feel like it's maybe not appropriate here.

For sure, there really aren't any efficient ways to create a deep copy. We are probably safe if this object is not likely to expand its properties in the future.