Fix QuadEdgeSubdivision remove method

[dr-jts]

This fixes this GEOS issue which causes an infinite loop for grids of regularly spaced points. The problem was caused by two bugs in the QuadEdgeSubdivision::remove method:

1. Since the method argument can be an edge in either base or sym orientation, the code to remove the corresponding QuadEdgeQuartet needs to check if the quartet base matches the arg edge OR its sym
2. The lambda capture clause must specify the edge parameter as e, not &e, since it is already a reference

[dbaston]

Thanks for fixing. I don't think #2 is a bug -- value capture and reference capture would be equivalent for a reference.

[dr-jts]

Thanks for fixing. I don't think 2 is a bug -- value capture and reference capture would be equivalent for a reference.

Well, it doesn't work without that change. And it fails in a really sneaky way... the edge seems to remove ok, but its point values end up different. (Verified by comparing traces from before and after the original commit). Unless there is something else going on?

[dbaston]

Actually they're not equivalent. Since the comparison &es.base() == &e is using addresses, and value capture [e] generates a new value at a new address, the comparison will always be false and nothing will be removed from quadEdges. Will look into changing point values.

[dbaston]

I think the remove method is fundamentally flawed in that QuadEdge objects use addresses to point to each other, but those addresses are not stable if we call quadEdges.erase(). So we would need to either (a) never remove anything from quadEdges, retaining stable addresses (as I think this PR does), (b) relax the requirement that QuadEdge memory locations remain stable (not sure if this is possible, haven't been in here for a while), or (c) revert the offending commit and return to individually-allocated `QuadEdges on the heap.

[dr-jts]

Ah, so you are saying that the change to the referencing to e turns the erase call into a no-op? And so this patch has the right effect in spite of being incorrect? In that case it sounds like we have de facto implemented a no-remove solution, which since it works seems ok to go forward with, at least until a better solution is found. And the other two options don't sound very feasible or worthwhile.

If the above is acceptable, we should remove the calls to erase.

[dr-jts]

It looks to me like the quadEdges deque is really only used as a container for the allocated quadEdges. So not removing detached edges from the container doesn't matter. In fact, I'm not even sure they need to be marked not alive, but there's no harm in leaving that code in place. (In JTS it's not even clear that the quadEdges list is needed at all.)

[pramsey]

Yeah, as long as the container isn't going to grow without bound as the algorithm chugs along, there's no reason not to just let it sit and just quietly deallocate everything when the algorithm completes.

[dbaston]

Sounds good. Thanks, @dr-jts .