40 visualise multiple point clouds simultaneously #41
Conversation
WalkthroughThis PR adds multi-point-cloud support and related UI hook wiring: config now exports an array of point-cloud metadata URLs; the viewer loads and coordinates multiple clouds (elevation, gradient, accepted filtering); elevation slider and panel code were updated to accept hooks and notify on range changes; README path guidance was adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Main as main.js
participant Viewer as potreeViewer.js
participant UI as threePanels.js
participant Cloud as PointCloud (repeated)
Main->>Viewer: createPotreeViewer(containerId, pointcloudUrls, settings)
loop for each url in pointcloudUrls
Viewer->>Cloud: load metadata.json & add point cloud
Viewer->>Cloud: apply material, projection, set initial elevation/gradient
end
Viewer-->>Main: return viewer with multiple clouds
User->>UI: adjust elevation slider
UI->>Viewer: hooks.onElevationRangeChange([low,high])
par propagate
Viewer->>Cloud: updateAllCloudsElevation([low,high])
end
sequenceDiagram
autonumber
participant User
participant UI as threePanels.js
participant Viewer as potreeViewer.js
participant Clouds as All Point Clouds
User->>UI: choose gradient
UI->>Viewer: overrideGradientSchemeClick(gradientName)
Viewer->>Clouds: updateAllCloudsGradient(gradientName)
Note over Clouds: Gradient applied uniformly to each cloud
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbit[bot]](https://avatars.git.ntnu.no/u/70329?s=60){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AcceptedFiltering/threePanels.js (1)
407-419: Blocker:hooksis not in scope inside attachSelfHealing
moveElevationContainer(hooks)references an undefinedhooks. Add a parameter and thread it through.-function attachSelfHealing(activeGetter) { +function attachSelfHealing(activeGetter, hooks) { const root = byId('potree_menu') || document.body const obs = new MutationObserver(() => { const mode = activeGetter() if (mode === 'elevation') { const src = byId('materials.elevation_container') const { body } = ensurePanelScaffold('elevation2_list') - if (src && body && src.parentNode !== body) moveElevationContainer(hooks) + if (src && body && src.parentNode !== body) moveElevationContainer(hooks) } }) obs.observe(root, { childList: true, subtree: true }) return obs }
🧹 Nitpick comments (6)
src/AcceptedFiltering/threePanels.js (2)
193-213: Throttle elevation updates to avoid UI jank across many cloudsThe slider fires on every slide/change; calling hooks for N clouds can stutter. Throttle with rAF.
-function setUpElevationSlider(hooks) { +function setUpElevationSlider(hooks) { const $ = window.jQuery || window.$ const slider = $ ? $('#sldHeightRange') : null const label = byId('lblHeightRange') if (!slider || !slider.length || !label) return - const update = () => { - const low = slider.slider('values', 0) - const high = slider.slider('values', 1) - label.textContent = `${low.toFixed(2)} to ${high.toFixed(2)}` - hooks?.onElevationRangeChange([low, high]) - } + let raf = 0 + const update = () => { + if (raf) return + raf = requestAnimationFrame(() => { + raf = 0 + const low = slider.slider('values', 0) + const high = slider.slider('values', 1) + label.textContent = `${low.toFixed(2)} to ${high.toFixed(2)}` + hooks?.onElevationRangeChange?.([low, high]) + }) + } slider.slider({ min: -10000, max: 0, values: [-10000, 0] }) slider.off('slide.custom slidestop.custom change.custom') slider.on('slide.custom slidestop.custom change.custom', update) update() }
145-147: Nit: fix typos in comments
- "wverything" → "everything"
- "Does appearnlty not work without this" → "Apparently does not work without this"
- * Polls the DOM for an element id so that wverything is ensured displayed. + * Polls the DOM for an element id so that everything is ensured displayed. ... - * Ensures a UL list host exists inside the Accepted section (Does appearnlty not work without this). + * Ensures a UL list host exists inside the Accepted section (apparently does not work without this).Also applies to: 324-327
README.md (1)
9-9: Consider BASE_URL for subpath deploymentsIf the site is served under a subpath, leading slashes in src/config.js paths may break loads. Consider documenting that paths can be prefixed with
import.meta.env.BASE_URLor made relative.src/config.js (1)
1-17: Optional: make URLs configurable and avoid leading slashTo support multiple environments, consider:
- Reading base path from
import.meta.env.BASE_URL.- Allow overriding via env (e.g., Vite env vars).
-export const POTREE_POINTCLOUD_URLS = [ - '/pointclouds/cell_1/metadata.json', +const BASE = import.meta?.env?.BASE_URL ?? '/' +export const POTREE_POINTCLOUD_URLS = [ + `${BASE}pointclouds/cell_1/metadata.json`, // ... ]src/potreeViewer.js (2)
131-137: Duplicate background rename logicYou already call
makeGlobeBackgroundOption(). The jQuery block re-renames the same input later and likely no-ops after the first rename. Remove the duplicate for clarity.- // Change name of default background from 'None' to 'Globe"' - $('#background_options_none') - .text('Globe') - .attr('id', 'background_options_globe') - .val('globe') - - viewer.setBackground('globe') + // `makeGlobeBackgroundOption()` already renames and selects 'Globe' + viewer.setBackground('globe')Also applies to: 105-106
111-129: Optional: parallelize multi-cloud loadingSequential
awaitin loop delays first render. Consider parallel loading with a bounded concurrency or Promise.all.- for (const url of pointcloudUrls) { - const e = await Potree.loadPointCloud(url) - const pc = e.pointcloud - viewer.scene.addPointCloud(pc) - // ... - pointclouds.push(pc) - } + const loaded = await Promise.all(pointcloudUrls.map((url) => Potree.loadPointCloud(url))) + for (const e of loaded) { + const pc = e.pointcloud + viewer.scene.addPointCloud(pc) + pc.material.pointSizeType = Potree.PointSizeType.ADAPTIVE + pc.material.shape = Potree.PointShape.CIRCLE + overrideShaderForGradient(pc) + pc.material.elevationRange = [-10000, 0] + pc.material.activeAttributeName = 'elevation' + pc.material.gradient = Potree.Gradients['VIRIDIS'] + e.pointcloud.projection = ecef + pointclouds.push(pc) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)src/AcceptedFiltering/threePanels.js(8 hunks)src/config.js(1 hunks)src/main.js(2 hunks)src/potreeViewer.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/potreeViewer.js (2)
src/AcceptedFiltering/threePanels.js (4)
initThreePanels(427-450)$(197-197)slider(198-198)toggleAcceptedLegend(318-321)src/config.js (2)
ecef(25-25)ecef(25-25)
src/main.js (1)
src/config.js (2)
POTREE_POINTCLOUD_URLS(1-17)POTREE_POINTCLOUD_URLS(1-17)
🔇 Additional comments (6)
src/AcceptedFiltering/threePanels.js (4)
219-230: LGTM: container move now rebinds slider with hooksRelocating the elevation container and immediately wiring the slider is correct for multi-cloud propagation.
242-245: LGTM: observer forwards hooks into moveElevationContainerEnsures re-bind on rebuilds. Good.
375-377: LGTM: ensurePanelCaptured threads hooks to moverKeeps the slider/hook wiring consistent after UI nudges.
425-426: LGTM: hooks documented and passed to self-healingSignature docs match usage; passing hooks works once the function signature is fixed.
After applying the signature change above, re-run the app to ensure no console ReferenceError remains for attachSelfHealing.
Also applies to: 446-446
src/main.js (1)
1-1: LGTM: config boundary updated to URL arrayImport and callsite align with multi-cloud API.
Also applies to: 21-22
src/potreeViewer.js (1)
85-90: Confirm 'Accepted' attribute availability across all cloudsSetting
activeAttributeName = 'Accepted'assumes the attribute exists uniformly. If any cloud lacks it, colors may be undefined.Would you like a guard that falls back to elevation if
'Accepted'is missing on a cloud?
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 (3)
src/AcceptedFiltering/threePanels.js(8 hunks)src/AnnotationControl/annotationPanel.js(1 hunks)src/potreeViewer.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/AcceptedFiltering/threePanels.js (1)
src/AnnotationControl/annotationPanel.js (3)
header(27-27)menu(25-25)panel(33-33)
src/potreeViewer.js (2)
src/AcceptedFiltering/threePanels.js (4)
initThreePanels(429-452)$(199-199)slider(200-200)toggleAcceptedLegend(320-323)src/config.js (2)
ecef(25-25)ecef(25-25)
src/AnnotationControl/annotationPanel.js (1)
src/AcceptedFiltering/threePanels.js (1)
$(199-199)
🔇 Additional comments (7)
src/AnnotationControl/annotationPanel.js (1)
59-67: LGTM: jQuery-optional toggle with proper fallbacks.The refactor correctly guards against missing jQuery by aliasing
$ = window.jQuery || window.$, checking for the accordion plugin via optional chaining ($.fn?.accordion), and providing a vanilla DOM fallback when jQuery is unavailable. This aligns with the pattern used insrc/AcceptedFiltering/threePanels.jsat line 198.src/potreeViewer.js (4)
13-21: Signature change supports multi-cloud loading.The updated signature now accepts
pointcloudUrls(array) instead of a singlepointcloudUrl, enabling the viewer to load and coordinate multiple point clouds as intended by the PR objectives.
38-38: Early declaration resolves TDZ.Declaring
pointcloudsbefore theloadGUIcallback fixes the Temporal Dead Zone issue flagged in the previous review, ensuring helper functions can safely reference the variable without a ReferenceError.
312-332: Gradient override improved with null-guard and non-destructive handlers.The refactor addresses the previous review comment by adding a null-guard at line 316 (
if (!gradientContainer) return) and usingaddEventListenerat line 320 instead ofonclickassignment, preventing handler clobbering and runtime errors when the container is absent.
113-130: No race condition exists; the original concern is incorrect.The code is properly structured. The
viewer.loadGUI()callback (line 40) runs before the point cloud loop (lines 113-130), but crucially:
- Event handlers defined in the callback (
onActivateElevation,onActivateAccepted,onElevationRangeChange) are closures that execute on user interaction, not immediatelyoverrideGradientSchemeClick()sets up event listeners at line 105; the actualpointcloudsarray processing occurs inside the click handler closure (line 324-326), which fires after user interaction- All event handlers capture the
pointcloudsarray by reference, so they will access the fully populated array when invokedExecution order is correct: GUI initialization with deferred handlers → point cloud loading → user interaction with populated data. No awaiting changes needed.
Likely an incorrect or invalid review comment.
src/AcceptedFiltering/threePanels.js (2)
49-58: LGTM: Consistent jQuery-optional toggle pattern.The toggle handler follows the same defensive pattern introduced in
annotationPanel.js(lines 59-67), aliasing jQuery, checking for the accordion plugin via optional chaining, and providing a vanilla DOM fallback. This ensures robust behavior across environments with or without jQuery.
196-209: Well-structured hook propagation for elevation range updates.The refactor from
rebindElevationLabeltosetUpElevationSlideradds support foronElevationRangeChange, enabling the viewer to synchronize elevation settings across multiple point clouds. The hooks are consistently propagated throughmoveElevationContainer(line 221) and related call sites (lines 244, 377, 416), with updated JSDoc at line 427 documenting the new callback.Also applies to: 221-221, 427-427
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/potreeViewer.js (2)
17-21: Consider adding input validation for the pointcloudUrls parameter.The function doesn't validate that
pointcloudUrlsis a non-empty array. Ifundefined,null, or a non-iterable is passed, the for-of loop will throw. If an empty array is passed, the viewer will initialize with no clouds, which may not be the intended behavior.Apply this diff to add defensive validation:
export async function createPotreeViewer( containerId, pointcloudUrls, settings ) { + if (!Array.isArray(pointcloudUrls) || pointcloudUrls.length === 0) { + throw new Error('pointcloudUrls must be a non-empty array') + } + const viewer = new Potree.Viewer(document.getElementById(containerId), { useDefaultRenderLoop: false })
38-56: Consider error handling and loading strategy for multiple clouds.The current implementation loads clouds sequentially and will reject entirely if any single cloud fails. Depending on requirements, you might want to:
- Add try-catch per cloud to handle partial failures gracefully
- Load clouds in parallel with
Promise.allSettled()for better performance- Continue with successfully loaded clouds rather than failing completely
Example with parallel loading and partial success handling:
const pointclouds = [] const results = await Promise.allSettled( pointcloudUrls.map(url => Potree.loadPointCloud(url)) ) for (const [idx, result] of results.entries()) { if (result.status === 'rejected') { console.error(`Failed to load cloud ${pointcloudUrls[idx]}:`, result.reason) continue } const e = result.value const pc = e.pointcloud viewer.scene.addPointCloud(pc) // ... rest of setup pointclouds.push(pc) } if (pointclouds.length === 0) { throw new Error('No point clouds could be loaded') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/potreeViewer.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/potreeViewer.js (2)
src/config.js (2)
ecef(25-25)ecef(25-25)src/AcceptedFiltering/threePanels.js (5)
initThreePanels(429-452)$(199-199)slider(200-200)toggleAcceptedLegend(320-323)icon(140-142)
🔇 Additional comments (6)
src/potreeViewer.js (6)
66-90: LGTM! Clean helper functions for coordinated cloud updates.The helper functions properly encapsulate the logic for updating all point clouds consistently. The use of
forEachis safe even ifpointcloudsis empty, and the functions provide good abstraction for the hook wiring.
93-103: Good defensive programming with slider value fallbacks.The hook properly handles cases where jQuery or the slider might not be available, with sensible defaults that match the initial elevation range. The type checks ensure numeric values before use.
104-110: LGTM! Hook wiring is correct and straightforward.The hooks properly coordinate UI state with point cloud visualization, correctly applying the GRAYSCALE gradient for accepted filtering and delegating elevation range changes to the helper function.
311-331: LGTM! Past review concerns addressed.The function now includes a null guard for the missing container case and uses
addEventListenerinstead of directonclickassignment, preventing handler clobbering. The logic correctly applies gradients to all clouds and tracks the selection via the callback.
336-347: Functional DOM manipulation with proper guards.The function correctly repurposes the "None" background option into a "Globe" option with appropriate null guards. While the approach of modifying existing DOM elements is somewhat unconventional, it's practical and safe with the guards in place.
190-196: Gradient texture lookup pattern is confirmed as intentional—appears consistently across all Potree shader functions.The search results reveal that
texture2D(gradient, vec2(w, 1.0-w))is not an isolated occurrence but a consistent pattern used throughout the Potree shader codebase. The pattern appears in:
- Multiple shader functions (getGpsTime, getElevation, getIntensity, getSourceID, getExtra)
- Both source files (
src/potreeViewer.js) and built files (public/build/shaders/shaders.js,public/build/potree/potree.js)- All instances with identical coordinate semantics
This uniform usage across different rendering modes and functions strongly suggests the pattern is intentional and specific to Potree's gradient texture format, rather than an error. While standard Three.js gradient maps use
vec2(w, 0.0), Potree's implementation appears to leverage the 2D nature of the gradient texture with both X and Y coordinates, which may be part of Potree's color mapping design.No issues detected.
There was a problem hiding this comment.
I only had one dataset to test with!
My testing steps:
- Convert a csv file to .las with the new lasConverter
- Used PotreeConverter to convert the .las to potree octree data
- The pointcloud is displayed as expected, also checked the pattern of the points, they are no longer in pair or triplets as far as I can see
- Tested normal features like gradient, measurements, saving locations. Everything worked as expected
When loading multiple point clouds the elevation control and accepted filtering stopped working (or only worked on one of the clouds), so had to make changes in order to fix this. While fixing the elevation control I also fixed a bug, where the elevation control and accepted filtering panels/accordions could not be closed, by using some code from the annotationPanel. CodeRabbit then commented on this logic and suggested making it safer, so I changed it both places. |
adriahso
deleted the
40-visualise-multiple-point-clouds-simultaneously
branch
Issue number
Closes #40
Description
Makes it possible to load and visualise multiple point clouds at once in order to enable grouping of data into smaller chunks. This is done to avoid floating point precision problems.
Testing steps
src/config.jsmatch your point cloud foldersSummary by CodeRabbit
New Features
Bug Fixes / UI
Documentation