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): Simplify counts its work toward Cancel across a feature's parts, and says its past-pole count is from before simplifying - #1883
Simplify looked at Cancel every 65,536 distances within a run, but each line part and polygon ring started its own count. A feature made of very many short parts (a million three-vertex parts, say) never reached the count, so it ran to its end after Cancel. Now the work counts across the whole feature, on the crate's control::Pace, the same pacing other tools use inside a long item: each distance, each ring's two anchor scans, and each vertex's set-up. Set-up (its latitude and scale) counts as 4 distances; I timed it at about 3 to 8. Once canceled, the rest of the feature's parts aren't walked. Inside one long scan this looks less often than before (every PACE_WORK units, a few milliseconds, rather than every 65,536 distances), the same as other tools.
A canceled feature is given back as it was, not part-simplified. In a tool run the run then stops as canceled anyway, so this just keeps a half-done feature from ever being returned.
Not in this PR: the mobile map package and the globe locator call simplify_geometry outside a tool run, where no Cancel is installed, so they don't look at Cancel at all. That's with the rest of those callers' gaps in #1811.
The past-pole notice counts the vertices found past a pole, before simplifying, and some of them may then be simplified away; it now says so ("taken as that pole before simplifying, so some may then go"). Counting only the ones that survive isn't possible from the output, since once taken to the pole they're indistinguishable from vertices that were at 90° to begin with.
Testing
simplify_looks_at_cancel_across_a_features_parts: with the work counted to just short of a look at Cancel, a three-vertex part is simplified and every part after it is left as it is; a ring is simplified in full and the next one's anchor scans reach the look, so it's left as it is; and a closed triangle's own work counts, so a polygon of a million small holes is looked at. It fails on main, where each part starts its own count, and fails again with the ring's count taken out.
simplify_coords_looks_at_cancel_inside_a_feature: a canceled 200,000-vertex line stops partway through its first scan and keeps its vertices, without calling it a work cap.
simplify_gives_back_a_canceled_feature_as_it_was: a 2.2-million-vertex ring, canceled, comes back unchanged.
cargo xtask ci and cargo build --target wasm32-unknown-unknown -p rustgis-web pass, apart from a_publication_footer_lines_up_with_its_legend, which fails the same way on main (ce27286's CI).
Simplification now applies cancellation consistently across all parts of a feature, including large line runs and polygon rings.
If cancellation occurs, the feature is returned unchanged rather than partially simplified. Reaching the work cap remains a separate outcome, preserving vertices in unfinished spans.
…e's parts, and says its past-pole count is from before simplifying
Simplify looked at Cancel every 65,536 distances within a run, but each line
part and polygon ring started its own count, so a feature of very many short
parts never looked at all and ran to its end after Cancel. Its work now
counts across the feature's parts and rings (SimplifyRun, on the crate's
control::Pace): each vertex set up, each distance, each ring's anchor scans.
Once canceled, the rest of the feature is left alone, and the feature is
given back as it was rather than part-simplified, for callers outside a tool
run (a tool run stops as canceled).
The past-pole notice counts the vertices found past a pole, before
simplifying, some of which may then go; it now says so.
No actionable comments were generated in the recent review.
Walkthrough
Simplify now shares cancellation pacing and work-cap state across a feature’s parts and rings. If cancellation occurs, it stops further simplification and returns the original geometry. The changes also update Simplify documentation, notice text, and tests.
Changes: Simplify cancellation
Layer / File(s)
Summary
Shared cancellation state and simplification scans crates/analysis/src/control.rs, crates/analysis/src/tools.rs
PACE_WORK is now visible within the crate. Simplify uses a per-feature SimplifyRun to track cancellation pace and capped status across vertex setup, span scans, and ring scans.
Geometry cancellation and result handling crates/analysis/src/tools.rs
Line and polygon processing stops after cancellation and returns the original geometry. Documentation describes cancellation checks across feature work, and the Simplify notice describes latitude clamping before simplification.
Cancellation and simplification tests crates/analysis/src/tools.rs
Tests cover cancellation within spans and across line parts and polygon rings, unchanged feature results, work caps, and ring simplification cases.
Issue 1811 discusses Simplify reporting for past-pole handling and callers. This PR updates the Simplify notice and cancellation result, but does not change the callers mentioned in the issue.
opengeos/RustGIS#1418: Defines the pacing model and work threshold used by Simplify.
Suggested reviewers:giswqs
Merge Risk: Minimal · up to 5db2f
Simplify now counts cancellation work across a feature's parts and rings, and returns a canceled feature unchanged. No merge-blocking risk is evident from the supplied information.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: Simplify now counts its cancellation work across a feature's parts, and the past-pole notice wording is also updated. The title matches the raw summary and PR objectiv…
Description check
The description follows the template. It has Summary, Testing, and Related issue sections. Its UI evidence section states 'No visible UI change', which the template allows when there is no visible cha…
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: Title check
Explanation
The title names the main change: Simplify now counts its cancellation work across a feature's parts, and the past-pole notice wording is also updated. The title matches the raw summary and PR objectives. It is long (135 characters) and combines two changes, but it is related to the changeset and is not vague or off-topic.
Full details: Description check
Explanation
The description follows the template. It has Summary, Testing, and Related issue sections. Its UI evidence section states 'No visible UI change', which the template allows when there is no visible change. The Summary explains the change and the reason for it. The Testing section lists new tests and the CI results, including one known failing test that also fails on main. The Related issue section links to #1033 and #1808 and to the follow-up #1811.
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 each vertex in the row, And counts the rings before they go. If cancellation calls, the work will cease, The shape stays whole, and rests in peace. Then hops away through clover green.
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: 6d5b1b8c-fc0b-465a-92b7-a11ed4ae9757
Commits
Reviewing files that changed from the base of the PR and between dd50dcc and 5db2f1b.
Files selected for processing: 2
crates/analysis/src/control.rs
crates/analysis/src/tools.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
messages.push(format!("Simplify: {past_pole} vertex(es) with a latitude past a pole found (taken as that pole)"));
messages.push(format!("Simplify: {past_pole} vertex(es) with a latitude past a pole found (taken as that pole before simplifying, so some may then go)"));
The reason will be displayed to describe this comment to others. Learn more.
This line is over the repo's max_width = 140 (rustfmt.toml), so cargo fmt --check, which cargo xtask ci runs, will reflow it and fail. Same shape as the notices above it:
Suggested change
messages.push(format!("Simplify: {past_pole} vertex(es) with a latitude past a pole found (taken as that pole before simplifying, so some may then go)"));
messages.push(format!(
"Simplify: {past_pole} vertex(es) with a latitude past a pole found (taken as that pole before simplifying, so some may then go)"
));
Confidence: high (about 85%); it's a line-length check, not a logic issue.
The reason will be displayed to describe this comment to others. Learn more.
Related to the short-part count set at tools.rs:4130 (not in the diff, so commenting here): that count comes from the input g, not the result. A feature canceled partway is given back as it was (simplify_checked returns unchanged), so its short parts are still in the output, yet they're reported here as left out. In a tool run this can't surface today, since run_under turns a canceled run into an error before the notice is shown. Still, the count should follow what was output. Gating the count on changed there (if let Geometry::Line(parts) = g && changed && parts.iter().any(|p| p.len() >= 2)) matches the output, because a line whose parts are all short is kept, and the existing any(len >= 2) check already covers that.
Short-part count is taken from the input rather than the output. tools.rs:4130 counts short line parts from g, but a feature canceled partway is returned unchanged, so those parts are still in the output yet reported as "left out". Unreachable in a tool run today, since run_under turns a canceled run into an error first. Confidence: medium (~60%), low impact. Posted inline at tools.rs:4172, because line 4130 is outside the diff.
Security
None found in the changed lines.
Performance
The innermost distance loop now calls run.stop(1) per distance. That adds a Cell take/set and a canceled read/write per iteration, where the old code used a plain counter. Probably a small cost; not measured. Confidence: low (~40%). Batching per scan would remove it.
Quality
Line 4175 is 158 characters, over the repo's max_width = 140. cargo fmt --check (run by cargo xtask ci) will fail. Confidence: high (~85%). Posted inline with a committable suggestion.
AGENTS.md
No unwrap, expect, unsafe, or indexing added in non-test code. Cancel is checked inside the long loops, and the new notice text follows the existing pattern.
The new cancel path has no Session::notify call for the given-back feature. This is acceptable because a canceled tool run errors, so there is no silent result.
Simplify's long-step doc comment on simplify_on already says that the pre-scan passes (for_each_coord, the plane round-trip, the vertex setup) are not counted, which satisfies the "say it in the doc comment" rule.
Checked and found sound
Pace is sticky once canceled. try_map_by checks control::cancelled() before each feature, and run_under errors when the flag is set after the work, so a canceled run never returns a partly simplified result.
The canceled guards in simplify_ring, the polygon loop, and the line loop all discard partial output.
The Cell<Pace> take/set pattern is correct, and Pace derives Default.
PACE_WORK visibility change is needed by the tests and has no other effect.
Pace granularity went from every 65,536 distances to every 4M units. The PR says so, and at a few ns per distance this is a few ms, which is within the repo's cancel standard.
Prior reviews:prior-reviews.json had no threads, so none were left alone.
Not verified: I did not run cargo test, fmt, or clippy. The test arithmetic (pace budgets of 14, 121, and 8) was checked by reading only.
Change size
Compared: the whole PR against main (merge base ce27286 to PR head 5db2f1b).
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
A follow-up to #1808, from its last review round.
Simplify looked at Cancel every 65,536 distances within a run, but each line part and polygon ring started its own count. A feature made of very many short parts (a million three-vertex parts, say) never reached the count, so it ran to its end after Cancel. Now the work counts across the whole feature, on the crate's
control::Pace, the same pacing other tools use inside a long item: each distance, each ring's two anchor scans, and each vertex's set-up. Set-up (its latitude and scale) counts as 4 distances; I timed it at about 3 to 8. Once canceled, the rest of the feature's parts aren't walked. Inside one long scan this looks less often than before (everyPACE_WORKunits, a few milliseconds, rather than every 65,536 distances), the same as other tools.A canceled feature is given back as it was, not part-simplified. In a tool run the run then stops as canceled anyway, so this just keeps a half-done feature from ever being returned.
Not in this PR: the mobile map package and the globe locator call
simplify_geometryoutside a tool run, where no Cancel is installed, so they don't look at Cancel at all. That's with the rest of those callers' gaps in #1811.The past-pole notice counts the vertices found past a pole, before simplifying, and some of them may then be simplified away; it now says so ("taken as that pole before simplifying, so some may then go"). Counting only the ones that survive isn't possible from the output, since once taken to the pole they're indistinguishable from vertices that were at 90° to begin with.
Testing
simplify_looks_at_cancel_across_a_features_parts: with the work counted to just short of a look at Cancel, a three-vertex part is simplified and every part after it is left as it is; a ring is simplified in full and the next one's anchor scans reach the look, so it's left as it is; and a closed triangle's own work counts, so a polygon of a million small holes is looked at. It fails on main, where each part starts its own count, and fails again with the ring's count taken out.simplify_coords_looks_at_cancel_inside_a_feature: a canceled 200,000-vertex line stops partway through its first scan and keeps its vertices, without calling it a work cap.simplify_gives_back_a_canceled_feature_as_it_was: a 2.2-million-vertex ring, canceled, comes back unchanged.cargo xtask ciandcargo build --target wasm32-unknown-unknown -p rustgis-webpass, apart froma_publication_footer_lines_up_with_its_legend, which fails the same way on main (ce27286's CI).UI evidence
No visible UI change.
Related issue
Part of #1033. Follows #1808; the callers' gaps are #1811.
Summary by CodeRabbit