Utilities: Add SHP Polyline #12850
Merged
Utilities: Add SHP Polyline #12850
Conversation
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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for loading SHP polyline files in addition to the existing KML support. Key changes include:
- Introduction of a ShapeFileType enum and a unified file type detection mechanism.
- Implementation of SHP polyline loading in both ShapeFileHelper and SHPFileHelper.
- UI update in QGCMapPolylineVisuals.qml to use a filter that accepts both KML and SHP files.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Utilities/Shape/ShapeFileHelper.h | Added ShapeFileType enum and new functions (_getShapeFileType, _fileIsSHP) |
| src/Utilities/Shape/ShapeFileHelper.cc | Updated file type handling and polyline loading logic via switch-case |
| src/Utilities/Shape/SHPFileHelper.h | Declared the new loadPolylineFromFile function |
| src/Utilities/Shape/SHPFileHelper.cc | Implemented loadPolylineFromFile with polyline-specific handling |
| src/FlightMap/MapItems/QGCMapPolylineVisuals.qml | Updated file filter to support both KML and SHP files |
Comment on lines
+230
to
+255
| if (!utmZone || !QGCGeo::convertUTMToGeo(shpObject->padfX[i], shpObject->padfY[i], utmZone, utmSouthernHemisphere, coord)) { | ||
| coord.setLatitude(shpObject->padfY[i]); | ||
| coord.setLongitude(shpObject->padfX[i]); | ||
| } |
There was a problem hiding this comment.
[nitpick] The use of '!utmZone' to check the validity of an integer variable may be unclear; adding a comment to clarify why zero is considered invalid would improve readability.
Suggested change
| if (!utmZone || !QGCGeo::convertUTMToGeo(shpObject->padfX[i], shpObject->padfY[i], utmZone, utmSouthernHemisphere, coord)) { | |
| coord.setLatitude(shpObject->padfY[i]); | |
| coord.setLongitude(shpObject->padfX[i]); | |
| } | |
| // If utmZone is 0, it indicates a lat/lon shape, and we skip UTM to Geo conversion. | |
| if (!utmZone || !QGCGeo::convertUTMToGeo(shpObject->padfX[i], shpObject->padfY[i], utmZone, utmSouthernHemisphere, coord)) { | |
| coord.setLatitude(shpObject->padfY[i]); | |
| coord.setLongitude(shpObject->padfX[i]); |
HTRamsey
force-pushed
the
dev-shp-polyline
branch
from
e5c7c09 to
71c5799
Compare
HTRamsey
force-pushed
the
dev-shp-polyline
branch
3 times, most recently
from
10bf3c1 to
5f62c05
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading SHP polyline files (in addition to KML) and updates related unit tests and UI dialogs accordingly.
- Added unit tests for both polyline and polygon SHP file loading
- Extended ShapeFileHelper and SHPFileHelper to support SHP polyline functionality
- Updated QGCMapPolyline and CorridorScanComplexItem to use a unified loadKMLOrSHPFile method
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Utilities/Shape/ShapeTest.cc | Added tests for polyline and polygon loading from SHP files |
| test/Utilities/Shape/CMakeLists.txt | Added Shape tests sources and include directories |
| test/Utilities/FileSystem/CMakeLists.txt | Added new FileSystem tests |
| test/Utilities/CMakeLists.txt | Updated subdirectory inclusion for Shape and FileSystem |
| test/UnitTestList.cc | Updated unit test registration order |
| test/UnitTest.qrc | Added SHP resource files for Shape tests |
| test/MissionManager/QGCMapPolylineTest.cc | Updated polyline file load tests to use loadKMLOrSHPFile |
| test/MissionManager/CorridorScanComplexItemTest.cc | Updated instantiation with new parameter naming |
| test/CMakeLists.txt | Updated qgc test targets for new Shape and FileSystem tests |
| src/Utilities/Shape/ShapeFileHelper.h | Added internal enum and methods to detect SHP file type |
| src/Utilities/Shape/ShapeFileHelper.cc | Replaced if-else with switch-case to support KML/SHP loading |
| src/Utilities/Shape/SHPFileHelper.h | Declared new method for loading polyline from a SHP file |
| src/Utilities/Shape/SHPFileHelper.cc | Implemented SHP polyline loading logic using goto Error flow |
| src/QmlControls/QGCMapPolyline.h | Modified properties and added new invokable method for SHP |
| src/QmlControls/QGCMapPolyline.cc | Updated polyline implementation for SHP file loading and related changes |
| src/QmlControls/QGCMapPolygon.h | Corrected inline documentation for file type |
| src/QmlControls/KMLOrSHPFileDialog.qml | Updated dialog title and file filter to reflect KML/SHP usage |
| src/MissionManager/CorridorScanComplexItem.h | Updated constructor parameter documentation to kmlOrShpFile |
| src/MissionManager/CorridorScanComplexItem.cc | Updated file load call to use loadKMLOrSHPFile |
| src/FlightMap/MapItems/QGCMapPolylineVisuals.qml | Updated UI text and callback to load KML/SHP files |
Comments suppressed due to low confidence (1)
src/QmlControls/QGCMapPolyline.cc:308
- [nitpick] Consider adding a comment that clarifies the intentional treatment of the first vertex as the NED origin (resulting in (0,0)) to help future maintainers understand this design decision.
if (i != 0) { const QGeoCoordinate vertex = vertexCoordinate(i); QGCGeo::convertGeoToNed(vertex, tangentOrigin, y, x, down); }
| goto Error; | ||
| } | ||
|
|
||
| shpObject = SHPReadObject(shpHandle, 0); |
There was a problem hiding this comment.
Consider checking if 'shpObject' is null immediately after the call to 'SHPReadObject' to avoid potential dereferencing of a null pointer.
Suggested change
| shpObject = SHPReadObject(shpHandle, 0); | |
| shpObject = SHPReadObject(shpHandle, 0); | |
| if (!shpObject) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Failed to read shape object.")); | |
| goto Error; | |
| } |
1 task
HTRamsey
force-pushed
the
dev-shp-polyline
branch
3 times, most recently
from
9087eaa to
7ef9b17
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds SHP polyline support alongside existing KML handling, updates ShapeFileHelper and SHPFileHelper, and extends QGCMapPolyline to load both KML and SHP polyline files. Unit tests and resources are added for the new functionality, and mission item constructors and dialogs are updated to accept SHP.
- Introduce
ShapeFileTypeenum and unified_getShapeFileTypein ShapeFileHelper - Implement
loadPolylineFromFilein SHPFileHelper and update QGCMapPolyline toloadKMLOrSHPFile - Add unit tests, test resources, and CMake entries for SHP/polyline
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Utilities/Shape/ShapeFileHelper.{h,cc} | Add enum-based dispatch and SHP detection for polylines |
| src/Utilities/Shape/SHPFileHelper.{h,cc} | Declare/implement loadPolylineFromFile and ARC support |
| src/QmlControls/QGCMapPolyline.{h,cc} | Rename loadKMLFile to loadKMLOrSHPFile |
| src/QmlControls/QGCMapPolygon.h | Update header for new loadKMLOrSHPFile |
| src/FlightMap/MapItems/QGCMapPolylineVisuals.qml | Replace KML dialog with generic KML/SHP dialog |
| src/MissionManager/* | Update constructors and JSON load calls to use SHP arg |
| test/ | Add Shape test resources, update UnitTestList and CMake |
Comments suppressed due to low confidence (4)
src/QmlControls/QGCMapPolygon.h:69
- The new Q_INVOKABLE signature was added to the header but there’s no corresponding implementation update in QGCMapPolygon.cc. Ensure loadKMLOrSHPFile is implemented and delegates to ShapeFileHelper.loadPolygonFromFile.
Q_INVOKABLE bool loadKMLOrSHPFile(const QString& file);
test/MissionManager/QGCMapPolylineTest.cc:189
- After loading the SHP/KML file, the test only checks success but does not verify that the polyline and model contain the expected vertices. Add assertions for count(), coordinateList(), and pathModel values.
QVERIFY(_mapPolyline->loadKMLOrSHPFile(shpFile));
src/QmlControls/QGCMapPolyline.cc:348
- [nitpick] The previous version included a reset guard (
_beginResetIfNotActive()/endReset) before parsing; removing it may leave stale model state. Consider re-adding begin/end reset calls around the coordinate load.
bool QGCMapPolyline::loadKMLOrSHPFile(const QString &file)
src/Utilities/Shape/SHPFileHelper.cc:63
- The regex uses
(\d+){1,2}which repeats one-or-more digits 1–2 times. It should likely be\d{1,2}to match exactly 1–2 digits for the UTM zone.
static const QRegularExpression regEx(QStringLiteral("^PROJCS\\[\"WGS_1984_UTM_Zone_(\\d+){1,2}([NS]{1})"));
HTRamsey
force-pushed
the
dev-shp-polyline
branch
from
7ef9b17 to
3d60601
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for SHP polyline loading in addition to the existing KML functionality and updates both production and test code to handle the new file type. Key changes include:
- Implementation of SHP polyline support in ShapeFileHelper and SHPFileHelper.
- Refactoring of QGCMapPolyline and related mission item classes to use loadKMLOrSHPFile.
- Updates to test files, CMakeLists, and resource files to include SHP and Shape file support.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Utilities/FileSystem/CMakeLists.txt | Adds FileSystem unit tests for file download. |
| test/Utilities/CMakeLists.txt | Removes old target configurations and adds subdirectories for FileSystem/Shape. |
| test/UnitTestList.cc | Updates unit test registrations and ordering for new SHP/shape tests. |
| test/UnitTest.qrc | Includes new resource files for Shape files. |
| test/MissionManager/QGCMapPolylineTest.{h,cc} | Refactors tests to use MultiSignalSpyV2 and coordinateList() for polyline tests. |
| test/MissionManager/CorridorScanComplexItemTest.cc | Updates parameter comments to reflect kmlOrShpFile usage. |
| test/CMakeLists.txt | Adjusts test configuration to add ShapeTest and QGCFileDownloadTest. |
| src/Utilities/Shape/{ShapeFileHelper.h,ShapeFileHelper.cc} | Introduces ShapeFileType enum and functions to determine file type; supports SHP polyline. |
| src/Utilities/Shape/{SHPFileHelper.h,SHPFileHelper.cc} | Adds loadPolylineFromFile and refactors error handling and SHP type detection. |
| src/QmlControls/{QGCMapPolyline.h,QGCMapPolyline.cc} | Renames loadKMLFile to loadKMLOrSHPFile and updates implementation. |
| src/QmlControls/QGCMapPolygon.h | Fixes documentation comment for loading a polygon from KML/SHP. |
| src/QmlControls/KMLOrSHPFileDialog.qml | Updates the file dialog to support SHP file selection. |
| src/MissionManager/{MissionController.cc,CorridorScanComplexItem.{h,cc}} | Updates constructor and parameter names for consistency with SHP support. |
| src/FlightMap/MapItems/QGCMapPolylineVisuals.qml | Adjusts UI to load KML/SHP files and update dialog text accordingly. |
Comment on lines
+221
to
+247
| SHPObject *shpObject = nullptr; | ||
|
|
||
| errorString.clear(); | ||
| vertices.clear(); | ||
|
|
||
| SHPHandle shpHandle = SHPFileHelper::_loadShape(shpFile, &utmZone, &utmSouthernHemisphere, errorString); | ||
| if (!errorString.isEmpty()) { | ||
| goto Error; | ||
| } | ||
| Q_CHECK_PTR(shpHandle); | ||
|
|
||
| int cEntities, shapeType; | ||
| SHPGetInfo(shpHandle, &cEntities, &shapeType, nullptr /* padfMinBound */, nullptr /* padfMaxBound */); | ||
| if (shapeType != SHPT_ARC) { | ||
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "File does not contain a polyline.")); | ||
| goto Error; | ||
| } | ||
|
|
||
| shpObject = SHPReadObject(shpHandle, 0); | ||
| if (!shpObject) { | ||
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Failed to read polyline object.")); | ||
| goto Error; | ||
| } | ||
|
|
||
| if (shpObject->nParts != 1) { | ||
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Only single part polylines are supported.")); | ||
| goto Error; |
There was a problem hiding this comment.
[nitpick] Consider refactoring the error handling in loadPolylineFromFile to avoid the use of 'goto'. Using RAII techniques or structured error handling could improve code clarity and maintainability.
Suggested change
| SHPObject *shpObject = nullptr; | |
| errorString.clear(); | |
| vertices.clear(); | |
| SHPHandle shpHandle = SHPFileHelper::_loadShape(shpFile, &utmZone, &utmSouthernHemisphere, errorString); | |
| if (!errorString.isEmpty()) { | |
| goto Error; | |
| } | |
| Q_CHECK_PTR(shpHandle); | |
| int cEntities, shapeType; | |
| SHPGetInfo(shpHandle, &cEntities, &shapeType, nullptr /* padfMinBound */, nullptr /* padfMaxBound */); | |
| if (shapeType != SHPT_ARC) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "File does not contain a polyline.")); | |
| goto Error; | |
| } | |
| shpObject = SHPReadObject(shpHandle, 0); | |
| if (!shpObject) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Failed to read polyline object.")); | |
| goto Error; | |
| } | |
| if (shpObject->nParts != 1) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Only single part polylines are supported.")); | |
| goto Error; | |
| errorString.clear(); | |
| vertices.clear(); | |
| std::unique_ptr<SHPHandle, decltype(&SHPClose)> shpHandle( | |
| SHPFileHelper::_loadShape(shpFile, &utmZone, &utmSouthernHemisphere, errorString), &SHPClose); | |
| if (!shpHandle || !errorString.isEmpty()) { | |
| return false; | |
| } | |
| int cEntities, shapeType; | |
| SHPGetInfo(shpHandle.get(), &cEntities, &shapeType, nullptr /* padfMinBound */, nullptr /* padfMaxBound */); | |
| if (shapeType != SHPT_ARC) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "File does not contain a polyline.")); | |
| return false; | |
| } | |
| std::unique_ptr<SHPObject, decltype(&SHPDestroyObject)> shpObject( | |
| SHPReadObject(shpHandle.get(), 0), &SHPDestroyObject); | |
| if (!shpObject) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Failed to read polyline object.")); | |
| return false; | |
| } | |
| if (shpObject->nParts != 1) { | |
| errorString = QString(_errorPrefix).arg(QT_TRANSLATE_NOOP("SHP", "Only single part polylines are supported.")); | |
| return false; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
KML had it but not SHP
Add Unit Tests
TODO next after this: #5978