-
Notifications
You must be signed in to change notification settings - Fork 0
V3 #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 significant new functionality to enhance the application's map calibration and measurement capabilities. It enables users to quickly get started with maps by defining a manual scale or using a single GPS point with default assumptions, allowing for immediate distance measurements and live position tracking. A robust mechanism for defining precise reference distances is now available, which can be used independently of GPS or to improve the accuracy and stability of GPS-based calibrations. These extensive changes are supported by a refactored codebase with improved modularity and comprehensive testing. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is an impressive pull request that introduces significant new functionality for "Instant Usage" and manual scale references. The implementation is of very high quality, demonstrating excellent software engineering practices. The features are well-documented in the new specification files, the code is cleanly refactored with a great separation of concerns (e.g., extracting pure state machine logic), and the test coverage is exceptionally thorough, including unit, integration, and property-based tests.
My review includes a couple of medium-severity suggestions for improving maintainability and user experience:
- Moving inline CSS from
index.htmlto a separate file. - Replacing the native
confirm()dialog with a custom modal for a more consistent UI.
Overall, this is a fantastic contribution that greatly enhances the application's capabilities. The changes are well-thought-out and robustly implemented.
| <style> | ||
| /* Custom marker and label styles for scale/measure features */ | ||
| .scale-marker-dot { | ||
| width: 14px; | ||
| height: 14px; | ||
| border: 2px solid white; | ||
| border-radius: 50%; | ||
| box-shadow: 0 2px 4px rgba(0, 0, 0, 0.3); | ||
| } | ||
| .distance-label { | ||
| color: white; | ||
| padding: 2px 8px; | ||
| border-radius: 4px; | ||
| font-size: 12px; | ||
| font-weight: 600; | ||
| white-space: nowrap; | ||
| box-shadow: 0 2px 4px rgba(0, 0, 0, 0.3); | ||
| transform: translate(-50%, -100%); | ||
| margin-top: -4px; | ||
| } | ||
| .distance-label--measure { | ||
| padding: 3px 10px; | ||
| font-size: 13px; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new CSS rules for markers and labels are currently defined in an inline <style> block. While this works, it's generally better for maintainability to keep styles in separate .css files. This separates concerns (structure vs. presentation) and allows for better caching and organization, especially as the project grows.
Consider moving these styles to a new CSS file (e.g., css/custom.css) and linking it in the <head>.
| if (btn) { | ||
| L.DomEvent.on(btn, 'click', (e) => { | ||
| L.DomEvent.stopPropagation(e); | ||
| if (confirm('Delete reference scale?')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses a native confirm() dialog to ask the user for confirmation before deleting a reference scale. While functional, confirm() is a blocking call that freezes the UI, and its appearance is inconsistent across different browsers and operating systems.
For a more polished and consistent user experience, it would be better to use a custom modal dialog that matches the application's visual style, similar to the one implemented for entering a distance.
For example:
// Replace confirm() with a custom modal for a better UX
showConfirmationModal('Delete reference scale?', () => {
state.referenceDistance = null;
clearReferenceVisualization();
recalculateCalibration();
saveSettings();
});This would require implementing a generic showConfirmationModal function but would improve the overall UX.
No description provided.