Add Overpass caching, generation estimation, OSM coverage checks, and improved Overture/WKB parsing#7
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
The PR successfully adds MultiPolygon support for Overture buildings and implements Overpass API caching. The implementation is sound with proper error handling, stable ID generation using part indices, and time-based cache expiry. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
Code Review
This pull request introduces several significant features: Overpass API response caching to speed up repeated generations, a generation estimation system for time and disk usage, and an OSM coverage assessment tool that warns users about low-density areas. Additionally, the Overture Maps integration was enhanced to support MultiPolygon geometries and dimensional variants in WKB parsing. Feedback focuses on ensuring the stability of cache keys across compiler updates, fixing a potential ID collision bug when processing Overture building parts, and localizing new UI warning messages.
|
|
||
| /// Compute a stable-enough cache key for an Overpass query and bbox. | ||
| fn overpass_cache_key(bbox: &LLBBox, query: &str) -> String { | ||
| let mut hasher = DefaultHasher::new(); |
There was a problem hiding this comment.
DefaultHasher is not guaranteed to be stable across different Rust compiler versions or platforms. Using it for a persistent on-disk cache means that after a compiler update, existing cache files may become unreachable because their keys (hashes) will change, leading to orphaned files. Consider using a stable hashing algorithm like FNV (already implemented in overture.rs) or a cryptographic hash to ensure cache persistence across updates.
| if (level === "very_low") { | ||
| banner.textContent = `⚠ Very low OSM coverage: only ${buildingCount} buildings found in ${areaKm2.toFixed(1)} km². The world may be mostly empty.`; | ||
| banner.style.display = "block"; | ||
| setTimeout(() => { banner.style.display = "none"; }, 10000); | ||
| } else if (level === "low") { | ||
| banner.textContent = `⚠ Low OSM coverage: ${buildingsPerKm2.toFixed(0)} buildings/km² found. Supplementary data may help fill the area.`; | ||
| banner.style.display = "block"; | ||
| setTimeout(() => { banner.style.display = "none"; }, 8000); | ||
| } else { |
There was a problem hiding this comment.
These user-facing warning messages are hardcoded in English. To maintain consistency with the project's internationalization efforts, these strings should be moved to the localization JSON files and accessed via window.localization, using placeholders for dynamic values like buildingCount and areaKm2.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Motivation
Description
estimationmodule (src/estimation.rs) withestimate_generationand exposed it to the GUI viagui_estimate_generationand printed in the CLI startup inmain.rs, plus UI/JS changes (src/gui.rs,src/gui/index.html,src/gui/js/main.js) to show and refresh estimates.src/retrieve_data.rsincludingoverpass_cache_key,load_cached_overpass,save_overpass_cache,cleanup_old_overpass_cache, and ause_cacheparameter tofetch_data_from_overpass, added CLI args--use-cache/--no-cacheinsrc/args.rs, and call site updates inmain.rs, GUI wiring ingui.rs, and test utility adjustments.src/osm_coverage.rs) withassess_osm_coverageand integrated it into both CLI and GUI flows to emit warnings and a GUI eventosm-coverage-warningthat the frontend displays; included UI banner and JS listener changes to show contextual messages.src/overture.rs) to return multiple building parts, generate stable per-part IDs, and replaced the oldparse_wkb_polygonwith a more robust WKB parser supporting Polygon, MultiPolygon and dimensional variants (Z/M/ZM) and added defensive checks to prevent resource exhaustion; also small fix insrc/elevation/providers/copernicus.rsiterator usage.Testing
cargo test, which executed the new tests insrc/estimation.rs,src/osm_coverage.rs, and updatedsrc/args.rstests; all tests passed.cargo buildand the build succeeded.Codex Task