-
Notifications
You must be signed in to change notification settings - Fork 0
Implement Snap2Map calibration pipeline and UI #3
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
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 foundational Snap2Map application, transforming the project from a generic template into a functional tool. It establishes a user-guided workflow for calibrating map photos with real-world coordinates, enabling live GPS tracking on imported images, and ensures offline usability. The changes encompass a complete UI overhaul, the implementation of core geometric calibration algorithms, and robust testing for the new modules. Highlights
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 pull request introduces the core functionality for the Snap2Map application, including a new UI, a service worker for offline support, and a comprehensive backend for map calibration. The calibration logic is robust and well-tested, covering various transformation models and edge cases. The UI is well-structured, but there are several issues related to theming and resource management that need attention. A critical issue is the use of CommonJS require statements in browser-facing code without a bundling step, which will prevent the application from running. I've provided detailed feedback on these points, including suggestions for image optimization and fixing UI inconsistencies.
src/calibration/calibrator.js
Outdated
| const { | ||
| computeOrigin, | ||
| wgs84ToEnu, | ||
| } = require('../geo/coordinate'); | ||
| const { | ||
| fitSimilarity, | ||
| fitAffine, | ||
| fitHomography, | ||
| applyTransform, | ||
| applyInverseTransform, | ||
| jacobianForTransform, | ||
| averageScaleFromJacobian, | ||
| } = require('../geo/transformations'); |
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.
This file and others (coordinate.js, transformations.js) use CommonJS require statements to import modules. This will fail in a browser environment as require is not defined. The application seems to be structured for a Node.js environment (like in tests) but isn't set up to run in the browser. To fix this, you either need to add a build step with a bundler (like Webpack, Rollup, or Parcel) to resolve these modules, or refactor the code to use ES modules with <script type='module'> and import maps, or load scripts in order and use a global namespace (like the current window.Snap2Map pattern, but without require). Given that index.html only loads src/index.js, the application will not be able to load its core calibration logic.
src/index.js
Outdated
| reader.onload = () => { | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| loadPhotoMap(reader.result, img.width, img.height); | ||
| }; | ||
| img.src = reader.result; | ||
| }; |
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 current implementation loads the full-size image into memory as a data URL. For large images from modern cameras, this can consume a significant amount of memory and cause performance issues or even crashes on mobile devices. The UI text mentions an 'optimized copy', so it would be beneficial to resize the image before storing and using it. You can achieve this by drawing the image onto a canvas at a reduced size and then generating the data URL from the canvas.
reader.onload = () => {
const img = new Image();
img.onload = () => {
const MAX_DIMENSION = 2048;
const { width, height } = img;
const scale = Math.min(MAX_DIMENSION / width, MAX_DIMENSION / height, 1);
const canvas = document.createElement('canvas');
canvas.width = width * scale;
canvas.height = height * scale;
const ctx = canvas.getContext('2d');
ctx.drawImage(img, 0, 0, canvas.width, canvas.height);
const optimizedDataUrl = canvas.toDataURL('image/jpeg', 0.9);
loadPhotoMap(optimizedDataUrl, canvas.width, canvas.height);
};
img.src = reader.result;
};| const combined = { | ||
| status: 'ok', | ||
| origin, | ||
| kind: result.kind, | ||
| model: result.model, | ||
| metrics, | ||
| quality: { | ||
| rmse: metrics.rmse, | ||
| maxResidual: metrics.maxResidual, | ||
| }, | ||
| statusMessage: calibrationStatus(result.kind), | ||
| residuals: metrics.residuals, | ||
| inliers: metrics.inliers, | ||
| }; |
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 object returned by calibrateMap on success includes the metrics object, but also duplicates several of its properties at the top level (residuals, inliers) and under quality (rmse, maxResidual). This redundancy can make the data structure confusing to use and maintain. It would be cleaner to return the metrics object and have consumers access its properties directly, or to structure the return object without the top-level metrics field if quality, residuals, and inliers are the intended public API.
const combined = {
status: 'ok',
origin,
kind: result.kind,
model: result.model,
quality: {
rmse: metrics.rmse,
maxResidual: metrics.maxResidual,
},
statusMessage: calibrationStatus(result.kind),
residuals: metrics.residuals,
inliers: metrics.inliers,
};
src/index.js
Outdated
|
|
||
| state.pairs.forEach((pair, index) => { | ||
| const row = document.createElement('tr'); | ||
| row.className = index % 2 === 0 ? 'bg-white' : 'bg-gray-50'; |
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 background colors bg-white and bg-gray-50 are for a light theme and will contrast poorly with the dark theme of the application. Consider using colors from the slate palette for consistency, or removing the background color and relying on the divide-y class on the tbody.
| row.className = index % 2 === 0 ? 'bg-white' : 'bg-gray-50'; | |
| row.className = index % 2 === 0 ? '' : 'bg-slate-900/40'; |
src/index.js
Outdated
| row.innerHTML = ` | ||
| <td class="px-3 py-2 text-sm text-gray-700 space-x-2">${indicator}<span>${pair.pixel.x.toFixed(1)}, ${pair.pixel.y.toFixed(1)}</span></td> | ||
| <td class="px-3 py-2 text-sm text-gray-700">${formatLatLon(pair.wgs84.lat, 'N', 'S')} · ${formatLatLon(pair.wgs84.lon, 'E', 'W')}</td> | ||
| <td class="px-3 py-2 text-sm text-gray-700">${residual !== null && residual !== undefined ? `${residual.toFixed(1)} m` : '—'}</td> | ||
| <td class="px-3 py-2 text-right"> | ||
| <button class="text-sm text-red-600 hover:underline" data-action="delete" data-index="${index}">Remove</button> | ||
| </td>`; |
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 text-gray-700 class is used here, which is a dark text color intended for light backgrounds. On the application's dark theme, this text will be unreadable. Please change this to a light text color, such as text-slate-300, for proper visibility. The text-red-600 on the button could also be lightened to text-red-500 for better contrast.
| row.innerHTML = ` | |
| <td class="px-3 py-2 text-sm text-gray-700 space-x-2">${indicator}<span>${pair.pixel.x.toFixed(1)}, ${pair.pixel.y.toFixed(1)}</span></td> | |
| <td class="px-3 py-2 text-sm text-gray-700">${formatLatLon(pair.wgs84.lat, 'N', 'S')} · ${formatLatLon(pair.wgs84.lon, 'E', 'W')}</td> | |
| <td class="px-3 py-2 text-sm text-gray-700">${residual !== null && residual !== undefined ? `${residual.toFixed(1)} m` : '—'}</td> | |
| <td class="px-3 py-2 text-right"> | |
| <button class="text-sm text-red-600 hover:underline" data-action="delete" data-index="${index}">Remove</button> | |
| </td>`; | |
| row.innerHTML = ` | |
| <td class="px-3 py-2 text-sm text-slate-300 space-x-2">${indicator}<span>${pair.pixel.x.toFixed(1)}, ${pair.pixel.y.toFixed(1)}</span></td> | |
| <td class="px-3 py-2 text-sm text-slate-300">${formatLatLon(pair.wgs84.lat, 'N', 'S')} · ${formatLatLon(pair.wgs84.lon, 'E', 'W')}</td> | |
| <td class="px-3 py-2 text-sm text-slate-300">${residual !== null && residual !== undefined ? `${residual.toFixed(1)} m` : '—'}</td> | |
| <td class="px-3 py-2 text-right"> | |
| <button class="text-sm text-red-500 hover:underline" data-action="delete" data-index="${index}">Remove</button> | |
| </td>`; |
src/index.js
Outdated
| function setActiveView(view) { | ||
| if (view === 'photo') { | ||
| dom.photoView.classList.remove('hidden'); | ||
| dom.osmView.classList.add('hidden'); | ||
| dom.photoTabButton.classList.add('bg-blue-600', 'text-white'); | ||
| dom.photoTabButton.classList.remove('bg-white', 'text-blue-600'); | ||
| dom.osmTabButton.classList.remove('bg-blue-600', 'text-white'); | ||
| dom.osmTabButton.classList.add('bg-white', 'text-blue-600'); | ||
| if (state.photoMap) { | ||
| state.photoMap.invalidateSize(); | ||
| } | ||
| } else { | ||
| dom.photoView.classList.add('hidden'); | ||
| dom.osmView.classList.remove('hidden'); | ||
| dom.osmTabButton.classList.add('bg-blue-600', 'text-white'); | ||
| dom.osmTabButton.classList.remove('bg-white', 'text-blue-600'); | ||
| dom.photoTabButton.classList.remove('bg-blue-600', 'text-white'); | ||
| dom.photoTabButton.classList.add('bg-white', 'text-blue-600'); | ||
| state.osmMap.invalidateSize(); | ||
| } | ||
| } |
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 classes applied to the inactive tab button are inconsistent. In index.html, the inactive tab has bg-white/10 text-blue-300. However, this function applies bg-white text-blue-600 when a tab becomes inactive. This creates a visual inconsistency. To fix this, the JavaScript should apply the same classes as defined in the initial HTML for the inactive state.
…or improved readability
…ccuracy circle logic into separate functions
…ast notifications
feat: implement guided pairing mode with user prompts and toast notif…
…on-as-per-readme.md
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68cd05411230832cadd9de6d804d7dc1