Feat/instant usage v4 #25
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
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.
…d enhance Quick Start Flow details
…ap calibration, improve fallback indicators, and enable live mode based on GPS points.
… position and enhance user feedback for missing scale.
…nd implement one-tap calibration functionality
…ases for improved clarity and consistency
…ale is set and no pairs exist
…g for prompt classes
…dPhotoMap function; update related test case
…ion; update related test case for clarity
…est passes by validating state
… ensure logic path is verified by state conditions
…calibrateMap to utilize it
…n and active calibration states
…ode organization and readability
…ases of implementation and testing
…add default rotation setting and refine prompt visibility logic
….md:100-112 to match the correct implementation in the source code. Tests: I improved the test case in scale.test.js:379-385 to specifically assert that 0.6090m (which rounds to 12 inches) correctly results in 2' 0".
…to reflect the correct implementation of getMetersPerPixelFromCalibration. The documentation now correctly states that the calibration maps pixels directly to ENU coordinates (meters), eliminating the need for degree-to-meter conversions. Removed Outdated Constant: I removed the unused METERS_PER_DEGREE_EQUATOR constant from scale.js:8, as it was part of the outdated logic and no longer needed. Verified Implementation: I confirmed that the actual implementation in scale.js already correctly handled the scale extraction without the unnecessary conversion. Fixed Lint Errors: I resolved a lint error in index.scale.test.js that was uncovered during the quality check process.
…fy reference scale and initial suggestion prompts
…ementation in transformations.js, which returns a structured object with scale, rotation, and translation instead of a raw matrix. Calibration Layer: Updated the calibrateMap snippet to include the calibrate1Point helper function as it exists in calibrator.js, ensuring the return structure (including status, metrics, and quality) is consistent with the actual API
Extracting the hiding and showing logic into two new helper functions: hideInstantUsagePrompts() and showInstantUsagePrompts(). Replacing all manual manipulations of dom.instantUsagePrompts.classList with calls to these helpers. Simplifying setPhotoImportState and beginPairMode by using the new hideInstantUsagePrompts() helper.
…convert rotation degrees (0, 90, 180, 270) into their corresponding labels ("North-up", "East-up", etc.).
Updated handleDistanceModalConfirm: Modified the toast message in index.js:1042 to dynamically include the selected rotation label when a 1-point calibration becomes active after setting the scale.
Updated handleOneTapClick: Modified the toast message in index.js:1137 to dynamically include the selected rotation label when a 1-point calibration is active after pinning a location
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 introduces the 'Instant Usage' feature, significantly enhancing the initial user experience by enabling measurement and live GPS tracking on imported maps with fewer calibration points. It achieves this through a new 1-point calibration method, a manual 'Set Scale' tool, and a streamlined quick-start flow. The changes involve updates to the core calibration logic, new UI components, and a refactored architecture for better testability and maintainability. 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 adds significant new functionality ("Instant Usage" via 1-point calibration and manual scale setting). The changes are extensive, touching math layers, calibration logic, UI, state management, and adding comprehensive documentation and tests. The code is well-structured, with good separation of concerns (e.g., extracting pure state machine logic from UI code). The addition of property-based tests is a great sign of quality.
I found a couple of issues: one high-severity bug in unit conversion logic that could lead to incorrect scale calculations, and one medium-severity logic issue in handling geolocation permissions. My review includes suggestions to fix these. Overall, this is a high-quality contribution.
| export function convertToMeters(value, unit) { | ||
| if (!Number.isFinite(value) || value <= 0) { | ||
| return null; | ||
| } | ||
| switch (unit) { | ||
| case 'ft': | ||
| return value / METERS_TO_FEET; | ||
| default: | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
The convertToMeters function doesn't handle the 'ft-in' unit, which is an option in the UI's distance input modal. When a user selects 'Feet & Inches', the default case in the switch statement is triggered, incorrectly treating the input value as meters instead of feet. This will lead to incorrect scale calculations.
Given the current UI has a single number input, a simple fix is to treat 'ft-in' the same as 'ft', assuming the user enters a decimal value for feet (e.g., 5.5 for 5' 6").
Additionally, it would be beneficial to add a unit test case for the 'ft-in' unit to prevent this from happening in the future.
| export function convertToMeters(value, unit) { | |
| if (!Number.isFinite(value) || value <= 0) { | |
| return null; | |
| } | |
| switch (unit) { | |
| case 'ft': | |
| return value / METERS_TO_FEET; | |
| default: | |
| return value; | |
| } | |
| } | |
| export function convertToMeters(value, unit) { | |
| if (!Number.isFinite(value) || value <= 0) { | |
| return null; | |
| } | |
| switch (unit) { | |
| case 'ft': | |
| case 'ft-in': // Assume decimal feet are entered for 'ft-in' | |
| return value / METERS_TO_FEET; | |
| default: | |
| return value; | |
| } | |
| } |
| if (status.state === 'granted') { | ||
| requestAndCenterOsmOnUser(); | ||
| } else if (status.state === 'prompt' && shouldPrompt) { | ||
| doRequest(); | ||
| } |
There was a problem hiding this comment.
There's a small logic inconsistency here. When the geolocation permission is already 'granted', you call requestAndCenterOsmOnUser() directly. However, when the state is 'prompt', you call doRequest(), which not only calls requestAndCenterOsmOnUser() but also sets state.osmGeoPrompted = true.
This can lead to inconsistent state. If permission is already granted, state.osmGeoPrompted is never set.
To make the behavior consistent and more robust, I suggest calling doRequest() in the 'granted' case as well. This ensures state.osmGeoPrompted is always correctly updated.
| if (status.state === 'granted') { | |
| requestAndCenterOsmOnUser(); | |
| } else if (status.state === 'prompt' && shouldPrompt) { | |
| doRequest(); | |
| } | |
| if (status.state === 'granted') { | |
| doRequest(); | |
| } else if (status.state === 'prompt' && shouldPrompt) { | |
| doRequest(); | |
| } |
No description provided.