Fix bug with concave hull algorithm

[JavedNissar]

As noted in a-b-street/abstreet#198, there seems to be a bug where the concave hull algorithm doesn't always "bend" edges inwards when it should. So, to fix this I've noticed that a parameter which I thought was the radius of the circle to search was actually the radius squared of the circle to search. So, I corrected this. I've also removed the Norway test since it's too brittle IMO.

[michaelkirk]

little readability nit: I think generally we should prefer the infix notation where available:

Suggested change

	let four = T::add(two, two);
	let four = two + two;
	let search_distance = diagonal_squared / four;

[michaelkirk]

I'm having a little trouble following this - does this method correspond to concaveman's findCandidate method? Or has our implementation diverged sufficiently that there's not really a corollary?

[michaelkirk]

concaveman uses a rectangular envelope where the borders are a distance of max_dist from the line to look for points. Unfortunately, rstar uses a circular envelope

That's interesting - is that something inherent in the rstar implementation? Can you link to any doc or code example that highlights this difference?

[JavedNissar]

The infix notation usage sounds reasonable. I'll set that up.

With regard to findCandidate, yeah, this is reasonably equivalent. The main difference is that findCandidate implements the traversal of the queue within itself while I offload that onto rstar.

With regard to where concaveman uses a rectangular envelope, I'm embarrassed to admit that it's not entirely clear from looking at the code again where that is. With regard to rstar, I use locate_within_distance which references a radius r which I took to mean that it looks for points within a circle of radius r. I'll have to dive into the concaveman code again.

[JavedNissar]

@michaelkirk From checking again, it seems I was wrong to think that concaveman used a rectangular envelope to search within. It seems to be using a circle just like rstar, I'm not entirely sure where I got the impression that it used a rectangular envelope. I've adjusted the calculation to be simpler.

[michaelkirk]

Thanks for double checking the envelop shape stuff @restitutororbis!

Since we don't have many tests to debug edge cases, anything we can do to make our code look more like the reference implementation (assuming that's correct) would make review easier.

Since our implementation has diverged from concaveman it was hard to see, but it looks like we compute max distance as the (line_length / concavity)^2 whereas concaveman has max distance as (line_length^2 / concavity^2). Is that divergence intentional?

[JavedNissar]

That divergence was intentional on the basis that those are equivalent statements anyways (ex. (x/y)^2=x^2/y^2) and it would be easier to understand the function call if it was handing over dist instead of a stranger notion of squared_dist.

[michaelkirk]

🤣 apparently I wasn't ready to do 4th grade math yesterday AM.

[JavedNissar]

@michaelkirk Hey! I finally found the time to get around to this. So, after checking against A/B Street, I found that those edges which visually look like they should flex don't because they are not the closest edge to their closest interior point.

For example, in abstreet_test, the relevant shape has been plucked from A/B Street. In that shape, the edge (355.333, 72.2397) -> (800.5243, 864.0099) looks like it should flex but it doesn't because it's closest interior point is (799.6805, 865.2500) whose closest line is (800.5243, 864.0099) -> (799.1702, 874.9085). Since the closest line of the closest interior point doesn't match (355.333, 72.2397) -> (800.5243, 864.0099), the algorithm doesn't flex the edge in response.

There are a few ways to resolve this:

1. Be more aggressive and don't just look for the closest interior point but look further off if the closest interior point isn't a good candidate. I don't like this approach because it will hurt performance.
2. Don't check if the current edge is the closest edge to it's closest interior point. This will hurt robustness because now the algorithm is sensitive to the order in which the convex hull returns edges.
3. Don't fix and acknowledge this as acceptable behaviour.

I'm inclined to say 3 because there is no real definition of what a concave hull should be and the current behaviour strikes me as reasonable.

[michaelkirk]

Hi @restitutororbis - thanks for continuing to look at this!

Here's what I'm seeing with this commit:

Do you think concaveman would give the same results? Or do you think it's an artifact in our implementation?

[JavedNissar]

The implementations are sufficiently different that expecting this implementation to be equivalent to concaveman probably isn't ideal. That being said, taking one of the input polygons from A/B Street and pumping them into Georust and Concaveman separately yields the following result. The first one is concaveman and the second is Georust:


Concaveman seems to be much more aggressive than Georust and this is with the same concavity value of 0.1. One thing I could try doing is removing the robustness check but that leads to weirder shapes so if that change were to be merged in, we would presumably want to have a flag on the algo to see if it works with the robustness check or without the robustness check.

[michaelkirk]

The implementations are sufficiently different that expecting this implementation to be equivalent to concaveman probably isn't ideal. That being said, taking one of the input polygons from A/B Street and pumping them into Georust and Concaveman separately yields the following result. The first one is concaveman and the second is Georust

Thanks for doing the work to compare results.

Do you understand where and why we are diverging from concaveman's implementation? To my eye it looks like we want something more like concaveman's results - if that means making our implementation more fully align with theirs, I think we should.

Is there a reason we intentionally diverged?

[JavedNissar]

The only divergence that comes to mind is the robustness check. If we remove that, we get the following:

I think trying to get any closer would probably necessitate a rewrite since I suspect that any further divergence is subtle. Does this seem reasonably close to you?

[michaelkirk]

I think trying to get any closer would probably necessitate a rewrite since I suspect that any further divergence is subtle. Does this seem reasonably close to you?

This one example looks pretty close to my eye, but it's hard for me to say if it's a reasonable solution in general.

Since this code is based on a reference implementation, rather than any rigorous definition of "what concavity is", I'd prefer matching the reference implementation unless there's some reason we intentionally diverged.

[JavedNissar]

Unfortunately, I don't think I can do that. From my perspective, I was using concaveman more for inspiration than as a reference. If we want to use concaveman as a reference, then it might be reasonable to open an issue to rewrite the algo to align it with concaveman.

[michaelkirk]

Fair enough! Thanks for looking into it as much as you have. I'll try to take a pass at it in the next couple weeks unless someone else gets to it first.