AdaptiveGrid. Fix tolerances and vertices loopkup.

[DmytroMuravskyi]

BACKGROUND:

• During my work with AdaptiveGrid I encountered weird errors time to time: two vertices were inserted into the same spot, or I was not able to retrieve the vertex that is there by it approximate coordinates within the tolerance.

DESCRIPTION:

• AdaptiveGrid.AddCutEdge: provided missing tolerance parameter PointOnLine. It's necessary since non default tolerance is used in AdaptiveGrid.
• Define AdaptiveGrid.Tolerance as distance tolerance. Set coordinate tolerance as half of tolerance, so sum of deviations is still less than twice the tolerance.
• Rework vertices lookup of AdaptiveGrid. Don't just check the first suitable X/Y coordinate. Go over each suitable coordinate branch until good vertex is found. The complexity stays the same but function is no longer error prone.

TESTING:

• Added ClipperTest file that shows data modification tolerances.
• Added AdaptiveGridFindsCorrectVertex test that shows weak spot of previous lookup implementation.
• All existing tests are passing.

FUTURE WORK:

• I'm considering not using Grid2D for splitting Polygon by a set points. The problem is simple enough and not really require boolean operation. This way I'll eliminate input data modifications made by Clipper.

REQUIRED:

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

COMMENTS:

• Any other notes.

---

This change is 

[wynged]

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

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 65 at r1 (raw file):

        /// <summary>
        /// Distance tolerance for points being considered the same.
        /// Tolerance is twice the epsilon because gird uses single tolerance for individual coordinates snapping.

spelling "grid"

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1444 at r1 (raw file):

        }

        private static IEnumerable<T> GetValue<T>(Dictionary<double, T> dict, double key, double? tolerance = null)

if this returns an IEnumerable it should be called GetValues with an 's'

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1463 at r1 (raw file):

        }

        private ulong GetVertexFromDictionary(Vector3 point,

here and the next methods that are To/FromDictionary I don't think we need to specify "FromDictionary" There is an internal dictionary that is used to store these things, but that's not important to the method is it?

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1471 at r1 (raw file):

            zDict = null;

            foreach (var yz in GetValue(_verticesLookup, point.X, tolerance))

can we rename some of these dictionary variables? _verticesLookup is kind of a xyzDict, but so we could rename it to that, or maybe go with xyzLookup, yzLookup, and zLookup rather than calling them dict? I think that would be more consistent.

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1511 at r1 (raw file):

            if (id == 0 || id != vertex.Id || zDict == null || yzDict == null)
            {
                throw new Exception("Vertex can't be removed. Coordinate dictionary is broken.");

This warning seems not quite right, if id==0 it could also be that there's simply no vertex at that point right?

---

Elements/test/ClipperTest.cs line 11 at r1 (raw file):

{
    public class ClipperTest
    {

can we get a comment description of why we have this test?

---

Elements/test/ClipperTest.cs line 13 at r1 (raw file):

    {
        [Fact]
        public void CliperChangesInputNumbers()

spelling, two 'p' in Clipper

---

Elements/test/ClipperTest.cs line 19 at r1 (raw file):

            var random = new Random();
            var z = 1.23456789;
            random.NextDouble();

I don't think we should use random in our tests unless it's required. Could we use a fixed list of values?

---

Elements/test/ClipperTest.cs line 32 at r1 (raw file):

            polygon = polygon.TransformedPolygon(new Transform().Rotated(Vector3.ZAxis, 25));

            var tolerance = Vector3.EPSILON * 2;

describe the significance of the *2 for the epsilon here.

---

Elements/test/ClipperTest.cs line 47 at r1 (raw file):

                Assert.True(changedPolygon.Vertices[i].Z == 0d);

                var v2d = new Vector3(polygon.Vertices[i].X, polygon.Vertices[i].Y);

why make a new Vector3 from the Vertex? we compare it to a polygon vertex below, are they different types somehow?

[DmytroMuravskyi]

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

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 65 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

spelling "grid"

Done.

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1444 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

if this returns an IEnumerable it should be called GetValues with an 's'

Done.

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1463 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

here and the next methods that are To/FromDictionary I don't think we need to specify "FromDictionary" There is an internal dictionary that is used to store these things, but that's not important to the method is it?

I don't want it to be just another AddVertex/GetVertex/RemoveVertex as it's doing only part of the job. Maybe instead of abstract "dictionary" Use AddToXYZLookup / GetFromXYZLookup / RemoveFromXYZLookup?

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1471 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can we rename some of these dictionary variables? _verticesLookup is kind of a xyzDict, but so we could rename it to that, or maybe go with xyzLookup, yzLookup, and zLookup rather than calling them dict? I think that would be more consistent.

Since I can use existing variable in foreach, I need two names per variable. I ended up with:

_verticesLookup -> _xyzLookup
yzLookup and zLookup for parameter names
yzToId and zToId for internal names

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1511 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

This warning seems not quite right, if id==0 it could also be that there's simply no vertex at that point right?

At this point we want to remove one of existing vertices. If the vertex is not part of the grid, we will fail much earlier. Removing from lookup is like cleanup, so if we can't something is really wrong happened.

Code quote:

zDict == null || yzDic

---

Elements/test/ClipperTest.cs line 47 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

why make a new Vector3 from the Vertex? we compare it to a polygon vertex below, are they different types somehow?

Clipper works only with 2D points so it's Z is always 0. I created a 2D point to calculate distance.

[wynged]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1463 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

I don't want it to be just another AddVertex/GetVertex/RemoveVertex as it's doing only part of the job. Maybe instead of abstract "dictionary" Use AddToXYZLookup / GetFromXYZLookup / RemoveFromXYZLookup?

That naming would work for me

---

Elements/src/Spatial/AdaptiveGrid/AdaptiveGrid.cs line 1511 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

At this point we want to remove one of existing vertices. If the vertex is not part of the grid, we will fail much earlier. Removing from lookup is like cleanup, so if we can't something is really wrong happened.

done. ahh, ok, thanks.

---

Elements/test/ClipperTest.cs line 32 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

describe the significance of the *2 for the epsilon here.

done, thanks.

---

Elements/test/ClipperTest.cs line 47 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Clipper works only with 2D points so it's Z is always 0. I created a 2D point to calculate distance.

but if the point is already 2D do you need to make a new 2D point?

[DmytroMuravskyi]

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

---

Elements/test/ClipperTest.cs line 47 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

but if the point is already 2D do you need to make a new 2D point?

Original points are with 1.23456789 elevation to show that it's lost after we go back

Code quote:

1.23456789;

[wynged]

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

---

Elements/test/ClipperTest.cs line 47 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Original points are with 1.23456789 elevation to show that it's lost after we go back

oh derp.... thanks!

[wynged]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @DmytroMuravskyi)