Fix output windings#14
Open
keijokapp wants to merge 3 commits into
Open
Conversation
keijokapp
force-pushed
the
fix-output-windings
branch
2 times, most recently
from
July 22, 2026 23:59
43b222e to
4486e5f
Compare
keijokapp
force-pushed
the
fix-output-windings
branch
from
July 23, 2026 00:01
4486e5f to
426ecac
Compare
keijokapp
force-pushed
the
fix-output-windings
branch
from
July 23, 2026 08:24
62d4fbc to
27836c1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13
This PR fixes two issues with the output region windings and significantly simplifies the code on the way. The README says that filled rings should be CCW and holes CW but that isn't often the case.
The chain finalizing code incorrectly expects the
fillflag to mean the fill status of the polygon but that's only the case if the first segment of the chain happened to be CCW and the fill flag isn't changed. The flag actually means something like "is the chain correctly winded?". The basic fix would be to skip the winding calculation and just reverse the chain iffill === false. But the flag could be removed altogether by simply keeping the chains correctly winded. The chains are now always winded so that the left side is filled. The segment itself is also reversed before processing so that it's filled on the left side, so many logic branches and additional reversing code could be removed.If two regions (both filled or both unfilled) touch at the corner, the code used to incorrectly chains the segments from different polygons together, not considering the fill statuses/windings (see picture below). That results in self-intersecting polygons. The first step to solving that would be to only match segment with a chain if the fill statuses also match. Since all chains and segments are now always winded consistently, "checking" the fill status means simply not testing the segment start against chain head and segment end against chain tail. A downstream problem is that now a single endpoint could be part of multiple chains and the chain joining code could get confused. The solution is to ignore one of the chains. If the chain that segment connects to at the other end (the segment should always connect to one of the chains at the other end) is ignored then that would result a larger self-touching polygon. If the other chain is ignored, then the polygon could be closed, resulting in two distinct polygons.
The later is chosen because it's easier to implement this behavior so it would work consistently.EDIT: It's impossible to get a consistent behavior with a simple sweep over the segment array, so I removed the code which attempted to do that.