13 minimap #42
13 minimap #42
Conversation
WalkthroughAdds a Potree minimap integration: exports initMiniMap(viewer), replaces online OSM with an offline GeoJSON layer, removes tile-download controls, overrides minimap update to compute a 2D pointer from 3D camera coordinates (including EPSG:4978 handling), and recenters the map via proj4 EPSG:4326→EPSG:3857. Changes
Sequence Diagram(s)sequenceDiagram
participant Viewer as Potree Viewer
participant Init as initMiniMap
participant Map as Minimap (OL/Potree Map)
participant Proj as proj4 / toMap
Viewer->>Init: initMiniMap(viewer)
Init->>Map: access mapView & layers
Init->>Map: replace layers with offline GeoJSON
Init->>Map: remove tile-download controls
Init->>Map: set background color
Init->>Map: override mapView.update
Note over Viewer,Map: Runtime — camera moves
Viewer->>Map: mapView.update() (patched)
Map->>Proj: transform 3D camera coords (handles EPSG:4978)
Proj-->>Map: 2D mapPos + orientation
Map->>Map: update pointer position, rotation, and gCamera
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbit[bot]](https://avatars.git.ntnu.no/u/70329?s=60){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MiniMap/miniMap.js(1 hunks)src/potreeViewer.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/MiniMap/miniMap.js (2)
src/AcceptedFiltering/threePanels.js (1)
$(188-188)src/main.js (1)
init(10-37)
src/potreeViewer.js (1)
src/MiniMap/miniMap.js (1)
initMiniMap(6-17)
🔇 Additional comments (5)
src/potreeViewer.js (2)
3-3: LGTM!The import follows the established pattern and correctly references the new minimap module.
94-94: Clarify the camera position change.The initial camera position has been changed, but this modification isn't mentioned in the PR description. Please confirm whether this change is:
- Intentional and related to the minimap feature (e.g., aligning with the minimap's center coordinates)
- Intended to provide a better starting view
- An accidental change that should be reverted
src/MiniMap/miniMap.js (3)
25-42: Verify globalolavailability and GeoJSON file path before finalizing.Based on the script output, I can confirm some but not all concerns:
Confirmed concerns:
Hardcoded layer index (line 40) ✓ Fragile—accessing
layers.item(6)by index is problematic if the minimap's layer structure changes.Missing explicit
olimport (lines 31, 32, 34) ✓ However,olis used throughout the codebase as a global. The script showsol.layer.Vector,ol.source.Vector, andol.format.GeoJSONare available globally via the bundled Potree library (public/build/potree/potree.js), not via an explicit module import in this file.Unconfirmed concern:
3. GeoJSON file path ⚠ The file/data/geo/world_simplified.geojsonis referenced only in this one location. The search found no matching files ordata/directory in the repository. This file may exist only in the deployment/build output or needs verification that it's served correctly.Recommendations:
- The
olglobal availability appears intentional (via Potree), so the missing import may not be an issue if documented.- The hardcoded index 6 should be addressed if possible or documented with assumptions about the layer structure.
- Verify the GeoJSON file path is correctly served from
/data/geo/in your deployment.
6-17: I need to check two things: (1) where and howproj4is loaded globally, and (2) howinitMiniMapis called to verify the execution order.The web search confirms that proj4 is exposed in the global variable and can be accessed as window.proj4. When a library like proj4 is loaded via a<script>tag, it becomes globally available in the browser. Now I need to verify the execution order and the reasonableness of the hardcoded coordinates.No issues found after verification—proj4 is globally available and coordinates are intentionally fixed.
The verification confirms:
proj4 availability: proj4 is exposed as a global variable accessible in the browser environment. The library is bundled in
public/libs/proj4/and becomes globally available when loaded, making the use on line 15 safe.Hardcoded center coordinates: The coordinates
[2, 69]represent longitude 2°E and latitude 69°N, which corresponds to northern Norway. This appears to be an intentionally fixed value for this specific Potree viewer deployment, not an arbitrary error.The code correctly uses proj4 without importing it locally because it's provided as a global library through the Potree environment's initialization. Both concerns in the original review are unfounded.
49-108: No action needed — globals and mapView are guaranteed to be available.The global dependencies (
THREE, jQuery$,ol,proj4) are intentionally loaded via script tags in index.html (lines 45, 49–50, 55) before the ES6 modules execute. ThemapViewis initialized in the Potree viewer beforeinitMiniMapis called frompotreeViewer.jsline 68 within theloadGUIcallback. The coordinate transformation logic is correct, including the z-coordinate handling for EPSG:4978 and the direction reversal. No changes are required.
I don't know if you have noticed, but if you hover on the buttons, there it appears the description of the two buttons. "D" downloads a .txt with the list of tiles in the map (not sure which tiles is it speaking about), "T" shows/hides the said list of tiles. I think we don't need those buttons, so we could delete them without problems.
I think we are safe with deleting it. Maybe we could ask the customer for an opinion? |
Removed the D and T tools now as we don't need them. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/MiniMap/miniMap.js (8)
1-5: Declare globals or import dependencies to avoid ReferenceErrors in bundled builds.You reference ol, proj4, THREE, and $ without imports. If they’re not guaranteed globals at runtime, this will throw. At minimum, declare globals for linting or import them as modules where applicable.
Apply a minimal lint pragma:
+/* global ol, proj4, THREE, $ */ /** * Initializes the Potree minimap to work with offline map and EPSG:4978 coordinates. * * @param {Potree.Viewer} viewer - The Potree viewer instance */If you rely on globals provided by Potree script tags, please confirm they are always present before this module executes. Otherwise, switch to explicit imports.
17-20: Avoid hard‑coding the map center; derive it from the current camera via toMap with a safe fallback.This keeps the minimap aligned with the actual dataset location and works in EPSG:4978.
- // Transform minimap center [lon, lat] from EPSG:4326 to EPSG:3857 - const center = proj4('EPSG:4326', 'EPSG:3857', [2, 69]) - map.getView().setCenter(center) + // Center the minimap on the current camera position (EPSG:4978 → map projection), fallback to a static center. + const camera = viewer.scene.getActiveCamera() + let center + try { + const forward = viewer.mapView.toMap.forward([ + camera.position.x, + camera.position.y, + camera.position.z + ]) + if (Array.isArray(forward) && Number.isFinite(forward[0]) && Number.isFinite(forward[1])) { + center = [forward[0], forward[1]] + } + } catch {} + if (!center) { + center = proj4('EPSG:4326', 'EPSG:3857', [2, 69]) + } + map.getView().setCenter(center)
28-45: Make layer replacement robust; don’t rely on magic indices.Assuming OSM is layers.item(0) and “extent” is item(6) is brittle and can break across Potree/OL versions or config changes.
-function replaceMiniMapLayers(layers) { - // Remove the old Open Street Map layer - const oldOSMLayer = layers.item(0) - layers.remove(oldOSMLayer) +function replaceMiniMapLayers(layers) { + // Remove the OSM base layer by type (more robust than assuming index 0) + const baseLayer = layers.getArray().find( + (l) => l instanceof ol.layer.Tile && + l.getSource && l.getSource() instanceof ol.source.OSM + ) + if (baseLayer) { + layers.remove(baseLayer) + } // Add new offline GeoJSON layer const newGeojsonLayer = new ol.layer.Vector({ source: new ol.source.Vector({ - url: '/data/geo/world_simplified.geojson', + url: '/data/geo/world_simplified.geojson', format: new ol.format.GeoJSON() }) }) layers.insertAt(0, newGeojsonLayer) - // Remove the extent layer - const extentLayer = layers.item(6) - layers.remove(extentLayer) + // Remove the extent layer if present (guard against missing/shifted index) + const extentLayer = layers.item(6) + if (extentLayer) { + layers.remove(extentLayer) + } }If the “extent” layer has an identifiable property (e.g., name/title), prefer selecting it by that property instead of index.
35-38: Asset URL should be base‑path safe.An absolute /data/... path breaks when the app is served under a sub‑path. Build a URL relative to the app base.
- url: '/data/geo/world_simplified.geojson', + url: `${document.baseURI.replace(/\/$/, '')}/data/geo/world_simplified.geojson`,Alternatively, if you use a bundler that supports it, prefer:
- Vite: new URL('../../data/geo/world_simplified.geojson', import.meta.url).href
- Webpack: import url from '.../world_simplified.geojson?url'
Please pick the variant that matches your toolchain.
52-61: Remove both “D” and “T” controls; avoid brittle constructor.name checks.constructor.name can change with minification and doesn’t catch the “T” button. Remove by class and by UI title/text heuristics.
-function removeTileTools(map) { - map.getControls().forEach((control) => { - if ( - control.constructor && - control.constructor.name === 'DownloadSelectionControl' - ) { - map.removeControl(control) - } - }) -} +function removeTileTools(map) { + const toRemove = [] + map.getControls().forEach((control) => { + const ctor = control && control.constructor && control.constructor.name + const el = control && control.element + const title = el && (el.title || (el.getAttribute && el.getAttribute('title'))) + const text = el && el.textContent && el.textContent.trim() + if ( + ctor === 'DownloadSelectionControl' || + ctor === 'TileListControl' || // covers a common name for the "T" tool + (title && /download|tile/i.test(title)) || + (text && /^[DT]$/.test(text)) + ) { + toRemove.push(control) + } + }) + toRemove.forEach((c) => map.removeControl(c)) +}If you know the exact control classes Potree registers for D/T, prefer instanceof checks against those classes.
68-71: Preserve original mapView.update and fallback on errors to avoid regressions.Monkey‑patching without a fallback can break the minimap if a transform fails. Keep the original update and call it on exceptions.
-function overrideMapViewUpdate(viewer) { - const Vector3 = THREE.Vector3 - const Vector2 = THREE.Vector2 - viewer.mapView.update = function (delta) { +function overrideMapViewUpdate(viewer) { + const Vector3 = THREE.Vector3 + const Vector2 = THREE.Vector2 + const originalUpdate = viewer.mapView.update.bind(viewer.mapView) + viewer.mapView.update = function (delta) { + try { if (!this.sceneProjection) { return } @@ - this.gCamera.setCoordinates([p1, p2, p3, p1]) - } + this.gCamera.setCoordinates([p1, p2, p3, p1]) + } catch (e) { + // Fallback to original behavior to keep the minimap usable + if (typeof originalUpdate === 'function') { + return originalUpdate(delta) + } + } + } }Also applies to: 125-126
76-87: Avoid per‑frame jQuery and extra DOM queries in the render loop.This runs every update; prefer the map target element’s client size and cache lookups.
- let pm = $('#potree_map') - if (!this.enabled) { return } - let mapSize = this.map.getSize() - let resized = pm.width() !== mapSize[0] || pm.height() !== mapSize[1] - if (resized) { - this.map.updateSize() - } + const targetEl = this.map.getTargetElement() + if (targetEl) { + const [w, h] = [targetEl.clientWidth, targetEl.clientHeight] + const [cw, ch] = this.map.getSize() + if (w !== cw || h !== ch) { + this.map.updateSize() + } + }If jQuery must stay, at least hoist the selection outside the loop and reuse it.
13-16: Optional: handle OpenLayers attribution/logo removal behind a flag.If the customer decides to remove the OL attribution, add a small helper and a toggle to avoid hard‑coding legal choices.
removeTileTools(map) - overrideMapViewUpdate(viewer) + // Optionally remove OL attribution/logo if permitted + // removeOlAttribution(map) + + overrideMapViewUpdate(viewer)Add helper:
function removeOlAttribution(map) { const toRemove = [] map.getControls().forEach((c) => { const ctor = c && c.constructor && c.constructor.name if (ctor === 'Attribution') toRemove.push(c) }) toRemove.forEach((c) => map.removeControl(c)) }Confirm licensing before enabling by default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MiniMap/miniMap.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MiniMap/miniMap.js (2)
src/potreeViewer.js (1)
viewer(19-21)src/AcceptedFiltering/threePanels.js (1)
$(188-188)
🔇 Additional comments (1)
src/MiniMap/miniMap.js (1)
108-126: Pointer computation looks correct; reversing direction fixes orientation.The arrow geometry (p1→p2→p3→p1) and side vector are consistent with OL coordinate space.
Please verify visually at multiple zoom levels and over dateline crossing to ensure no wrap issues.
Issue number
Closes #13
Description
Add/set up minimap to view where you are from another perspective.
Note:
Testing steps
"projection": "+proj=geocent +datum=WGS84 +units=m +no_defs"in the point cloudmetadata.jsonfile(s).Summary by CodeRabbit
New Features
Changes