8 point filtering #39
Conversation
Created a separate panel for accepted filtering. When the button is clicked the colors of the points are either black for not accepted and white for accepted points. The panel also have a description for this.
Changes styles, hooks, css, scrolling, visuals to make it a better user experience
Refactor: moved elevation panel into same file as accepted panel for structural purposes
…s they now exist in new joined file
WalkthroughReplaces the standalone ElevationControl with a new two-panel UI (Elevation + Accepted Filter), adds CSS and E2E tests, moves elevation-related DOM plumbing into a self-healing threePanels module, and updates potreeViewer integration to initialize the panels and provide activation hooks and scroll-suppression utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Viewer as potreeViewer.js
participant Init as initThreePanels()
participant Panels as ThreePanels Module
participant Hooks as onActivate Callbacks
participant UI as Potree / DOM
Viewer->>Init: initThreePanels(viewer, {onActivateElevation,onActivateAccepted})
Init->>Panels: create Elevation panel & Accepted panel
Panels->>Panels: attach MutationObserver (self‑healing)
Panels->>Panels: wire activation buttons
Panels->>Hooks: switchMode('elevation') triggers onActivateElevation
Hooks->>UI: set activeAttributeName='elevation'\nset gradient=VIRIDIS
Hooks->>UI: suppressSidebarAutoScroll(...) during transition
Note over Panels: user clicks Accepted Filter
Panels->>Panels: switchMode('accepted')
Panels->>Hooks: onActivateAccepted
Hooks->>UI: set activeAttributeName='accepted'\nset gradient=GRAYSCALE
Hooks->>UI: toggleAcceptedLegend(true)
Hooks->>UI: suppressSidebarAutoScroll(...) during transition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
🧰 Additional context used🧬 Code graph analysis (1)src/potreeViewer.js (1)
🔇 Additional comments (2)
Comment |
![coderabbit[bot]](https://avatars.git.ntnu.no/u/70329?s=60){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
src/cameraSync.js (1)
36-38: Consider using Cesium's DataSourceCollection API for cleaner integration.The guard pattern is safe, but storing the data source as
_countryBorderson the viewer instance bypasses Cesium's standard DataSourceCollection API.Consider this alternative approach:
- if (cesiumViewer._countryBorders) { - cesiumViewer._countryBorders.show = showGlobe - } + const countryBorders = cesiumViewer.dataSources.getByName('countryBorders')[0] + if (countryBorders) { + countryBorders.show = showGlobe + }This uses the official API and eliminates the need for the custom
_countryBordersproperty.cypress/e2e/filtering.cy.js (3)
1-6: Add assertions to verify filtering behavior.The test only clicks the button but doesn't verify that the accepted filtering actually works. Consider adding assertions to check the filter state or visual changes.
Example:
describe('Accepted filtering', () => { it('test accepted filter button', function () { cy.visit('http://localhost:5173/#') cy.get('#doAcceptedFilter').click() // Verify the panel opened or filter was applied cy.get('#accepted_list_host').should('be.visible') cy.get('#accepted_legend').should('be.visible') }) })
8-22: Strengthen test with assertions and more reliable selectors.The test interactions lack assertions to verify the elevation control behavior, and the nth-child selectors are brittle.
Consider:
describe('Elevation Control', () => { it('test elevation control', function () { cy.visit('http://localhost:5173/#') cy.get('#btnDoElevationControl').click() cy.get('#elevation2_list').should('be.visible') // Use data-testid or more specific selectors cy.get('#elevation_gradient_scheme_selection').should('be.visible') // Verify slider exists and is interactive cy.get('#sldHeightRange').should('exist').click() // Verify point cloud material changed cy.window().then((win) => { const pc = win.potreeViewer?.scene?.pointclouds?.[0] expect(pc?.material?.activeAttributeName).to.equal('elevation') }) }) })
3-3: Use Cypress baseUrl instead of hardcoded URLs.Both tests hardcode the URL. Configure
baseUrlincypress.config.jsand usecy.visit('/')for better maintainability.In
cypress.config.js:export default defineConfig({ e2e: { baseUrl: 'http://localhost:5173' } })Then update tests:
- cy.visit('http://localhost:5173/#') + cy.visit('/')Also applies to: 10-10
src/cesiumViewer.js (1)
38-54: Consider configuration for the GeoJSON path and validate viewer parameter.The hardcoded path and custom property storage work but could be more robust.
Suggestions:
- Configuration for path: Move the GeoJSON path to config.js for easier customization.
// In config.js export const COUNTRY_BORDERS_GEOJSON_PATH = '/data/geo/world_simplified.geojson' // In cesiumViewer.js import { COUNTRY_BORDERS_GEOJSON_PATH } from './config.js' export async function loadCountryBorders(viewer) { if (!viewer) { throw new Error('Viewer parameter is required') } const dataSource = await Cesium.GeoJsonDataSource.load( COUNTRY_BORDERS_GEOJSON_PATH, // ...
- Standard property access: As mentioned in cameraSync.js review, consider using
viewer.dataSources.getByName('countryBorders')[0]instead of the custom_countryBordersproperty for consistency with Cesium's API.cypress/e2e/globeRendering.cy.js (2)
2-13: Replace hardcoded wait with proper state checks and fix meaningless assertion.The test has two issues:
cy.wait(6000)is an arbitrary delay that slows tests unnecessarily.expect(cv.scene.imageryLayers.length).to.be.at.least(0)is always true and doesn't validate anything useful.Improved version:
it('Check that the globe is rendered correctly', () => { cy.visit('http://localhost:5173/#') cy.window({ timeout: 10000 }).its('cesiumViewer').should('exist').then((cv) => { expect(cv.scene.globe.show).to.equal(true) // Check that imagery layers were properly configured // Since imageryProvider: false, we expect the collection to exist but be empty or have only added sources expect(cv.scene.imageryLayers).to.exist }) })
23-28: Add assertions to verify interaction results.The test clicks the canvas twice but has no assertions to verify the expected outcome. Without validation, the test only confirms the canvas exists and accepts clicks.
Add assertions to validate the interaction:
it('Test canvas interactions', () => { cy.visit('http://localhost:5173/#') cy.window({ timeout: 10000 }).its('potreeViewer').should('exist') cy.get('#potree_render_area canvas[tabindex="2222"]') .should('be.visible') .click() .click() // Verify interaction had an effect (e.g., camera moved, element selected, etc.) cy.window().then((win) => { // Example: verify camera position changed or element was selected expect(win.potreeViewer.scene.getActiveCamera()).to.exist }) })src/AcceptedFiltering/threePanels.css (1)
1-49: Add focus styles for keyboard accessibility.The button styles include hover and active states but are missing
:focusstyles for keyboard navigation, which is important for accessibility.Add focus styles:
#btnDoElevationControl:focus, #doAcceptedHost:focus, #btnTHU:focus, #btnTVU:focus, #btnTHUFilter:focus { outline: 2px solid #7ba8ff; outline-offset: 2px; } #btnDoElevationControl:focus:not(:focus-visible), #doAcceptedHost:focus:not(:focus-visible), #btnTHU:focus:not(:focus-visible), #btnTVU:focus:not(:focus-visible), #btnTHUFilter:focus:not(:focus-visible) { outline: none; }This ensures keyboard users can see which button is focused while not showing the outline for mouse users.
src/AcceptedFiltering/threePanels.js (6)
227-236: Add teardown to disconnect MutationObservers (avoid leaks).Observers are created but never disconnected; over time this can accumulate.
Apply:
@@ const byId = (id) => document.getElementById(id) +// Track observers for cleanup +const __panelObservers = [] @@ const obs = new MutationObserver(() => { if (byId('materials.elevation_container')) moveElevationContainer() }) obs.observe(root, { childList: true, subtree: true }) + __panelObservers.push(obs) } @@ 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() } }) obs.observe(root, { childList: true, subtree: true }) - return obs + __panelObservers.push(obs) + return obs @@ export function initThreePanels(viewer, hooks = {}) { @@ - attachSelfHealing(getActive) + attachSelfHealing(getActive) @@ // Default: auto-activate Elevation once clickOnce('btnDoElevationControl') + + // Expose teardown to callers (backwards compatible if ignored) + return function destroyThreePanels() { + __panelObservers.splice(0).forEach(o => { + try { o.disconnect() } catch {} + }) + } }Also applies to: 398-410, 418-439
122-133: Click the jstree anchor, not the icon.jstree selection is bound to the anchor; clicking the icon may not select the node.
- const icon = document.querySelector( - '#scene_objects i.jstree-themeicon-custom' - ) - if (icon) icon.dispatchEvent(new MouseEvent('click', { bubbles: true })) + const el = document.querySelector('#scene_objects a.jstree-anchor, #scene_objects i.jstree-themeicon-custom') + const target = el?.closest('a') || el + if (target) target.dispatchEvent(new MouseEvent('click', { bubbles: true }))
356-365: Treat hooks as possibly async; await them to avoid races.If hooks trigger Potree UI rebuilds asynchronously, not awaiting can cause flakiness.
- if (!src && typeof hooks?.onActivateElevation === 'function') { + if (!src && typeof hooks?.onActivateElevation === 'function') { // nudge: switch away and back if needed to force Potree to build the UI - hooks.onActivateAccepted?.() + await Promise.resolve(hooks.onActivateAccepted?.()) openScenePane() selectCloudNode(hooks) - hooks.onActivateElevation() + await Promise.resolve(hooks.onActivateElevation()) openScenePane() selectCloudNode(hooks) src = await waitForOrPoll('materials.elevation_container', 1800) } @@ - if (typeof hook === 'function') hook() + if (typeof hook === 'function') await Promise.resolve(hook())Also applies to: 383-385
30-37: Avoid innerHTML for static text; use DOM APIs.Safer and avoids accidental HTML injection if headerText ever becomes dynamic.
const header = document.createElement('h3') header.id = headerId - header.innerHTML = `<span>${headerText}</span>` + { + const span = document.createElement('span') + span.textContent = headerText + header.appendChild(span) + } @@ const panel = document.createElement('div') panel.className = 'pv-menu-list' - panel.innerHTML = `<div id="${listId}" class="auto"></div>` + { + const inner = document.createElement('div') + inner.id = listId + inner.className = 'auto' + panel.appendChild(inner) + }
79-85: Improve a11y when toggling bodies.Use the hidden attribute and update headers’ aria-expanded for screen readers.
- if (elevBody) elevBody.style.display = key === 'elevation' ? '' : 'none' - if (accBody) accBody.style.display = key === 'accepted' ? '' : 'none' + if (elevBody) elevBody.hidden = key !== 'elevation' + if (accBody) accBody.hidden = key !== 'accepted' + byId('menu_elevation')?.setAttribute('aria-expanded', String(key === 'elevation')) + byId('menu_accepted')?.setAttribute('aria-expanded', String(key === 'accepted'))
136-141: Typos in comments.“wverything” → “everything”; “Does appearnlty nor work” → “Apparently does not work”.
Also applies to: 315-318
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cypress/e2e/filtering.cy.js(1 hunks)cypress/e2e/globeRendering.cy.js(1 hunks)index.html(1 hunks)src/AcceptedFiltering/threePanels.css(1 hunks)src/AcceptedFiltering/threePanels.js(1 hunks)src/ElevationControl/elevationControl.js(0 hunks)src/cameraSync.js(1 hunks)src/cesiumViewer.js(1 hunks)src/main.js(2 hunks)src/potreeViewer.js(5 hunks)
💤 Files with no reviewable changes (1)
- src/ElevationControl/elevationControl.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/main.js (1)
src/cesiumViewer.js (1)
loadCountryBorders(38-54)
src/potreeViewer.js (1)
src/AcceptedFiltering/threePanels.js (5)
initThreePanels(418-439)toggleAcceptedLegend(309-312)icon(129-131)el(145-145)active(424-424)
🔇 Additional comments (9)
index.html (1)
41-41: LGTM!The CSS link addition follows the existing pattern and correctly references the new threePanels stylesheet.
src/cesiumViewer.js (1)
20-25: LGTM!The imagery provider removal and custom globe base color are valid Cesium configuration changes that support the new visualization approach.
src/main.js (1)
2-2: LGTM! Good error handling.The try/catch block ensures the application continues to initialize even if country borders fail to load, which is the correct approach for an optional visual enhancement.
Also applies to: 13-17
src/AcceptedFiltering/threePanels.css (1)
157-163: LGTM!The legend color choices (white for accepted, black for not-accepted) provide clear visual distinction and good contrast.
src/potreeViewer.js (3)
2-5: LGTM!The import changes correctly reflect the architectural shift from ElevationControl to the new threePanels system.
45-66: LGTM! Well-structured callback integration.The callbacks properly guard against missing point cloud and use appropriate material attribute updates. The integration with
suppressSidebarAutoScrollprevents unwanted scroll behavior during UI updates.
83-86: LGTM!The comment clarifies the default visualization settings and the code correctly initializes elevation with VIRIDIS gradient.
src/AcceptedFiltering/threePanels.js (2)
418-424: Ensure #potree_menu exists before building sections.If initThreePanels runs before Potree mounts the accordion, sections won’t be inserted. Either guarantee call order or wait for the menu.
If not guaranteed, consider:
-export function initThreePanels(viewer, hooks = {}) { +export async function initThreePanels(viewer, hooks = {}) { + await waitForOrPoll('potree_menu', 3000)This is a behavior change (async); confirm call sites can await. Otherwise, attach a one‑time observer to build when the menu appears.
1-4: Scope check vs issue #8 — incorrect: TVU/THU and point‑source‑ID support exist elsewhere in the repo.Repo search shows THU/TVU UI and point‑source‑ID handling outside this module (examples: src/AcceptedFiltering/threePanels.css; public/build/potree/** contains "source id", shader define
clip_point_source_id_enabled, and decoder/worker code for source IDs). threePanels.js focusing on Elevation + Accepted UI is consistent with that layout — the original comment’s implication that those features are missing from the repo is incorrect.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Great work! Left some comments. Not sure if having a whole section for just accepted filtering is necessary, as the section can fell somewhat empty, but this is maybe something to consider for later? Not really a big problem having it in a separate section either. One thing that I did find though is that I am not able to close the Elevation Control and Accepted Filtering sections anymore, like I am able to for the other sections.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/potreeViewer.js (3)
67-70: Handle point cloud load failures gracefully.Uncaught rejection will crash initialization. Wrap in try/catch and abort dependent setup on failure.
Apply:
- const e = await Potree.loadPointCloud(pointcloudUrl) - const pc = e.pointcloud - viewer.scene.addPointCloud(pc) + try { + const e = await Potree.loadPointCloud(pointcloudUrl) + pc = e.pointcloud + viewer.scene.addPointCloud(pc) + } catch (error) { + console.error('Failed to load point cloud:', error) + throw new Error(`Point cloud loading failed: ${error?.message || error}`) + }
41-55: ReferenceError risk:pcused before declaration during auto-activation.
clickOnce('btnDoElevationControl')triggersonActivateElevationbeforeconst pc = …runs, hitting TDZ and crashing onif (!pc) return.Apply:
export async function createPotreeViewer(containerId, pointcloudUrl, settings) { const viewer = new Potree.Viewer(document.getElementById(containerId), { useDefaultRenderLoop: false }) + // Hoist point cloud reference so hooks can safely check it before load completes + let pc = null viewer.loadGUI(() => { @@ - onActivateElevation: () => { - if (!pc) return + onActivateElevation: () => { + if (!pc) return pc.material.activeAttributeName = 'elevation' pc.material.gradient = Potree.Gradients['VIRIDIS'] suppressSidebarAutoScroll(clickCloudIconOnce) }, onActivateAccepted: () => { - if (!pc) return + if (!pc) return pc.material.activeAttributeName = 'accepted' - pc.material.gradient = Potree.Gradients['GRAYSCALE'] - toggleAcceptedLegend(true) + pc.material.gradient = Potree.Gradients['GRAYSCALE'] suppressSidebarAutoScroll(clickCloudIconOnce) } }) @@ - const e = await Potree.loadPointCloud(pointcloudUrl) - const pc = e.pointcloud - viewer.scene.addPointCloud(pc) + let load + try { + load = await Potree.loadPointCloud(pointcloudUrl) + pc = load.pointcloud + viewer.scene.addPointCloud(pc) + } catch (error) { + console.error('Failed to load point cloud:', error) + throw new Error(`Point cloud loading failed: ${error?.message || error}`) + }Note: I also removed the duplicate
toggleAcceptedLegend(true)here;switchModealready controls legend visibility. See below.Also applies to: 67-70
169-259: Guard global prototype overrides; prevent overlap and reduce a11y impact.
suppressSidebarAutoScrollmodifies Element/HTMLElement prototypes without guarding concurrent calls; overlapping invocations can leave the page in a broken state. Also, unconditional blur harms keyboard users.Apply:
+let suppressionActive = false function suppressSidebarAutoScroll(action, holdMs = 350) { + if (suppressionActive) { + // Already suppressing; run action without altering prototypes again + action() + return + } + suppressionActive = true @@ try { action() } finally { @@ - const active = document.activeElement - if (active && active.closest && active.closest('#scene_objects')) { - active.blur() - } + // Only blur if the jsTree container would otherwise change scroll + const active = document.activeElement + const container = + active && active.closest ? active.closest('.jstree-container-ul') : null + if (container) { + const s = states.find((x) => x.el === container) + if (s && (container.scrollTop !== s.top || container.scrollLeft !== s.left)) { + active.blur() + } + } @@ states.forEach(({ el, overflow }) => { const h = handlers.get(el) if (h) el.removeEventListener('scroll', h) el.style.overflow = overflow }) + suppressionActive = false } } requestAnimationFrame(restoreLoop) } }src/AcceptedFiltering/threePanels.js (1)
187-203: Fix event binding and avoid clobbering Potree/user slider state.
- Wrong
.onsignature: second arg is treated as a selector;slidestop/changewon’t fire.- Reinitializing slider resets range/values, wiping Potree config and user choices. Initialize only if not already initialized, and preserve values if present.
Apply:
function rebindElevationLabel() { 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)}` } - 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) + // Initialize only if needed; do not overwrite existing options/values + let values = [-10000, 0] + try { + const low = slider.slider('values', 0) + const high = slider.slider('values', 1) + if (typeof low === 'number' && typeof high === 'number') values = [low, high] + } catch {} + if (!slider.slider('instance')) { + slider.slider({ min: -10000, max: 0, values }) + } + slider.off('slide.custom slidestop.custom change.custom') + slider.on('slide.custom slidestop.custom change.custom', update) update() }
🧹 Nitpick comments (3)
src/AcceptedFiltering/threePanels.js (2)
136-140: Doc typo.“wverything” → “everything”. Also consider simplifying the sentence.
- * 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.
314-317: Doc typo.“appearnlty” → “apparently”; optional wording tweak.
- * 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 required by Potree menu CSS/JS).src/potreeViewer.js (1)
48-54: Avoid duplicate legend toggling; let switchMode own it.
switchModealready callstoggleAcceptedLegend(mode === 'accepted'). Keeping it here may cause flicker or inconsistency.- toggleAcceptedLegend(true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/e2e/filtering.cy.js(1 hunks)src/AcceptedFiltering/threePanels.css(1 hunks)src/AcceptedFiltering/threePanels.js(1 hunks)src/potreeViewer.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/AcceptedFiltering/threePanels.css
- cypress/e2e/filtering.cy.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/AcceptedFiltering/threePanels.js (1)
src/potreeViewer.js (2)
el(178-178)viewer(17-19)
src/potreeViewer.js (1)
src/AcceptedFiltering/threePanels.js (5)
initThreePanels(417-440)toggleAcceptedLegend(308-311)icon(129-131)el(145-145)active(423-423)
🔇 Additional comments (1)
src/potreeViewer.js (1)
41-56: Nice integration of the three-panel hooks.Clean separation of mode hooks and UI wiring; the self-healing plus centralized
switchModekeeps rerenders in check.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/potreeViewer.js (2)
48-54: Use the correct Potree gradient key.
Potree.Gradients['GRAYSCALE']is invalid. Potree uses'greys'or'Greys'for grayscale gradients. This issue was flagged in a previous review.Apply this diff:
onActivateAccepted: () => { if (!pc) return pc.material.activeAttributeName = 'accepted' - pc.material.gradient = Potree.Gradients['GRAYSCALE'] + pc.material.gradient = Potree.Gradients['greys'] || Potree.Gradients['Greys'] || Potree.Gradients['VIRIDIS'] toggleAcceptedLegend(true) suppressSidebarAutoScroll(clickCloudIconOnce) }
163-260: Prototype modification and accessibility concerns remain unaddressed.This function still has the critical issues identified in previous reviews:
- No concurrency guard: Overlapping calls can interfere with each other's prototype restoration
- Global prototype modification: Affects the entire page and could break other code
- Unconditional blur (lines 240-243): May disrupt keyboard navigation for accessibility users
These issues were flagged in earlier reviews and have not been resolved.
Please address the concerns from previous reviews, particularly:
- Add a module-scoped flag to prevent concurrent suppression
- Consider jsTree configuration options before resorting to prototype modification
- Make the blur conditional based on whether it would actually prevent scroll
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.html(1 hunks)src/potreeViewer.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- index.html
🧰 Additional context used
🧬 Code graph analysis (1)
src/potreeViewer.js (1)
src/AcceptedFiltering/threePanels.js (5)
initThreePanels(417-440)toggleAcceptedLegend(308-311)icon(129-131)el(145-145)active(423-423)
🔇 Additional comments (2)
src/potreeViewer.js (2)
2-5: LGTM! Clean refactoring of imports.The transition from the old elevationControl module to the new threePanels architecture is well-structured, and both imports are used correctly in the initialization flow.
83-87: LGTM! Well-documented initialization defaults.Setting elevation as the default visualization mode is consistent with the three-panel UI behavior, and the comment clearly explains the reasoning.
Issue number
Closes #8
Description
Vreated a Potree sidebar sections with buttons + panel bodies:
Elevation → moves #materials.elevation_container into the Elevation body
Accepted → custom UI for accepted filtering
Testing steps
Try alternating between accepted filtering and elevation control. Also try to use measurements and other tools in between so we ensure consistency.
Screenshots (optional)
Summary by CodeRabbit
New Features
Style
Tests