check points for distance instead

[jamesbradleym]

Evidently using the distance from one line to another was not sufficient to determine if projection was needed. This PR compares the end points of the lines for to determine if they are within tolerance, but further than epsilon, and thus needs projection.

---

This change is 

[wynged]

Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @jamesbradleym)

---

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 128 at r1 (raw file):

                            {
                                // project lines with points within tolerance of eachother but further than epsilon
                                if (LinesWithinTolerance(line, otherLine, tolerance))

I'm having trouble figuring what we've fundamentally changed and be sure we're not discarding something we might want to join.

like, what if there are two lines that overlap where the overlap is greater than tolerance, they won't pass the new check right, because their endpoints aren't close enough. is that what we want?

Can you help me understand the failure mode of the original code better?

[jamesbradleym]

Well we only want to join things that are within tolerance of one another, that's the fuzzy distance we're defining as "The user is intending to join these but they just didn't perfectly snap the spaces together to get a straight wall".

If two lines overlap they do not require projection to be successfully merged with MergedCollinearLine. It doesn't matter that those end points are not within tolerance because they don't need projection and are already in the flow of being merged after passing:
if (line.TryGetOverlap(otherLine, out var overlap) || line.DistanceTo(otherLine) < tolerance)

The original code failed because judging the distance between two lines did not provide a distance that triggered the need for projection even though the line was off axis, aka was a distance between epsilon and tolerance.

[jamesbradleym]

Per conversation:

At the point of reaching line merging we have grouped collinear lines, however line.IsCollinear(otherLine) let's nearly collinear lines join the group. This is all well and good, however line.MergedCollinearLine(otherLine) does not tolerate near collinearity and thus throws.

Therefore we must ensure these lines are actually collinear and actually within tolerance.

With that said, line.DistanceTo(otherLine) resolves to 0 for near 0 values while line.MergedCollinearLine(otherLine) does not tolerate this near zero distance. (Hence the previous commit to check for an actual near zero distance). Rather than check, let's project.

[wynged]

Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @jamesbradleym)

---

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 128 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

I'm having trouble figuring what we've fundamentally changed and be sure we're not discarding something we might want to join.

like, what if there are two lines that overlap where the overlap is greater than tolerance, they won't pass the new check right, because their endpoints aren't close enough. is that what we want?

Can you help me understand the failure mode of the original code better?

done.

---

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 127 at r2 (raw file):

                                // we project the lines because line.IsCollinear resolves to true on
                                // near 0 differences which MergedCollinearLine does not tolerate
                                // line.DistanceTo is similarly fuzzy and resolves to 0 on near (but greater than epsilon) distances

might not need this last line of the comment, unless you're going to add a note that this is why we're not using that method to determine if projection is necessary.

[wynged]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 1 approvals obtained, 1 unresolved discussion (waiting on @jamesbradleym)