DelaunayVoronoi icon indicating copy to clipboard operation
DelaunayVoronoi copied to clipboard

AdjacentTriangles not cleared when the graph is recreated

Open Roland09 opened this issue 5 years ago • 1 comments

Very awesome implementation, thank you very much for giving it the MIT license.

I made your repository interactive, i. e. allow the user to add points via mouse click. I noticed that you have to clear the AdjacentTriangles map in the Points class

https://github.com/RafaelKuebler/DelaunayVoronoi/blob/f9fe717b6d209d0b1320541bb0a4f4580e115555/DelaunayVoronoi/Point.cs#L9

whenver you recreate the graph.

Reproduction:

Make

https://github.com/RafaelKuebler/DelaunayVoronoi/blob/f9fe717b6d209d0b1320541bb0a4f4580e115555/DelaunayVoronoi/MainWindow.xaml.cs#L17

a class member.

Then invoke

https://github.com/RafaelKuebler/DelaunayVoronoi/blob/f9fe717b6d209d0b1320541bb0a4f4580e115555/DelaunayVoronoi/MainWindow.xaml.cs#L19-L29

then add a new point to the points list and re-invoke the graph creation again.

AdjacentTriangles just grows and grows and the graph will get unconnected (old) lines when it is repainted.

Just fyi in case you want to address this.

Roland09 avatar Dec 27 '19 12:12 Roland09

Hi @Roland09, thanks for the interest in the project! I am happy to hear the implementation has been useful for you. I really like the work you have done on your fork.

The issue you mention is indeed unexpected behavior and undesired. However, it stems from a deeper problem which is the storing of the AdjacentTriangles map in the Point class. This was meant as an optimization, but it pollutes what should be a POD type. As making the diagram interactive was not intended in the beginning, the underlying (though undocumented) assumption was that the point list is re-generated before every run and therefore 'clean'. Clearing all AdjacentTriangles at the beginning of each run would introduce some overhead, which in this non-interactive version is overkill? I am well aware of this drawback but somewhat reluctant to introduce an additional loop over all points (in this specific implementation). Nevertheless, I am very open to suggestions and would appreciate further input, as I agree from an engineering perspective it is kinda broken.

Cheers

RafaelKuebler avatar Apr 26 '20 10:04 RafaelKuebler