Skip to content

fix: drop degenerate coverage cells and publish prebuilt manylinux wheels - #35

Open
alejp1998 wants to merge 10 commits into
mainfrom
fix/cgal-sigsegv-coordinate-snapping
Open

alejp1998 wants to merge 10 commits into
mainfrom
fix/cgal-sigsegv-coordinate-snapping

Conversation

@alejp1998

Copy link
Copy Markdown
Collaborator

What

  • Coverage fix: snapped coordinates are deduplicated and cells that collapse under the 10 cm snap grid (e.g. thin BCD slivers) are dropped or rejected with a clear ValueError instead of leaking CGAL's hard Outer polygon is not counterclockwise oriented error.
  • Regression tests: real-world farmland_2 (vt_kentland_farm) sliver case plus snap-degenerate unit tests; PROJ golden values replaced by runtime pyproj expectations.
  • Release packaging: cibuildwheel builds cp310–cp314 manylinux wheels (EPEL 8 CGAL-devel), tests each wheel in-container, publishes wheels + sdist to PyPI via trusted publishing, and creates a sigstore-signed GitHub release.

Notes

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).

CGAL's exact-arithmetic decomposition and sweep-pattern code can produce
a SIGSEGV when fed coordinates that carry floating-point noise introduced
by pyproj map-projection (e.g. a WGS-84 polygon projected to UTM becomes
a slightly non-rectangular quadrilateral with sub-millimetre jitter on
each vertex).

Add a _snap_polygon() helper that rounds every vertex to 1 decimal place
(10 cm precision in a metric CRS such as UTM).  Apply it to:

  - the boundary polygon at the start of decompose_polygon()
  - each obstacle polygon that does not intersect the boundary
  - the input polygon at the start of generate_sweep_pattern()

This removes the floating-point noise before it reaches CGAL while
introducing at most 5 cm of positional error — negligible for any
practical drone coverage-planning use case.

Discovered and worked around in kasperg3/trajgenpy consumer code:
the workaround isolated all trajgenpy C++ calls in a child process so
that a SIGSEGV would only crash the child, not the parent service.  This
commit fixes the root cause so the subprocess isolation is no longer
necessary.
Decomposed coverage cells whose vertices collapse onto the same 10 cm
snapping grid point (e.g. the ~1 cm-wide sliver produced for the
vt_kentland_farm farmland_2 polygon) degenerated into zero-area rings
that CGAL rejected with "Outer polygon is not counterclockwise oriented".

- _snap_polygon merges vertices that round to the same grid point and
  returns an empty polygon when the ring degenerates
- decompose_polygon drops cells that degenerate after snapping
- generate_sweep_pattern raises a descriptive ValueError instead of
  leaking the CGAL error
- GeoData.buffer passes cap_style/join_style as keywords (shapely
  deprecation)
- tests: PROJ-version-independent CRS expectations plus a regression
  test with the real farmland_2 ring
- CI: build cp310-cp314 manylinux wheels with cibuildwheel and publish
  them together with the sdist
- version bump to 0.3.1

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
Keep main's CRS refactor (shapely.ops.transform), docstrings, and uv-based test workflow. Keep the degenerate-cell fix: snap dedupe, degenerate filtering, and clear ValueError guards. Keep the cibuildwheel release pipeline for prebuilt manylinux wheels. Version 0.3.1.

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
…ilds

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
cibuildwheel installs the wheel without dependencies, so pytest could not import geojson/shapely/etc. Install requirements.txt via CIBW_BEFORE_TEST before running the suite.

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
Copilot AI lite review requested due to automatic review settings September 10, 2026 12:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The release workflow YAML appears to have invalid steps indentation (would prevent releases) and _snap_polygon’s ring snapping still fails on 3D coordinate tuples.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens TrajGenPy’s coverage-planning pipeline against snap-induced degenerate polygons (avoiding CGAL hard failures) and adds automated packaging to publish prebuilt manylinux wheels alongside an sdist.

Changes:

  • Improve coordinate snapping to deduplicate collapsed vertices and drop/raise on degenerate polygons before calling CGAL.
  • Add regression/unit tests for snap-degenerate cases and make CRS conversion golden values computed via runtime pyproj.
  • Update packaging/CI metadata (version bump, Python classifiers, ruff config) and introduce a wheel+sdist release workflow using cibuildwheel and trusted publishing.
File summaries
File Description
trajgenpy/Geometries.py Adds degenerate-geometry guards and improves snapping to prevent CGAL sweep/decomposition failures.
trajgenpy/__init__.py Minor import formatting / __all__ ordering tweak.
tests/test_geometries.py Adds regression tests for snap-collapse cases and makes projection expectations runtime-derived.
pyproject.toml Bumps version to 0.3.1, updates Python classifiers, and adjusts ruff configuration.
.github/workflows/test.yml Expands CI Python matrix and makes ruff linting non-optional.
.github/workflows/release.yml Adds wheel+sdist build/publish pipeline with cibuildwheel, PyPI trusted publishing, and Sigstore signing.
Review details

Suppressed comments (3)

.github/workflows/release.yml:47

  • The steps: list items under build-sdist are not indented under the steps key, which will make this job invalid YAML.
    steps:
    - uses: actions/checkout@v4
    - name: Set up Python
      uses: actions/setup-python@v5
      with:
        python-version: "3.11"
    - name: Build sdist

.github/workflows/release.yml:75

  • The publish-to-pypi job has the same indentation issue: sequence items are aligned with steps: instead of being nested under it, which breaks the workflow syntax.
    steps:
    - name: Download wheels
      uses: actions/download-artifact@v4
      with:
        name: wheels
        path: dist
    - name: Download sdist
      uses: actions/download-artifact@v4
      with:

.github/workflows/release.yml:96

  • The github-release job steps: block is also mis-indented; items need to be nested under steps: or the job won't run.
    steps:
    - name: Download wheels
      uses: actions/download-artifact@v4
      with:
        name: wheels
        path: dist/
    - name: Download sdist
      uses: actions/download-artifact@v4
      with:
  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/release.yml Outdated
Comment thread trajgenpy/Geometries.py
Comment on lines +600 to +604
def _snap_ring(coords):
rounded = [
(round(x, precision), round(y, precision))
for x, y in coords
]
Comment thread trajgenpy/Geometries.py Outdated
Comment on lines +583 to +585
Vertices that round to the same grid point are merged (consecutive
duplicates dropped). If the exterior ring degenerates below three distinct
points, or the snapped polygon becomes invalid or zero-area, an empty
CPython 3.14 interpreters require glibc >= 2.34, so the cp314 wheel must use the AlmaLinux 9 image while older Pythons stay on manylinux_2_28. Also make PyPI publish skip already-uploaded files so the same tag can be re-run.

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
- _snap_ring indexes coordinates instead of unpacking, so 3D rings (with Z) are handled by dropping Z like the CRS reprojection path\n- Clarify that snap merging drops consecutive duplicates\n- Use conventional indentation for workflow step lists

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
@alejp1998

Copy link
Copy Markdown
Collaborator Author

Addressed the review threads:

  1. Workflow indentation (High): The steps: sequence style is valid YAML — GitHub parsed and executed this workflow end-to-end several times (including the PyPI publish job). Still, I re-indented the step lists to the conventional style in the latest commit.
  2. 3D ring snapping (Medium): Fixed — _snap_ring now indexes coordinate pairs instead of unpacking, so Z-carrying rings are snapped by dropping Z, consistent with the CRS reprojection path. Tested (16/16 pass, lint clean).
  3. Docstring wording (Low): Reworded to "merged by dropping consecutive duplicates".

@copilot could you re-review the changes? (The release workflow check shown in progress is the tag-triggered run publishing the cp314 wheel.)

Copilot AI commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Addressed the review threads:

  1. Workflow indentation (High): The steps: sequence style is valid YAML — GitHub parsed and executed this workflow end-to-end several times (including the PyPI p...

Re-reviewed: no further code changes needed from my side. 1b33993 addresses the 3D snapping/docstring/workflow-style review points, the focused geometry regressions pass locally, and the branch pytest workflow for that commit completed successfully.

v2.23.4 has no cp314 build identifier (its table ends at cp313), so the cp314 wheel was silently dropped. v4.2.1 builds cp310-cp314; the manylinux_2_34 override for cp314 is kept.

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
0.3.1 publishes cp310-cp314 manylinux wheels, so plain pip install needs no CGAL/pybind11 on Linux x86_64. Keep the apt/brew instructions for the source-distribution fallback and correct the misleading macOS/Windows note.

AI-assisted by GitHub Copilot (DeepSeek V4 Pro).
@kasperg3
kasperg3 self-requested a review September 11, 2026 11:47

@kasperg3 kasperg3 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 🥇

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.

4 participants