V4 #21
V4 #21
Conversation
… and its implementation plan
…tionality for setting reference distances and measuring
…ion and interaction improvements
…s for centroid calculation
…ith weight influence
…tric properties and edge cases
…r improved accuracy
… null for coincident pixel points
…e over GPS calibration
… and unit selection
…te for reference distance feature
…ng reference distances
…ne module with comprehensive tests
…th refactored state management
…istance input modal for reference distance entry with validation and unit selection
…es for scale and measure markers and labels in the UI
…into a reusable function for cleaner code
…d rely on conversion function for improved clarity
…ility and integrate into distance validation
…lined user experience
…structure. I also utilized createScaleModeState() and createMeasureModeState() from the logic module to ensure consistent initial states. Logic Integration: Updated all calls to pure functions (like handleScaleModePoint or computeMeasurement) to pass only the logic sub-object. UI Management: Updated all Leaflet-related code to store and retrieve markers/lines from the ui sub-object
…ement enhancements
… calibration checks
…l referenceDistance and verify it remains unchanged after handleDistanceModalCancel() is called
…nitSelect in index.js to include logic that updates all pinned measurement labels. Exported setupEventHandlers: Added setupEventHandlers to the __testables export in index.js to facilitate integration testing. Added Integration Test: Added a new test case to index.scale.test.js:268-301 to verify that pinned measurement labels are correctly updated when the global unit changes. Improved Mocks: Updated the L.divIcon mock in index.scale.test.js:35 to correctly capture and return options for verification in tests
Creating a centralized createDistanceLabelHtml helper function in index.js:754 that handles the generation of distance labels with optional "Pin" or "Delete" buttons. Updating drawReferenceVisualization, updateMeasureLabel, and pinCurrentMeasurement to use this new helper. Replacing the regex-based replacement in pinCurrentMeasurement with a clean re-render of the label using the helper function
…DistanceLabelHtml.
Summary of ChangesHello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new feature to Snap2Map, empowering users with a flexible and accurate way to measure distances directly on their imported maps and photos. By allowing the definition of a manual reference scale, the application can now provide immediate, GPS-independent measurements, which are prioritized as 'ground truth' over GPS data. The changes encompass a complete user experience for setting and utilizing this manual scale, supporting various measurement units, and are built upon a thoroughly tested and refactored codebase for improved maintainability and reliability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an impressive pull request that introduces a significant and well-designed feature for manual scaling and measurement. The implementation is of very high quality, demonstrating excellent software engineering practices. The code is modular, with a clear separation between pure state logic and UI-side effects, which is a great pattern. The testing strategy is exceptionally thorough, including unit, integration, and even property-based tests, ensuring a high degree of confidence in the new functionality. My feedback is minor and focuses on aligning the design documentation with the final implementation to ensure it remains a reliable source of truth for future development.
| case 'ft-in': { | ||
| const totalInches = meters * METERS_TO_FEET * 12; | ||
| const feet = Math.floor(totalInches / 12); | ||
| const inches = Math.round(totalInches % 12); | ||
| return `${feet}' ${inches}"`; | ||
| } |
There was a problem hiding this comment.
The example implementation for formatDistance in the ft-in case has a potential bug. Math.round(totalInches % 12) can result in 12 if totalInches % 12 is, for example, 11.5. This would lead to an incorrect output like 5' 12". The actual implementation in src/scale/scale.js correctly handles this edge case. It would be best to update this documentation to reflect the correct, more robust implementation.
| case 'ft-in': { | |
| const totalInches = meters * METERS_TO_FEET * 12; | |
| const feet = Math.floor(totalInches / 12); | |
| const inches = Math.round(totalInches % 12); | |
| return `${feet}' ${inches}"`; | |
| } | |
| case 'ft-in': { | |
| const totalInches = meters * METERS_TO_FEET * 12; | |
| const feet = Math.floor(totalInches / 12); | |
| const inches = Math.round(totalInches % 12); | |
| if (inches === 12) { | |
| return `${feet + 1}' 0\"`; | |
| } | |
| return `${feet}' ${inches}\"`; | |
| } |
| /** | ||
| * Extracts the scale (meters per pixel) from a calibration result. | ||
| * For similarity transforms, scale = sqrt(a² + b²) where matrix is [a, b, tx; -b, a, ty]. | ||
| * Note: The calibration matrix maps pixels → geo coordinates, so this gives geo-units/pixel. | ||
| * For lat/lon, additional conversion to meters is needed based on latitude. | ||
| * | ||
| * @param {Object} calibrationResult - Result from calibrateMap() | ||
| * @returns {number|null} - Scale in meters per pixel, or null if not extractable | ||
| */ | ||
| function getMetersPerPixelFromCalibration(calibrationResult) { | ||
| if (!calibrationResult || !calibrationResult.matrix) return null; | ||
| const { a, b } = calibrationResult.matrix; | ||
| const geoUnitsPerPixel = Math.sqrt(a * a + b * b); | ||
| // Convert degrees to meters (approximate, using center latitude) | ||
| // 1 degree ≈ 111,320 meters at equator, adjusted by cos(lat) | ||
| const centerLat = calibrationResult.centerLat || 0; | ||
| const metersPerDegree = 111320 * Math.cos(centerLat * Math.PI / 180); | ||
| return geoUnitsPerPixel * metersPerDegree; | ||
| } |
There was a problem hiding this comment.
The documentation and example implementation for getMetersPerPixelFromCalibration appear to be outdated. The code describes a conversion from "geo-units/pixel" (implying degrees) to "meters/pixel" using the latitude. However, the application's calibration pipeline (calibrateMap) works with ENU coordinates, which are already in meters. The actual implementation in src/scale/scale.js correctly reflects this by directly extracting the scale from the similarity or affine model without any degree-to-meter conversion. To avoid confusion, this section of the documentation should be updated to match the simpler, correct implementation.
/**
* Extracts the scale (meters per pixel) from a calibration result.
* For similarity transforms, the scale is a direct property of the model.
* For affine transforms, an average scale is computed.
* Note: The calibration matrix maps pixels → ENU coordinates (meters),
* so the scale is already in meters/pixel.
*
* @param {Object} calibrationResult - Result from calibrateMap()
* @returns {number|null} - Scale in meters per pixel, or null if not extractable
*/
function getMetersPerPixelFromCalibration(calibrationResult) {
if (!calibrationResult || calibrationResult.status !== 'ok' || !calibrationResult.model) {
return null;
}
const { model } = calibrationResult;
if (model.type === 'similarity' && typeof model.scale === 'number') {
return Math.abs(model.scale);
}
if (model.type === 'affine' && model.matrix) {
const { a, b, c, d } = model.matrix;
const scaleX = Math.hypot(a, b);
const scaleY = Math.hypot(c, d);
return (scaleX + scaleY) / 2;
}
return null;
}Achieved 100% Statement, Branch, Function, and Line coverage across all files in the src directory. Added comprehensive test cases to transformations.test.js to cover: Degenerate cases in fitHomography. Singular matrices in invertHomography. Jacobian calculations for affine and homography transforms. Unsupported transform types in applyTransform and applyInverseTransform. Added test cases to calibrator.test.js to cover: Automatic origin computation in calibrateMap. Edge cases in evaluateModel (e.g., infinite RMSE). Defensive branches in accuracyRingRadiusPixels. Code Refinement: Simplified a redundant null check in accuracyRingRadiusPixels within calibrator.js to improve branch coverage and code clarity.
No description provided.