xpath implementation for map functions #1487
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe codebase was refactored to replace the use of the JTS planar geometry library with the geodesy library for ellipsoid-aware computations. Polygon creation, point-in-polygon tests, and closest-point calculations now use geodesic calculations with Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PolygonUtils
participant GeodesyLib
Caller->>PolygonUtils: createPolygon(latLngList)
PolygonUtils->>GeodesyLib: Validate/convert coordinates
PolygonUtils-->>Caller: List<GlobalCoordinates>
Caller->>PolygonUtils: isPointInsideOrOnPolygon(point, polygon)
PolygonUtils->>GeodesyLib: Ray casting/geodesic calculations
PolygonUtils-->>Caller: Boolean
Caller->>PolygonUtils: findClosestPoint(point, polygon)
PolygonUtils->>GeodesyLib: Project point onto polygon edges geodesically
PolygonUtils-->>Caller: Closest point as String
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/org/javarosa/core/model/utils/PolygonUtils.java (1)
144-173: Consider floating-point precision for vertex equality checks.The ray casting algorithm is correctly implemented, but the vertex equality check on lines 161-162 uses exact floating-point comparison:
if ((testLat == lat1 && testLon == lon1) || (testLat == lat2 && testLon == lon2)) {Consider using a small epsilon for floating-point comparison to handle potential precision issues.
+ private static final double VERTEX_EPSILON = 1e-12; + // Vertex check - if ((testLat == lat1 && testLon == lon1) || (testLat == lat2 && testLon == lon2)) { + if ((Math.abs(testLat - lat1) < VERTEX_EPSILON && Math.abs(testLon - lon1) < VERTEX_EPSILON) || + (Math.abs(testLat - lat2) < VERTEX_EPSILON && Math.abs(testLon - lon2) < VERTEX_EPSILON)) { return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build.gradle(1 hunks)src/main/java/org/javarosa/core/model/utils/PolygonUtils.java(1 hunks)src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java(3 hunks)src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java(4 hunks)src/test/java/org/javarosa/xpath/test/XPathEvalTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: build
🔇 Additional comments (13)
build.gradle (1)
86-86: Verify the geodesy library version is current and stable.The addition of
org.gavaghan:geodesy:1.1.3looks correct for the migration from JTS to geodesic calculations. Please confirm this is the latest stable version and check for any known security vulnerabilities.#!/bin/bash # Check for the latest version of the geodesy library and any security advisories echo "Checking latest version of org.gavaghan:geodesy on Maven Central..." curl -s "https://search.maven.org/solrsearch/select?q=g:org.gavaghan+AND+a:geodesy&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion // "Version not found"' echo "Checking for security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.gavaghan:geodesy") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'src/test/java/org/javarosa/xpath/test/XPathEvalTest.java (2)
614-682: LGTM! Enhanced precision reflects geodesic calculations.The updated expected coordinate values with higher floating-point precision correctly reflect the transition from planar JTS geometry to geodesic-aware computations. The more precise results are expected when accounting for Earth's curvature.
704-712: Excellent addition of large-scale polygon tests.The new test cases for ~150km scale polygons are valuable for verifying the accuracy of geodesic calculations over larger geographic areas where the difference between planar and ellipsoidal calculations becomes more significant.
src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java (2)
3-3: Correct import for geodesic coordinates.The import of
GlobalCoordinatesfrom the geodesy library appropriately replaces the JTSPolygonimport, aligning with the migration to ellipsoid-aware calculations.
70-75: Good practice: added coordinate validation and geodesic-aware processing.The explicit coordinate validation using
PolygonUtils.isValidCoordinates()before creatingGlobalCoordinatesis excellent defensive programming. The transition toList<GlobalCoordinates>andisPointInsideOrOnPolygon()properly implements geodesic calculations.src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java (2)
3-3: Consistent import changes for geodesic library.The import of
GlobalCoordinatesmaintains consistency with the other XPath polygon function and supports the migration to geodesic-aware calculations.
75-80: Proper implementation of geodesic closest point calculation.The coordinate validation and use of
PolygonUtils.findClosestPoint()withGlobalCoordinatescorrectly implements ellipsoid-aware closest point calculations. The pattern is consistent with the point-in-polygon function.src/main/java/org/javarosa/core/model/utils/PolygonUtils.java (6)
2-8: Appropriate imports for geodesic calculations.The imports from
org.gavaghan.geodesyprovide the necessary components for ellipsoid-aware geographic calculations, properly replacing the JTS dependency.
23-51: Robust polygon creation with proper validation.The
createPolygonmethod includes excellent validation:
- Minimum vertex count (3 distinct vertices)
- Even number of coordinates
- Coordinate bounds checking
- Automatic polygon closure
The implementation correctly handles the transition from flat coordinate lists to
GlobalCoordinates.
60-64: Simple and effective coordinate validation.The public
isValidCoordinatesmethod provides proper bounds checking for latitude (-90 to 90) and longitude (-180 to 180). Making it public allows the XPath functions to validate coordinates before processing.
73-97: Well-implemented geodesic closest point calculation.The
findClosestPointmethod correctly:
- Uses WGS84 ellipsoid for accurate Earth calculations
- Projects the point onto each polygon segment geodesically
- Finds the minimum distance using ellipsoidal calculations
- Returns proper coordinate formatting
109-135: Mathematically sound geodesic projection.The
projectOntoSegmentmethod implements proper geodesic projection:
- Handles degenerate cases (identical endpoints)
- Uses azimuth and distance calculations on the ellipsoid
- Correctly clamps projection to segment endpoints
- Uses trigonometry to project point onto geodesic line
166-169: Good practice: epsilon handling for numerical stability.The addition of
1e-10in the denominator of the ray casting calculation (line 167) is excellent for avoiding division by zero and maintaining numerical stability in edge cases.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looks great, left some naming suggestions as something to improve on.
| double lat1 = A.getLatitude(); | ||
| double lon1 = A.getLongitude(); | ||
| double lat2 = B.getLatitude(); | ||
| double lon2 = B.getLongitude(); |
There was a problem hiding this comment.
should use A and B in naming instead of 1 and 2 to be consitent.
| int n = polygon.size(); | ||
|
|
||
| double testLat = point.getLatitude(); | ||
| double testLon = point.getLongitude(); |
There was a problem hiding this comment.
Use Long for all Longitude short forms in all vars in this class.
| * @param longitude Longitude in degrees | ||
| * @throws IllegalArgumentException if values are outside geographic bounds | ||
| */ | ||
| public static void isValidCoordinates(double latitude, double longitude) { |
There was a problem hiding this comment.
rename to validateCoordinates as it is not returning any boolean
|
@pm-dimagi This needs to be duplicated as per instructions in PR template. |
|
@shubham1g5 i don't understand this |
|
look at |
|
@shubham1g5 this comment is not working this is due to the branch is already merged I have checked the PR's in formplayer repo there is no duplicate branch as such |
|
@pm-dimagi You will need to manually do what the duplicate PR script does in that case and create a duplicate PR for it. |
|
done @shubham1g5 #1489 |
|
thanks! |
Product Description
Link
this is the XPath implementation of function
-is-point-inside-polygon
-closest-point-of-polygon
purpose is to keep earth curvature in mind to calculate the closest point
used Gavaghan Geodesy lib to do so
Technical Summary
Safety Assurance
Safety story
Automated test coverage
QA Plan
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit