Skip to content

first snap, then clip - #24

Merged
traines-source merged 1 commit into
masterfrom
fix-beaches
Nov 3, 2026
Merged

traines-source merged 1 commit into
masterfrom
fix-beaches

Conversation

@traines-source

Copy link
Copy Markdown
Member
  • revert winding correction, because winding fixes are handled by clipper
  • clipper's polygon tree sometimes attaches holes to the wrong outer ring. Assign holes to smallest outer.

testing planet on test pending

so winding fixes are handled by clipper
assign holes to smallest outer
@felixguendling

Copy link
Copy Markdown
Member

maybe this is better in Clipper2?
#21

Clipper2 would speed up rectangular clipping (currently we use the generic clipping for polygons, no special case for rectangles).

@traines-source

Copy link
Copy Markdown
Member Author

It appears that this is not better in clipper2, and that we don't actually want to use the rectangular clipping because it makes the flooding worse, meaning the speedup is also lost. for coastline import clipper2 does appear to make sense (and speeds up 2x), so it is probably still worthwile to upgrade. i would then proceed to fix the clipper2 PR?

@felixguendling

Copy link
Copy Markdown
Member

#21 might be in a rough state - it was just an experiment and numbers so far looked good.

and that we don't actually want to use the rectangular clipping because it makes the flooding worse

maybe the to_fixed_polygon causes this?

not sure why rectangular clipping makes the issue worse? The "Assign holes to smallest outer." strategy does not help?

@traines-source

Copy link
Copy Markdown
Member Author

I haven't looked into it in detail, but: "It only cuts rings at the tile edge and doesn't remove self-intersections. I tested RectClip followed by an even-odd union in Clipper2 to clean up afterwards. Quality was the same (10 bad features, worst 0.025%), but it was only about 5% faster (11.4 s vs. 12.0 s), and its output differs in 237 tiles. That's not worth the extra step. "

The custom hole assignment is still needed anyways.

@felixguendling

Copy link
Copy Markdown
Member

5% faster

since we run this also in prod (tiles endpoint), I would say 5% is better than nothing

but for the sake of progress let's just merge the fix asap :)

@traines-source
traines-source merged commit 15bd427 into master Nov 3, 2026
8 checks passed
@traines-source
traines-source deleted the fix-beaches branch November 3, 2026 14:32
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.

2 participants