Skip to content

fix bug with flipEdgeNetwork created by piecewise Dijkstra#237

Merged
nmwsharp merged 3 commits intonmwsharp:masterfrom
Joel-Hecht:fix_flipout_piecewise_dijkstra
Apr 27, 2026
Merged

fix bug with flipEdgeNetwork created by piecewise Dijkstra#237
nmwsharp merged 3 commits intonmwsharp:masterfrom
Joel-Hecht:fix_flipout_piecewise_dijkstra

Conversation

@Joel-Hecht
Copy link
Copy Markdown
Contributor

@Joel-Hecht Joel-Hecht commented Mar 3, 2026

Fixes #209

For some vertices A,B,C using shortestEdgePath() to connect vertex A to vertex B, and vertex B to vertex C would result in paths AB and BC that possibly contain one or more duplicate edges (i.e. one or more edges in AB has its twin in BC). The flipout algorithm is not able to flip these 'back-and-forth' loops (or the edges immediately before/after them), leading to incorrect shortening of the input path.

PR adds an option (enabled as default) in constructFromPiecewiseDijkstraPath() to remove any subset of the resulting curve that starts and ends at the same vertex, which, for all halfedges in the subset, also contains their twins.

Only fixes the 'artifacts' problem in the linked issue. Other 'problems' mentioned in the linked issue are fundamental misunderstandings of the algorithm.

Also adds a few contrived tests illustrating cases that would fail before, but now have the expected behavior. These tests are not very interesting and can probably get deleted.

Possibly also fixes this issue noted in comment in wedgeIsClear() (?):
TODO handle checks in case where the path lollipops out and back along a single edge

@Joel-Hecht Joel-Hecht changed the title fix bug with flipEdgeNetworks created by piecewise Dijkstra fix bug with flipEdgeNetwork created by piecewise Dijkstra Mar 3, 2026
@nmwsharp
Copy link
Copy Markdown
Owner

Thank you for this! The problem with the "out-and-back" piecewise Dijkstra paths makes sense. I see how it could happen and agree this simple change helps a lot at least.

We can't apply it when marking vertices, but otherwise I think it should always be enabled. I pushed a commit to this PR that makes that tweak, as well as some other cleanup around the code style and test organization, but the algorithm should be unchanged.

I'm going to go ahead and merge, let me know if you spot any issues I introduced in translation.

@nmwsharp nmwsharp merged commit bc93ad5 into nmwsharp:master Apr 27, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Artifacts on the resulting intrinsic mesh

2 participants