You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(analysis): Clip and the overlay tools say which mask features they leave out - #1733
Clip, Erase, Intersect, Identity, Union, Symmetrical Difference, Update and Split skipped features of the mask or overlay layer that have no polygon (no geometry, a point or line in the polygon layer, an empty polygon) and didn't say so. Since #1572, Minimum Bounding Geometry writes flat hulls as points and lines in a polygon layer, so this is easier to hit. The bot review on #1572 caught it after the merge.
Each tool now counts those features and says how many.
Clip also counts a mask whose union has no area (it already dropped those). Union and Symmetrical Difference count an overlay feature whose parts union to nothing, since its row used to vanish.
When no feature of that layer has a polygon, Clip, Intersect and Split stop with an error, as Dissolve does, since their result would be empty. The others return the input with the notice.
A mask or overlay row selected twice is counted once.
Split checks its field before it reads the split polygons.
Update no longer writes an update polygon with no rings; it's counted with the rest.
What I left alone:
Real polygons work as on main. I tried leaving out every overlay polygon with no area, the way Clip does, but it changed results for points and lines on flat rings, could drop very thin polygons that the union rounds away, and doubled the setup work. So a single flat ring is still a zone in the overlay tools.
Input features without geometry, and input rows selected twice, are older behavior on the input side. I can open issues for them.
Testing
cargo xtask ci: fmt, clippy and layers are clean. All workspace tests pass except popupmedia::tests::decoding_keeps_the_caps, which also fails on main on macOS and Windows (main's CI for f62c795).
The browser build passes.
A new test runs all eight tools with a mask layer holding points, lines, empty polygons, a feature without geometry and flat rings. It checks the output matches the clean run, the counts and wording, that a bowtie gives no notice, that a row selected twice counts once, and when it's an error versus a notice.
Polygon tools now omit selected features without usable polygon geometry and report how many were excluded. Clip also excludes masks that cover no area; Union and Symmetrical Difference exclude overlays whose combined parts cover no area.
Clip, Intersect, and Split now return an error when no usable polygon features remain. Duplicate selections are counted once, and Update reports skipped features without polygons.
Split checks the selected field before processing polygons.
Documentation
Clarified how zero-area hulls are handled by analysis tools and Shapefile export.
…er nothing
Clip, Erase, Intersect, Identity, Union, Symmetrical Difference, Update
and Split skipped a feature of the mask or overlay layer that has no
polygon (no geometry, a point or line in the polygon layer, an empty
polygon) without saying so. Minimum Bounding Geometry now writes flat
hulls as points and lines in a polygon layer, so these are easier to
hit. Each tool now counts them and gives one notice; Clip also counts a
polygon with no area, and the overlay tools a feature whose parts union
to nothing. Empty update and overlay polygons are no longer written out
by Update, Union and Symmetrical Difference.
Minimum Bounding Geometry with Group: All looks at Cancel per feature,
and both group modes join the long tools in the cancel test.
…sk is an error
Review follow-up. overlay_zones keeps every feature with a polygon as a
zone again, so Update still writes an update polygon whose parts have no
area, as it is. Union and Symmetrical Difference, where such a feature's
row would vanish, count it in a notice of its own.
Clip and the overlay tools stop with an error when every feature of the
mask or overlay layer covers nothing, as Dissolve does, rather than give
an empty layer or the input unchanged. A row selected twice is counted
once, and Minimum Bounding Geometry's notice says the overlay tools leave
its points and lines out of a mask or overlay layer only.
…Clip does
Review follow-up. An erase, overlay or split polygon with no area, one
part or several, covers nothing, yet Erase, Intersect, Identity and Split
kept it as a zone without a word, and Union and Symmetrical Difference
wrote a single one out. overlay_zones now leaves these out and counts
them, with a notice of their own, and counts them toward the error when
every feature is left out. Update keeps them, writing its update
polygons as they are.
Minimum Bounding Geometry uses each_once for its rows, and the cancel
test names each tool with its parameters, so the two convexHull runs are
told apart.
…hing would come out
Review follow-up. The overlay tools judged a polygon to have no area by
its shoelace area, which is zero for a bowtie that covers ground, so
they left such a polygon out where Clip keeps it. They now keep every
polygon as a zone, as before; Union and Symmetrical Difference count an
overlay feature whose parts union to nothing, as nothing of it would be
written, and Clip counts a mask whose union is empty, each in a notice
of its own, apart from the features with no polygon.
Leaving out every feature is an error only for Clip, Intersect and
Split, whose result would be empty; Erase, Identity, Union, Symmetrical
Difference and Update give the input, with the notice, as before. The
error says how many of each kind.
Review follow-up. A polygon has no area when its union is empty, as
Clip judges its masks: rings on a line, one part or several, but not a
bowtie, which covers its two triangles. overlay_zones now leaves such
features out of Erase, Intersect, Identity, Union, Symmetrical
Difference and Split, counted in a notice of their own and toward the
error for Intersect and Split, instead of Union and Symmetrical
Difference alone catching some of them later. Update keeps them, as it
writes its update polygons as they are. A one-part polygon is unioned
once more to judge it.
…ncel for hulls moves out
Narrow the change to what was silent. Judging which overlay polygons
have no area changed results for points and lines on flat rings, could
drop very thin polygons the union rounds away, and doubled the overlay
tools' setup work. Erase, Intersect, Identity, Union, Symmetrical
Difference, Update and Split keep every polygon as a zone, as on main,
and add only the notices: features with no polygon, and in Union and
Symmetrical Difference a feature whose parts union to nothing, which
would otherwise vanish. Clip keeps counting masks whose union is empty.
Leaving out every feature is an error only for Intersect and Split, and
for Clip.
Minimum Bounding Geometry's Cancel check for Group: All goes to a pull
request of its own.
Review follow-up. Split read its split polygons, and could stop with
"none covers anything", before it checked the field parameter, so a
wrong field was hidden behind that error. The user guide now says the
error comes when no feature of the layer has a polygon (for Clip, when
none covers any area), which is when it does.
No actionable comments were generated in the recent review.
Walkthrough
Polygon overlay tools now omit selected features without usable polygon area, report omission counts, and return errors for specified operations when no usable masks or overlays remain. Split validates its field before reading split polygons. Tests and the user guide cover these behaviors.
Changes: Polygon overlay handling
Layer / File(s)
Summary
Shared geometry filtering and Clip crates/analysis/src/tools.rs, docs/user-guide/analysis.md
Clip deduplicates selected masks, excludes masks without polygon geometry or area, and reports exclusions. Dissolve uses the shared polygon check. The Minimum Bounding Geometry notice describes flat-hull handling by Dissolve, Clip, and overlay tools.
Overlay feature filtering and reporting crates/analysis/src/tools.rs, docs/user-guide/analysis.md
Erase and overlay tools exclude features without polygon geometry and report their counts. Intersect errors when no usable overlay remains. Union and Symmetrical Difference also exclude overlay features whose union has no area.
Update and Split validation crates/analysis/src/tools.rs, docs/user-guide/analysis.md
Update and Split report features without polygon geometry. Split validates its field before reading split polygons and errors when no usable split polygons remain. Tests and guide text cover the updated behavior.
opengeos/RustGIS#1033 — This change reports Clip masks whose union has no area, but does not correct the coordinate-conversion latitude clamp.
opengeos/RustGIS#1354 — This change reports Clip masks whose union has no area, but does not determine or repair polar ring interpretation.
Possibly related PRs
opengeos/RustGIS#1103 — This change modifies how Symmetrical Difference, Update, and Split handle unusable selected features.
opengeos/RustGIS#982 — This change adds exclusion reporting to overlay behavior introduced by that PR.
opengeos/RustGIS#1572 — This change adds exclusion counts and notices for flat hulls used by Clip and overlay tools.
Suggested reviewers:giswqs
Merge Risk: Minimal · up to 11ece
The reviewed behavior and guide text are consistent; no identified issue requires a change before merging.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly describes the main change: Clip and overlay tools now report which mask features they omit.
Description check
The description includes a clear summary, testing details, the required UI evidence statement, and a related-issue reference.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 1 unsupported, 1 too large.)
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the polygons with care, Skips empty masks that offer no area. Counts each lost shape, then hops along, Split fields get checked before the polygons’ song. The maps return, clear as a springtime dawn.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: bc75851d-d667-4d8e-b3cd-ac0b09f94f4a
Commits
Reviewing files that changed from the base of the PR and between 7cc9bb6 and 11ece6d.
Files selected for processing: 2
crates/analysis/src/tools.rs
docs/user-guide/analysis.md
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Silent drop for Intersect, Identity, Erase and Split.overlay_zones only counts features with no polygon. An overlay feature with several parts whose union is empty (mask_of gives an empty MultiPolygon) is kept as a zone, but part returns None for it, so its row never reaches the output. That is the same loss the PR counts for Union and Symmetrical Difference, and the new test codifies the silence: the _ => vec![note(name, 5, NO_POLYGON)] arm at tools.rs:7047 expects no notice for mask 16 (two flat rings) under Intersect and Identity.
Suggested fix: count the empty-union zones in overlay_zones (e.g. left.no_area when mask_of(poly).0.is_empty()) and drop them from zones, then report them through left.notes for every tool that uses it, except Update, which writes its zone geometry as-is. Then update the expected notes in the test for those tools. Confidence: high on the behavior, medium that every tool needs the notice (Erase and Split lose nothing visible, but the rule is still "say how many").
Intersect, Identity, Erase and Split silently drop an overlay feature whose parts union to nothing. overlay_zones counts only features with no polygon, so the empty-union zone is kept, part yields nothing, and the row vanishes with no notice. Union and Symmetrical Difference do count this case, and the test at tools.rs:7047 expects silence for the other tools. Posted inline. Confidence: high on the behavior, medium that Erase and Split need the notice, since they lose nothing visible.
Security
None found. Non-test code adds no unwrap, expect, panic or unsafe, and the new loops check for Cancel.
Performance
None found. each_once allocates a set only when the selection is unsorted, and polygon_of runs once per row, as before.
Quality (nits, left for the release quality pass)
nothing_containing wording reads "1 a polygon with no area" in the mixed-cause error (tools.rs:1296, asserted at tools.rs:7096).
The analysis.md paragraph about Dissolve and Shapefile export has ragged line wrapping (around lines 484–490).
AGENTS.md
Not violated beyond the silent-drop finding above, which breaks the "Never silent" rule.
Not re-raised: prior review threads (none exist on this PR).
Confidence overall: medium-high. I could not build or run the tests (no Bash), so the findings rest on reading the code.
Change size
Compared: the whole PR against main (merge base f62c795 to PR head 11ece6d).
GitHub Pages takes a minute or two to publish a new commit.
This branch has not been deployed
No deployments
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
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Clip, Erase, Intersect, Identity, Union, Symmetrical Difference, Update and Split skipped features of the mask or overlay layer that have no polygon (no geometry, a point or line in the polygon layer, an empty polygon) and didn't say so. Since #1572, Minimum Bounding Geometry writes flat hulls as points and lines in a polygon layer, so this is easier to hit. The bot review on #1572 caught it after the merge.
What I left alone:
Testing
cargo xtask ci: fmt, clippy and layers are clean. All workspace tests pass exceptpopupmedia::tests::decoding_keeps_the_caps, which also fails on main on macOS and Windows (main's CI for f62c795).UI evidence
No visible UI change.
Related issue
Follow-up to #1572.
Summary by CodeRabbit