Do not use Grid2d.GetTrimmedGeometry for populating AdaptiveGrid from Grid2d

[DmytroMuravskyi]

BACKGROUND:

• Grid2d.GetTrimmedGeometry was used to convert perimeter and set of points into a set of edges representing gird. But it has few disadvantages:
  a) it's slow since it performs complex polygon to polygon intersection and every edge is added twice.
  b) GetTrimmedGeometry uses Clipper that performs double to int conversions for each coordinate of point. This leads to sometimes painful precision loss, even if there is no transformation involved.

DESCRIPTION:

• Grid2d is still used but, custom algorithm used for instead, that creates lines instead of polygons.

TESTING:

• Added new test AdaptiveGridRotatedHasExactPoints to check if points are as expected on rotated grid.

FUTURE WORK:

• We can consider return AdaptiveGrid.Tolearnce to 1e-5.
• The algorithm can became a part of public interface of some class, Grid2D.GetLineGrid for example.

REQUIRED:

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

COMMENTS:

• Any other notes.

---

This change is 

[wynged]

Reviewed 3 of 3 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 1340 at r1 (raw file):

            }

            var ans = grid.Cells.Select(GetGridDividers).Select(l => l.First()).ToList();

what is ans?

also, I don't quite get whey we are returning a list of min and max only to get the first of the list in the next step.

---

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

        }

        private List<(Vertex from, Vertex to)> SplitSegmentWithPoints((Vector3 from, Vector3 to) line, List<Vertex> points)

lets avoid using from since it's a keyword. can we use start/end instead?

---

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

        }

        private List<(Vertex from, Vertex to)> SplitSegmentsWithPoints(

change wrapping please, wrap and align arguments

---

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

            foreach (var segment in segments)
            {
                var previousPoint = segment.Start;

why can't SplitSegmentsWithPoints use the singular SplitSegmentWithPoint internally?

Code quote:

SplitSegmentWi

---

Elements/test/AdaptiveGridTests.cs line 138 at r1 (raw file):

        public void AdaptiveGridRotatedHasExactPoints()
        {
            var adaptiveGrid = new AdaptiveGrid(new Transform().Rotated(Vector3.ZAxis, 26.5650512)); //tan (26.5650512) = 0.5

can you add more comment about why we want the 0.5 rotate or why we have the tan value here?

[DmytroMuravskyi]

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

---

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

Previously, wynged (Eric Wassail) wrote…

what is ans?

also, I don't quite get whey we are returning a list of min and max only to get the first of the list in the next step.

I decided to remove this function but rather make Grdi1d.DomainsToSequence public as GetCellDomains. It does the same work and cleaner

---

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

Previously, wynged (Eric Wassail) wrote…

lets avoid using from since it's a keyword. can we use start/end instead?

Done.

---

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

Previously, wynged (Eric Wassail) wrote…

change wrapping please, wrap and align arguments

Done.

---

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

Previously, wynged (Eric Wassail) wrote…

why can't SplitSegmentsWithPoints use the singular SplitSegmentWithPoint internally?

They are quite different: one takes a line and set of unordered points on it and splits the line by them. Other takes a list of ordered line segments that belong to the same infinite line, in local transformation, so line segments lay on imaginary X axis, and list of ordered parameters on this infinite line and split using them. We could find something in common but there is not much besides return type, few parameters and the fact that both add vertices. In my opinion, in will do more head pain than gain. I remove one function into SplitLineSegmentsWithParameters.

---

Elements/test/AdaptiveGridTests.cs line 138 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can you add more comment about why we want the 0.5 rotate or why we have the tan value here?

Done.

[wynged]

couple comment edits

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

---

Elements/test/AdaptiveGridTests.cs line 138 at r2 (raw file):

        public void AdaptiveGridRotatedHasExactPoints()
        {
            //tan (26.5650512) = 0.5. This is convinient for testing edges that are connected to the middle of the squre

convinient -> convenient
squre -> square

---

Elements/test/AdaptiveGridTests.cs line 139 at r2 (raw file):

        {
            //tan (26.5650512) = 0.5. This is convinient for testing edges that are connected to the middle of the squre
            //will be projected to the perimeter differenty for X and Y coordinates but still in a way that is easy to

"They will"
differenty -> differently

---

Elements/test/AdaptiveGridTests.cs line 140 at r2 (raw file):

            //tan (26.5650512) = 0.5. This is convinient for testing edges that are connected to the middle of the squre
            //will be projected to the perimeter differenty for X and Y coordinates but still in a way that is easy to
            //understand: one coordinate is twice as displaced as other by definition of tan.

period after understand.
"One coordinate is twice as displaced as the other."

---

Elements/test/AdaptiveGridTests.cs line 141 at r2 (raw file):

            //will be projected to the perimeter differenty for X and Y coordinates but still in a way that is easy to
            //understand: one coordinate is twice as displaced as other by definition of tan.
            //The resulting edges will be (5, 5) -> (7.5, 0), (5, 5) -> (0, 2.5), etc.

spaces after all //

[wynged]

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