Improve HalfEdgeTopology(::Vector{<:Connectivity}) correctness

[halleysfifthinc]

This PR reveals/fixes a couple of bugs in the HalfEdgeTopology constructor.

1. Dict's are unordered, so constructing the halves::Vector{Tuple{HalfEdge,HalfEdge}} by iterating through half4pair pairs produces a roughly random order (by element) for halves.

   Meshes.jl/src/topologies/halfedge.jl

   Line 220 in 3a3d0e3

   for ((u, v), he) in half4pair

   This has downstream affects on e.g. edge indices and the starting vertex index from loop. (Cause of most/all? test failures.)
2. Incorrect orientation consistency handling in the presence of incorrectly- or unsorted elements. A couple of things to note:
   • Treating incorrectly- or unsorted elements as a "no warranty" situation is perfectly fine, but the current/master state of the function can lead to non-closed edge loops, such that the loop function never terminates and allocates memory until Julia is OOM killed¹.

     • # without this redefinition, `loop` will eat all your memory
       function Meshes.loop(e::HalfEdge)
       n = e.next
       v = [e.head]
       cnt = 1
       while n != e
           if cnt < 3
               push!(v, n.head)
               n = n.next
               cnt += 1
           else
               throw(error("too many edges!"))
           end
       end
       v
       end
       
       e = connect.([(1,2,3), (1,3,4), (2,5,3), (5,4,6), (3,5,4)], Triangle)
       t = HalfEdgeTopology(e; sort=false)
       Meshes.loop(half4elem(t, 1)) # works
       Meshes.loop(half4elem(t, 2)) # errors

   • Minor nit: The orientation consistency variable name (CCW) was misleading, as actual element orientation isn't checked (and cannot be without actual vertices afaict?). Also, when there are multiple unconnected components (which I believe is still valid under a 2-manifold?) the orientation consistency checks don't/can't cross the disconnection, leading to further potential mismatches between the name CCW and reality. I renamed it to make this clearer.

The cause of most of the current test failures are due to changing to the halves construction to be element ordered. I believe this is more correct, and that any stable/consistent edge indices with the previous version are because the hashing algorithm in Base has not been changed between Julia versions (that will no longer be true after 1.13 with JuliaLang/julia#57509).

Another cause of test failures is a change to the reverse index iteration, which now ensures that the first index returned by loop (which affects several downstream dependent functions) is either:

1. The original first index of the element (regardless of whether the original element had an inconsistent orientation)
2. The (head) index of the first observed edge of the element

With regard to sorting and orientation consistency, the PR is correct (no infinite memory use) even for incorrectly- or unsorted elements when called with sort=false, but the edge indices will/may change between sort=false and sort=true.

If you agree with this PR's general direction. I will update the tests and share benchmarks.

Footnotes

1. In my code, I am redefining loop similar to the given example, but using a ScopedValue so that the maximum number of edges can be changed from outside the function if needed. I'm happy to share/submit that if you are interested, but I realize that such a solution might be too limiting generally to be added to the library. ↩

[juliohm]

Thank you @halleysfifthinc ! Left a few minor comments. I can review again after we get green tests.

[juliohm]

Let's avoid this auxiliary NULL_EDGE and make the logic more explicit. We can check the key in the half4pair and then do something else if it is not present.

[halleysfifthinc]

This way (using get with a default of NULL_EDGE) is faster because it avoids double indexing the Dict (compared to e.g. haskey(half4pair, uv) && !isnothing(half4pair[uv].elem)). The const externally defined NULL_EDGE is helpful because HalfEdge is mutable, which means defining a HalfEdge allocates. Defining NULL_EDGE in the function would incur that definition/allocation cost every time the function is called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (3a3d0e3) to head (608d7eb).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   88.31%   88.39%   +0.07%     
==========================================
  Files         196      196              
  Lines        6094     6136      +42     
==========================================
+ Hits         5382     5424      +42     
  Misses        712      712              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[halleysfifthinc]

Benchmarking procedure same as previously described, using sort=false to limit extraneous testing time/allocations:

Quads only:

BenchmarkTools.TrialJudgement: 
  time:   -66.93% => improvement (5.00% tolerance)
  memory: -46.14% => improvement (1.00% tolerance)

Quads and tris:

BenchmarkTools.TrialJudgement: 
  time:   -66.81% => improvement (5.00% tolerance)
  memory: -43.88% => improvement (1.00% tolerance)