feat(cesium): support CZML dynamic 3D scenes on the globe - #2350
feat(cesium): support CZML dynamic 3D scenes on the globe#2350RohithPariki wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (14)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe PR adds CZML support across the core layer model, desktop Add Data dialog, Cesium renderer, Python API, localization, and automated tests. CZML can use remote URLs, local files, inline packets, or sample scenes. ChangesCZML support
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to CZML creation, desktop selection, Cesium loading, visibility, error handling, teardown, and Python layer generation are covered by the supplied tests and integration summaries. No actionable merge risk remains. Sequence Diagram(s)sequenceDiagram
participant Desktop as AddDataDialog
participant Core as createCzmlLayer
participant Sync as CesiumLayerSync
participant Loader as CzmlDataSource
participant Viewer as Cesium viewer
Desktop->>Core: Create layer from URL, file, or sample
Core->>Sync: Submit CZML layer
Sync->>Loader: Load CZML document
Loader-->>Sync: Return data source and clock
Sync->>Viewer: Add source and synchronize clock
Desktop->>Sync: Toggle layer visibility
Sync->>Viewer: Update data source visibility
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 78.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 13 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
apps/geolibre-desktop/src/components/layout/AddDataDialog.tsxtypescript-eslint does not support TS 7.0. Oops! Something went wrong! :( ESLint: 10.10.0 Error: typescript-eslint does not support TS 7.0. apps/geolibre-desktop/src/components/layout/add-data/constants.tsESLint skipped: the matched ESLint configuration already failed (config-incompatibility). apps/geolibre-desktop/src/components/layout/add-data/sources/CzmlSource.tsxESLint skipped: the matched ESLint configuration already failed (config-incompatibility).
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit hops where CZML flies Comment |
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!target) return; | ||
|
|
||
| try { | ||
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); |
There was a problem hiding this comment.
Bug: source.data can be a string (per CzmlLayerOptions.data/czml's own JSDoc: "Inlined CZML packet array or serialized JSON string"), but it's passed straight through to Cesium.CzmlDataSource.load(). Cesium's CzmlDataSource.load treats a string argument as a URL to fetch, not as inline CZML text (that's exactly why the url case works). So any caller that builds a layer with data as a JSON string (a documented, supported input shape) will have Cesium try to fetch() the raw JSON text as if it were a URL and fail.
The current desktop UI happens to always pre-parse file contents via parseCzml() before calling createCzmlLayer, so this path isn't hit today, but it's a latent bug for any other caller (hand-authored .geolibre.json, a future MCP tool, a plugin) that uses the string form.
Suggested fix: normalize source.data through parseCzml() (or JSON.parse) when it's a string, before handing it to Cesium:
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); | |
| const rawData = source.data; | |
| const parsedData = | |
| typeof rawData === "string" ? (JSON.parse(rawData) as object) : rawData; | |
| const target = parsedData ?? source.url; | |
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); |
(adjust variable placement as needed; the key point is: don't hand a raw JSON string of packets to CzmlDataSource.load.)
| source: { | ||
| type: "3d-tiles", | ||
| sourceId: id, | ||
| ...(data !== undefined ? { czmlData: data, czml: data } : {}), |
There was a problem hiding this comment.
Storing the same payload under both czmlData and czml doubles the serialized size of every CZML layer in the saved .geolibre.json project for no functional benefit — czmlSource() only ever reads czmlData ?? czml, so czml is dead weight once czmlData is set (and the Python builder only ever writes czmlData, never czml). For large inline documents (satellite constellations, dense trajectories) this meaningfully bloats project files. Consider dropping the czml alias entirely, or only ever writing one key.
| Cesium3DTileset, | ||
| CesiumWidget, | ||
| Color, | ||
| CzmlDataSource, |
There was a problem hiding this comment.
Nit: CzmlDataSource is imported as a type but never referenced elsewhere in the file — entry.handle is typed as the broader DataSource union and Cesium.CzmlDataSource.load(...)'s return type is inferred from the Cesium namespace parameter, not from this import. Looks like dead code left over from an earlier draft.
| case "czml": | ||
| return ( | ||
| czmlSource(prev)?.url !== czmlSource(next)?.url || | ||
| czmlSource(prev)?.data !== czmlSource(next)?.data || |
There was a problem hiding this comment.
czmlSource(prev)?.data !== czmlSource(next)?.data compares the inline packet array/object by reference. Other array/object fields elsewhere in this same needsRebuild switch (e.g. requestHeaders, tileMatrixLabels) are compared via JSON.stringify(...) !== JSON.stringify(...) specifically to avoid spurious rebuilds when an unrelated store update produces a structurally-identical-but-new object. If anything upstream (undo/redo, an immer produce, a project reload) ever reconstructs layer.source without literally reusing the same czmlData array reference, this will force a full CZML data-source teardown/reload (restarting the clock/animation) even though nothing about the CZML content changed. Low confidence this is hit in practice today, but worth confirming source.czmlData's reference stability across incidental layer updates (opacity, rename, visibility toggle already look safe since they spread layer without touching source).
| """``metadata.sourceKind`` of a layer that references a CZML dynamic scene.""" | ||
|
|
||
|
|
||
| def czml_layer( |
There was a problem hiding this comment.
The PR description states this PR adds "Map.add_czml in geolibre.py" and "add_czml_layer in the MCP server," mirroring the existing cesium_ion_layer → Map.add_cesium_ion (geolibre.py:2673) → add_cesium_ion_layer MCP tool (mcp/server.py:693) chain. Neither geolibre.py nor mcp/server.py appear in this diff, and neither file contains any czml reference in the current tree. As it stands, czml_layer() is a standalone builder with no Map method and no MCP tool wired to it, so CZML layers can't actually be authored through the Python Map API or the MCP server despite the PR description's claim — only by hand-building a layer dict. Worth confirming whether this was meant to ship in a follow-up PR, since CLAUDE.md's MCP layering convention (project.py builds → authoring.py/Map applies → MCP tools delegate) implies these three land together.
Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
| if (!target) return; | ||
|
|
||
| try { | ||
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); |
There was a problem hiding this comment.
source.data can be a string (per CzmlLayerOptions.data/czml's JSDoc: "Inlined CZML packet array or serialized JSON string"), but it's passed straight to Cesium.CzmlDataSource.load() here. Cesium's CzmlDataSource.load() treats a string argument as a URL/Resource to fetch, not as inline JSON text — it does not JSON.parse it for you. So a caller that supplies data as a raw CZML JSON string (a supported input per the type) will have that string misinterpreted as a URL and fail to load instead of rendering the inline document.
The Add Data dialog never hits this today because CzmlSource.tsx always runs file/URL input through parseCzml() first, which returns an array — but any other caller of the exported createCzmlLayer({ data: someJsonString }) API will silently break.
Consider normalizing the string case before calling load, e.g.:
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); | |
| const raw = typeof source.data === "string" ? (parseCzml(source.data) ?? source.data) : source.data; | |
| const target = raw ?? source.url; | |
| if (!target) return; | |
| const dataSource = await Cesium.CzmlDataSource.load(target as string | object); |
(would need to import parseCzml from @geolibre/core, already imported as czmlSource in this file).
| clock?: { | ||
| startTime?: unknown; | ||
| stopTime?: unknown; | ||
| currentTime?: unknown; | ||
| clockRange?: unknown; | ||
| multiplier?: unknown; | ||
| }; | ||
| } | ||
| ).clock; | ||
|
|
||
| if (dsClock && viewer.clock) { | ||
| if (dsClock.startTime) viewer.clock.startTime = dsClock.startTime as never; | ||
| if (dsClock.stopTime) viewer.clock.stopTime = dsClock.stopTime as never; | ||
| if (dsClock.currentTime) viewer.clock.currentTime = dsClock.currentTime as never; | ||
| if (dsClock.clockRange !== undefined) viewer.clock.clockRange = dsClock.clockRange as never; | ||
| if (dsClock.multiplier !== undefined) viewer.clock.multiplier = dsClock.multiplier as never; | ||
| } |
There was a problem hiding this comment.
Loading a CZML layer with a clock packet unconditionally stomps the shared viewer.clock (start/stop/current time, range, multiplier) with no coordination:
- The globe already has a Time Slider that drives
viewer.clock.currentTimeviaCesiumLayerSync.setTime(). Adding a CZML layer (or having one finish loading async) after the user has scrubbed the Time Slider will silently jump the clock back to the CZML document's own interval. - Loading a second CZML layer with a different
clockinterval will silently override the first layer's clock again — there's no "owner" concept, so whichever CZML layer's load resolves last wins, and the two layers' animations end up desynchronized from what either document intended.
Worth gating this behind something (e.g. only sync the clock for the first/primary CZML layer, or only when no other time-driving source — Time Slider — is active), or at least documenting the "last CZML layer to load wins" behavior as intentional.
| source: { | ||
| type: "3d-tiles", | ||
| sourceId: id, | ||
| ...(data !== undefined ? { czmlData: data, czml: data } : {}), |
There was a problem hiding this comment.
The inline document is stored twice — under both czmlData and czml — but czmlSource() only ever reads czmlData first, falling back to czml (layer.source?.czmlData ?? layer.source?.czml). Since a layer is always built through createCzmlLayer, czmlData is always present when data is supplied, so czml is dead weight here: it doubles the serialized size of every CZML layer with inline data in .geolibre.json (and in the in-memory store) for no functional benefit — potentially significant for a real orbit/trajectory document with many packets.
| ...(data !== undefined ? { czmlData: data, czml: data } : {}), | |
| ...(data !== undefined ? { czmlData: data } : {}), |
| <Label>{t("addData.czml.file")}</Label> | ||
| <div className="flex items-center gap-2"> | ||
| <Button type="button" variant="outline" size="sm" onClick={handleChooseFile}> | ||
| <FileUp className="mr-1.5 h-4 w-4" /> |
There was a problem hiding this comment.
Physical Tailwind utility (mr-1.5) instead of the logical form. CLAUDE.md requires logical utilities (ms-/me-/...) for RTL support, and every other Add Data source's FileUp icon in this directory uses me-2 (see CadSource.tsx, PolylineSource.tsx, MbtilesSource.tsx, DeckVizSource.tsx, DelimitedTextSource.tsx, GpxSource.tsx, GeoRssSource.tsx). This one won't mirror correctly in RTL locales.
| <FileUp className="mr-1.5 h-4 w-4" /> | |
| <FileUp className="me-2 h-4 w-4" /> |
| Cesium3DTileset, | ||
| CesiumWidget, | ||
| Color, | ||
| CzmlDataSource, |
There was a problem hiding this comment.
CzmlDataSource is imported as a type but never referenced — the loaded data source is typed/cast via DataSource everywhere else in this file (e.g. entry.handle as DataSource, the dataSource result of Cesium.CzmlDataSource.load(...) is used untyped). This looks like dead code left over from an earlier draft; worth removing unless there's a planned use.
| @@ -0,0 +1,42 @@ | |||
| """CZML dynamic 3D scene layers (issue #2290): builders, the Map API, and the MCP tool.""" | |||
There was a problem hiding this comment.
This docstring (and the PR description) says CZML support covers "builders, the Map API, and the MCP tool," but this diff only adds project.czml_layer — there's no Map.add_czml in geolibre.py and no add_czml_layer tool registered in the MCP server (python/src/geolibre/mcp/). Per CLAUDE.md's description of the Python/MCP layering ("Map and the MCP tools delegate to authoring.py"), a builder in project.py alone doesn't make CZML layers authorable from the Map API or MCP — those integration points appear to be missing from this PR despite being claimed as delivered.
Code reviewBugs
Quality
CLAUDE.md
No SQL/command-injection, secret-leak, or clear performance-hotspot issues found beyond the data-duplication item above. Vector/legend/screenshot-readiness code that keys off |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Problem
As tracked in #2290 (and #2259), GeoLibre's Cesium 3D globe lacks native support for loading time-dynamic CZML (Cesium Language) documents describing orbits, vehicular trajectories, moving 3D entities, paths, and clock-synchronized animations. Users working with dynamic geospatial feeds, satellite telemetry, or time-varying 3D scenes could not load CZML documents or sync them with the Cesium globe's clock.
Root Cause
CesiumLayerSynconly recognized static GeoJSON, 3D Tiles URL endpoints, Ion assets (#2321), and point clouds, but did not interface with Cesium'sCzmlDataSource. Furthermore, neither@geolibre/corenor Desktop UI had domain types or layer builders to author or validate CZML documents or expose them in Add Data.Solution
Implemented native CZML layer support across core, map synchronizer, desktop UI, and Python/MCP tooling:
@geolibre/core(czml.ts):CZML_SOURCE_KIND = "czml",createCzmlLayer,isCzmlLayer,czmlSource, andparseCzmlwith complete JSDoc documentation.CZML_QUICK_PICKS(Point and dynamic Orbit trajectory samples).isCesiumOnlyLayerincesium-ion.tsso CZML layers are appropriately badged "3D only" in the 2D pane.@geolibre/map(cesium-layer-sync.ts):"czml"entry kind and registered it inisCesiumSupportedLayerType,isSupported,entryKind, andisSettled.createCzmlto asynchronously load CZML documents viaCesium.CzmlDataSource.load(...), bind dataSource visibility, synchronize the globe clock (viewer.clock) with document clock packets (startTime,stopTime,currentTime,clockRange,multiplier), and add toviewer.dataSources.destroyEntry.Desktop UI (
apps/geolibre-desktop):CzmlSource.tsxsupporting URL endpoints, local.czml/.jsonfile picker, and sample quick picks.czmlinAddDataDialog,AddDataMenu(under 3D, disabled when on 2D map), constants, types, UI catalog, and English localization (en.json).Python & MCP (
python/src/geolibre):czml_layerhelper inproject.py,Map.add_czmlingeolibre.py, andadd_czml_layerin the MCP server.Testing
tests/czml.test.tscovering:parseCzmlparsing and validation of JSON strings and packet objectsCesiumLayerSyncloading, clock synchronization, visibility toggling, error reporting ingetRenderStatus, and teardownpython/tests/test_czml.pytesting Python layer creation and Map integration.node --import tsx --test tests/czml.test.ts tests/cesium-ion.test.ts tests/cesium-3d-tiles-style.test.ts(14/14 tests pass).npm run i18n:tools:check(passes).Risk
None. CZML layers are isolated behind
metadata.sourceKind === "czml"and marked external native layers. The 2D MapLibre renderer leaves them untouched, and existing 3D Tiles and Cesium Ion workflows are completely unaffected.Issue
Closes #2290
Refs #2259
Summary by CodeRabbit