Repository navigation
fix(edit): preserve same-feature targets when snapping vertices - #818
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Snapping API and source dispatch crates/engine/src/cmd/design.rs |
snap_vertex_except validates a source vertex address and dispatches its feature to vertex-specific candidate processing. snap_except retains whole-feature exclusion. |
Vertex and edge candidates crates/engine/src/cmd/design/snap_vertex.rs, crates/engine/tests/vertex_snap_identity.rs |
Candidate processing excludes the addressed vertex and its incident edges. Tests cover geometry identity, projection, candidate priority, tolerance, and invalid addresses. |
Vertex-drag integration crates/ui-egui/src/vertexedit.rs, crates/ui-egui/tests/vertex_snapping.rs |
Existing vertex drags use vertex-specific exclusion; insertions retain whole-feature exclusion. UI tests cover previews, commits, cancellation, and editing state. |
Command-key scan
| Layer / File(s) | Summary |
|---|---|
Scanner file selection crates/engine/src/tests.rs |
The command-key test scanner skips directories and files without the .rs extension. Comments specify that the scanner checks declared parameter keys for each discovered command. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant VertexEditor
participant DesignSnap
participant ProjectedCache
participant SnapVertex
VertexEditor->>DesignSnap: request snapping with source vertex address
DesignSnap->>ProjectedCache: iterate projected target features
DesignSnap->>SnapVertex: consider addressed source geometry
SnapVertex-->>DesignSnap: return eligible candidates
DesignSnap-->>VertexEditor: return selected snap coordinate and kind
Possibly related issues
- opengeos/RustGIS#748: The issue requests interactive editing tests, including vertex editing. This PR adds UI integration tests for vertex dragging and snapping.
Suggested reviewers: giswqs
Merge Risk: 
34f05
-
No concrete merge-blocking risk remains in the reviewed changes.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "fix(edit): preserve same-feature targets when snapping vertices" matches the main change. The PR changes vertex-drag snapping to exclude only the moving VertexId and its incident edges, not… Description check The description follows the template. It has a Summary that states what changes and why, including the same-feature snapping problem and the exclusion rules. It has a Testing section that lists platfo… Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 5 files. (1 skipped: 1… 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 "fix(edit): preserve same-feature targets when snapping vertices" matches the main change. The PR changes vertex-drag snapping to exclude only the moving VertexId and its incident edges, not the whole feature, so other nodes and edges in the same feature stay snap targets. The title uses a conventional-commit style and is specific. It does not name the engine and UI files, which is expected.
Full details: Description check
-
Explanation
The description follows the template. It has a Summary that states what changes and why, including the same-feature snapping problem and the exclusion rules. It has a Testing section that lists platform results, named regression tests, clippy, wasm check, screenshot inspection, and known gaps such as the unclaimed full workspace CI and Linux x86_64 runtime. It has a Related issue section that refers to
#143and#428. The description is detailed, and the Related issue wording is a follow-up reference rather than a Closes/Part of line, which is a minor deviation from the template example.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 5 files. (1 skipped: 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 nudges nodes in line,
And leaves the other paths to shine.
The edges near its chosen place
Step back and let new targets grace.
One careful hop, one undo too,
Then carrots wait when edits are through.
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let closed = polygon && path.len() > 1 && path.first() == path.last(); | ||
| let count = path.len().saturating_sub(usize::from(closed)); | ||
| let skip = |index| { | ||
| address.part == excluded.part && address.ring == excluded.ring && excluded.index == if closed && index == count { 0 } else { index } |
There was a problem hiding this comment.
Low-confidence edge case: skip normalises the path index count to 0, but excluded.index is compared as given. If a caller addresses a polygon's stored closing copy (index == count) and Geometry::vertex/move_vertex accept that address as an alias of node 0, then excluded.index == count never matches. Nothing is excluded and the moving node snaps to itself. Normalise excluded the same way, for example by mapping excluded.index == count to 0 for a closed polygon ring. The tests only cover index 0 for polygons, so please add a case for the closing index. Ignore this if vertex() rejects that index.
Code reviewThe PR replaces whole-feature exclusion with exclusion of the moved node and its incident edges when a line or polygon vertex is dragged. I reviewed the diff and the changed files without building or running anything, so the new tests are unrun. I posted one inline comment. Bugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared: the whole PR against main (merge base fa96baa to PR head 2931a71).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
|
@yongyin-leon Can you fix the ci errir or enable the option for maintainer edit? |
Code reviewPR #818 makes vertex-drag snapping skip only the node being moved and its incident edges. It no longer skips the whole feature. I found no blocking issues. I could only read the code, so I did not build it or run the tests. Bugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared: the whole PR against main (merge base fa96baa to PR head 34f056f)
|
|
Thanks for flagging this! Fixed in 34f056f: the command-parameter audit was trying to read the new helper directory as a file. All CI checks are now passing. Thanks for reviewing and merging! |
Summary
Dragging a vertex currently excludes its entire feature from snapping. For example, an endpoint in one MultiLineString part cannot snap to another part of the same feature. Exclude the moving source
VertexIdand its incident edges instead, retaining other eligible nodes and edges in that feature.Testing
Previously completed on the exact frozen patch; these results were reused for submission:
The existing regression is
tests::modify_tools_snapping_topology_relationships_and_elevation. The new cases cover coincident but distinct nodes, parts/holes, polygon closure versus closed-line endpoints, projection cuts, switches/tolerance/priority, invalid addresses and read-only state. UI cases cover rotated dragging, read-only previews, single-step commit with undo/redo, Escape, insertion, disabled snapping and read-only editing.Also completed on Windows: affected engine/UI libraries and integration targets passed Clippy with
-D warnings;cargo check --target wasm32-unknown-unknown -p rustgis-webpassed. Three headless UI screenshots were inspected for snapped preview, committed geometry and insertion preview. Rustdoc, including private helpers, built with 11 pre-existing link warnings; each warning's source line matches the upstream baseline, and the new docs introduced none.Full workspace CI and Linux x86_64 runtime are not claimed. The Windows full-command hostile-parameter traversal was interrupted and is not counted as passed; only the named targeted regression above is counted.
Follow-up CI fix: the existing command-parameter source audit attempted to read the new
src/cmd/design/helper directory as a file. It now selects regular top-level.rsfiles before reading them, preserving all existing key checks and the minimum-command-count assertion. The affected audit test passed locally with an exact filter; Rust formatting and diff checks passed. The five node-snapping source/test files are unchanged, so the three-platform results above remain applicable.Related issue
Follow-up to the Edit Vertices tool from #143 / #428; this fixes same-feature snapping within the existing workflow.
Summary by CodeRabbit