fix: Enhance QGCMapPolygon center logic and vertex translation#13883
fix: Enhance QGCMapPolygon center logic and vertex translation#13883Eureak-zon wants to merge 2 commits intomavlink:masterfrom
Conversation
Improved the geometric accuracy and performance of polygon manipulations: 1. Corrected center calculation by implementing an area-based centroid algorithm. This prevents the center from being "weighted" toward locations where multiple vertices overlap. 2. Optimized 'setCenter' to handle vertex translation more efficiently during drags by suppressing redundant signals and using deferred updates. 3. Added robustness for degenerate polygons (zero area) by providing a fallback to the arithmetic mean.
Build ResultsPlatform Status
All builds passed. Test Resultslinux_gcc_64: 479 passed, 0 skipped Total: 479 passed, 0 skipped Code CoverageCoverage: 100.0% No baseline available for comparison Artifact Sizes
No baseline available for comparisonUpdated: 2026-01-29 08:25:04 UTC • Triggered by: MacOS |
Build ResultsPlatform Status
Some builds still in progress. Test Resultslinux_gcc_64: 479 passed, 0 skipped Total: 479 passed, 0 skipped Code CoverageCoverage: 100.0% No baseline available for comparison Artifact Sizes
No baseline available for comparisonUpdated: 2026-01-22 07:08:13 UTC • Triggered by: Windows |
|
I'm not sure why CoPilot won't review this for me. Here is a Claude Sonnet review on the _updateCenter method: I'll review the _updateCenter method for bugs and performance issues. Read QGCMapPolygon.cc, lines 340 to 375 Review of _updateCenter Method Bugs:
Performance Issues:
Recommendations:
Medium Priority:
@Eureak-zon Can you look through this and see what is appropriate. Also can you add a comment stating that this code is the shoelace algorithm. |
|
I'm not seeing the pathChanged problem you are fixing here. I added logging to the existing code and when I drag the center I get this pattern: This shows a single pathChanged signaled per center update. Do I need to do something else to reproduce the problem? Here's what I'm testing with: #13924 |
|
@DonLakeFlyer This submission addresses the issue that when dragging the vertices of a polygon, the center point is not actually in its original position, while dragging the center point itself works without any problem. When dragging the vertices of a polygon, for instance, if multiple vertices overlap, the original algorithm performs weighted calculations on these overlapping points, which leads to the problem. |
Surveyor's formulaThe following snippet is the actual implementation of the formula: for (int i = 0; i < n; i++) {
int j = (i + 1) % n; // Gets the index of the next vertex to close the loop
// Core Shoelace calculation: x_i * y_{i+1} - x_{i+1} * y_i
double crossProduct = (polygonF[i].x() * polygonF[j].y()) - (polygonF[j].x() * polygonF[i].y());
area += crossProduct; // Summing the cross-product terms
// Calculating the intermediate values for the Centroid coordinates
centroid.rx() += (polygonF[i].x() + polygonF[j].x()) * crossProduct;
centroid.ry() += (polygonF[i].y() + polygonF[j].y()) * crossProduct;
}
area *= 0.5; // Final step of the Shoelace formula: multiplying by 1/2Mathematical Correlation
In the code, the
This corresponds to the line: |
There was a problem hiding this comment.
Pull request overview
This PR updates QGCMapPolygon’s center handling to compute a geometric centroid (shoelace formula) instead of an arithmetic mean, and reduces UI churn when translating vertices during center moves.
Changes:
- Replaced mean-based center computation with area-based centroid calculation, with a fallback for near-zero-area polygons.
- Deferred
centerChangedemission insetCenterto reduce redundant updates during interactive moves. - Added
_deferredCenterChangedstate to support deferred center signaling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/QmlControls/QGCMapPolygon.h |
Adds _deferredCenterChanged to throttle/batch centerChanged emissions. |
src/QmlControls/QGCMapPolygon.cc |
Implements area-based centroid logic in _updateCenter and defers centerChanged in setCenter. |
| void QGCMapPolygon::_updateCenter(void) { | ||
| if (!_ignoreCenterUpdates && _polygonPath.count() > 2) { | ||
| QPolygonF polygonF = _toPolygonF(); |
There was a problem hiding this comment.
_updateCenter now returns early unless _polygonPath.count() > 2, which means the polygon center will no longer be updated (or invalidated) when the path is cleared or reduced to 1–2 vertices. This can leave a stale center value in QML (e.g. the center drag handle remains at the previous location during tracing/after clear). Consider updating the center for 1–2 vertices (mean of available points) and explicitly resetting it when the path becomes empty, while still keeping the area-based centroid for count() >= 3.
| { | ||
| if (!_ignoreCenterUpdates) { | ||
| QGeoCoordinate center; | ||
| void QGCMapPolygon::_updateCenter(void) { |
There was a problem hiding this comment.
Brace/whitespace style in the updated function definitions is inconsistent with the rest of this file (e.g. void QGCMapPolygon::clear(void) uses the opening brace on the next line at src/QmlControls/QGCMapPolygon.cc:70-71). For consistency/readability, align _updateCenter (and setCenter below) with the prevailing brace placement and spacing used throughout this compilation unit.
| void QGCMapPolygon::_updateCenter(void) { | |
| void QGCMapPolygon::_updateCenter(void) | |
| { |
|
I'm not asking about the algorithm. In you modified code you made changes to the signalling which I asked about above. I can't reproduce why you had to change that. |
Summary
Improved the geometric accuracy and performance of polygon manipulations:
Corrected Center Calculation: Implemented an area-based centroid algorithm to prevent the center from being "weighted" toward locations where multiple vertices overlap.
Optimized setCenter: Enhanced efficiency during vertex translation by suppressing redundant signals and utilizing deferred updates.
Added Robustness: Provided a fallback to the arithmetic mean for degenerate polygons (zero area) to ensure stability.
Description
Problem
In the original QGCMapPolygon implementation, the center was calculated as the arithmetic mean of all vertex coordinates. When a polygon contains overlapping vertices (e.g., two points in a 5-vertex polygon are at the same location), that specific location gains 40% of the weight, pulling the center away from the true geometric heart of the shape. Additionally, updating each vertex individually during a center drag triggered multiple recursive path updates, leading to UI lag.
Solution
Geometric Centroid Algorithm: The _updateCenter function now uses the Shoelace formula (Surveyor's formula) to calculate the true centroid based on the polygon's area. This ensures the center remains mathematically accurate regardless of vertex density.
Signal Suppression: Introduced _ignoreCenterUpdates and utilized QTimer::singleShot in setCenter to batch pathChanged signals. This prevents the UI from stuttering during heavy manipulation.
Fallback Logic: If the polygon area is zero (degenerate case), the system gracefully falls back to the mean-point calculation to maintain a valid coordinate.
Type of Change
Bug fix (Fixed center offset caused by overlapping vertices)
New feature
Refactoring (Improved performance and math logic)
Testing
Test Cases
Overlap Test: Created a square polygon and added a 5th vertex directly on top of an existing one. Verified the center remained at the geometric center of the square.
Drag Performance: Dragged the center of complex polygons (15+ vertices) in the plan view to ensure smooth rendering and no "lag spikes."
Degenerate Case: Reduced a polygon to 2 points or moved all points to one location to verify no "divide by zero" errors occurred.
Platforms Tested
Linux
Windows
macOS
Android
Checklist
My code follows the project's coding standards.
I have used _ignoreCenterUpdates to prevent signal loops.
The centroid logic handles both clockwise and counter-clockwise windings.