Fix S-57 area spike artifacts from merged interior holes#469
Merged
Conversation
S57ToS101Translator.TranslateAreaSpatial grouped every consecutive interior (USAG=2) edge into a single interior composite curve, so areas with multiple holes had all holes merged into one ring. Flattening that ring to coordinates jumped between holes, producing long straight "spike" segments through curves (visible in line/outline rendering; hidden by area fills). Chain area boundary edges into contiguous rings by shared begin/end node identity, reversing edges as needed, so each hole becomes its own interior ring. Applied to exterior edges too for ordering robustness. Verified on NOAA US3WA01M (Strait of Juan de Fuca): worst interior-ring jump 1.32 deg -> eliminated; the two new regression tests fail without the fix (spike = 8.485E-05 deg) and pass with it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f45ed968-6f44-421b-bd0f-0519d83599e0
Contributor
Performance Gate✅ PASSED — no regressions. Threshold: 10.0%, MAD multiplier (k): 3.0, retry-zone mult: 2.0× Scenario summary
exchange-set-openIteration statistics
Spans (sum of all iterations)
Metrics
s101-portray-coldIteration statistics
s101-portray-warmIteration statistics
s101-real-coldIteration statistics
s101-real-warmIteration statistics
s101-render-warmIteration statistics
s102-coverageIteration statistics
s102-coverage-openIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-largeIteration statistics
s102-real-warmIteration statistics
s111-real-warmIteration statistics
s124-vectorIteration statistics
s201-vectorIteration statistics
Generated by EncDotNet.S100.PerfReport gate command |
There was a problem hiding this comment.
Pull request overview
Fixes S-57 area “spike” artifacts caused by incorrectly merging multiple interior holes into a single boundary when translating S-57 areas into S-101 surface topology.
Changes:
- Updates
TranslateAreaSpatialto chain boundary edges into contiguous rings by shared begin/end node identity, emitting one ring per hole (and improving exterior ordering robustness). - Adds regression tests covering a two-hole area translation and asserting the absence of cross-hole “spike” segments.
- Documents the ring-chaining behavior and rationale in the S-57 dataset README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/EncDotNet.S100.Datasets.S57/S57ToS101Translator.cs |
Rebuilds area boundary rings by node contiguity and introduces helpers for edge chaining and node lookup. |
tests/EncDotNet.S100.Datasets.S57.Tests/S57ToS101TranslatorTests.cs |
Adds regression tests to ensure multi-hole areas produce separate interior rings and no cross-hole spikes. |
src/EncDotNet.S100.Datasets.S57/README.md |
Documents why and how area rings are reassembled to avoid spike artifacts. |
Address PR #469 review feedback: - ChainEdgesIntoRings now seeds each ring using the seed edge's own FSPT orientation (deriving start/end nodes accordingly) instead of hard-coding forward, so reconstructed rings stay consistent with the encoded orientation and the XML docs. - Harden BuildTwoHoleArea regression fixture: boundary pointers are shuffled and each ring's first edge is referenced in reverse, so the tests fail if chaining relies on input order or orientation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f45ed968-6f44-421b-bd0f-0519d83599e0
Address PR #469 review feedback: - ChainEdgesIntoRings now precomputes each edge's (begin,end) nodes once and indexes edges by incident node, so each extension step looks up connecting edges directly instead of rescanning the pool and re-calling EdgeNode. This replaces the O(n^2) scan (noticeable on large NOAA areas with many holes) while preserving identical output: input-order seeding, seed FSPT orientation, earliest-unused-incident tie-break, and forward-before-reverse handling. - Compute MaxSegment(ring) once in the no-cross-hole-spike test assertion and reuse it in both the condition and the failure message. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f45ed968-6f44-421b-bd0f-0519d83599e0
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.
Fixes #470
Problem
Viewing S-57 (NOAA ENC) datasets showed large straight "spike" segments cutting across otherwise-smooth sea/land area curves — most visible in line/outline rendering (area fills hid them as zero-width slivers). Reproduced with NOAA
US3WA01M(Strait of Juan de Fuca).Root cause
This repo renders S-57 by translating each cell in-memory to an
S101Document(S57ToS101Translator) and reusing the S-101 pipeline. InTranslateAreaSpatial, every consecutive interior (USAG=2) boundary edge of an area was grouped into a single interior composite curve. S-57 lists all of an area's interior edges consecutively regardless of which hole they belong to, so multi-hole areas had all their holes merged into one ring. Flattening that ring to coordinates jumped from the end of one hole to the start of the next, producing the long "spike" segments.Empirically, on
US3WA01M: 71 of 134 interior composites merged more than one disjoint hole (e.g.DepthAreamerging 4 holes with a 1.32° jump;QualityOfBathymetricDatamerging 151 chains into one ring).Fix
TranslateAreaSpatialnow chains area boundary edges into contiguous rings by shared begin/end node identity (reversing individual edges as needed), emitting one composite/ring per hole. Applied to exterior edges too for ordering robustness. Two small helpers were added:ChainEdgesIntoRingsandEdgeNode.Verification
dotnet format whitespace/style IDE0005clean.Files changed
src/EncDotNet.S100.Datasets.S57/S57ToS101Translator.cs— the fixtests/EncDotNet.S100.Datasets.S57.Tests/S57ToS101TranslatorTests.cs— regression testssrc/EncDotNet.S100.Datasets.S57/README.md— documented ring-chaining behavior