docs: Add '3D Buildings' Example #80
Conversation
ciscorn
added
documentation
Deploying svelte-maplibre-gl with
|
| Latest commit: |
b14a4b6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://111ae8f3.svelte-maplibre-gl.pages.dev |
| Branch Preview URL: | https://example-3d-buildings.svelte-maplibre-gl.pages.dev |
commit: |
WalkthroughThis pull request encompasses a comprehensive update to the Key modifications include updates to package.json with version upgrades for various dependencies, introduction of new example components like 3D Buildings and Globe with Atmosphere, and refinements to existing components such as Terrain and Custom Layer. Several markdown files were updated with metadata improvements, including adding original source links and adjusting descriptions. The project also saw improvements in map component interfaces, with additions like new properties for MapLibre and Terrain components, and enhancements to custom layer implementations. The changes reflect ongoing development to improve the library's functionality, documentation, and user experience across different mapping scenarios. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (51)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (29)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (20)
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (12)
src/content/examples/globe-atmosphere/Globe.svelte (3)
4-17: Enhance code readability with documentation and better naming.The mathematical calculations are complex and would benefit from:
- Documentation explaining the purpose and mathematical basis of each formula
- More descriptive variable names (e.g.,
thetaDegrees,phiDegreesinstead ofthetaDeg,phiDeg)- Constants for magic numbers (e.g.,
180,Math.PI)Here's a suggested improvement:
-let thetaDeg = $state(135); -let phiDeg = $state(0); -let p = $derived( - (Math.acos(Math.cos((thetaDeg / 180 + 1) * Math.PI) * Math.cos((phiDeg / 180) * Math.PI)) / Math.PI) * 180 -); -let a = $derived( +// Constants for angle conversions +const DEG_TO_RAD = Math.PI / 180; +const RAD_TO_DEG = 180 / Math.PI; + +// Theta angle for light source position (horizontal rotation) +let thetaDegrees = $state(135); + +// Phi angle for light source position (vertical rotation) +let phiDegrees = $state(0); + +// Calculate polar angle (p) for spherical coordinates +let polarAngle = $derived( + RAD_TO_DEG * Math.acos( + Math.cos((thetaDegrees * DEG_TO_RAD) + Math.PI) * + Math.cos(phiDegrees * DEG_TO_RAD) + ) +); + +// Calculate azimuthal angle (a) for spherical coordinates +let azimuthalAngle = $derived(
20-29: Consider externalizing configuration values.The MapLibre setup is well-structured, but consider:
- Moving the style URL to a configuration constant or environment variable
- Extracting the atmosphere-blend interpolation array for better readability
Example refactor:
+const MAP_STYLE_URL = 'https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json'; +const ATMOSPHERE_BLEND = ['interpolate', ['linear'], ['zoom'], 0, 1, 5, 1, 7, 0]; + <MapLibre class="h-[55vh] min-h-[300px]" - style="https://basemaps.cartocdn.com/gl/dark-matter-gl-style/style.json" + style={MAP_STYLE_URL} zoom={1.5} center={{ lng: 155, lat: 45 }} > <Projection type="globe" /> <Light anchor="map" position={[100, a, p]} /> - <Sky atmosphere-blend={['interpolate', ['linear'], ['zoom'], 0, 1, 5, 1, 7, 0]} /> + <Sky atmosphere-blend={ATMOSPHERE_BLEND} />
30-42: Well-implemented accessible controls with room for enhancement.The control panel is well-structured with proper accessibility features. Consider adding:
titleattributes to provide additional context when hovering over the controlsaria-valuemin,aria-valuemax, andaria-valuenowfor better screen reader supportExample enhancement:
<input type="range" id="theta" bind:value={thetaDeg} min={-180} max={180} + title="Adjust horizontal rotation of the light source" + aria-valuemin="-180" + aria-valuemax="180" + aria-valuenow={thetaDeg} />src/content/examples/threejs-model/CustomLayer.svelte (2)
12-12: Ensure 'this.map' is safely initialized before usageThe
this.mapproperty is initialized in theonAddmethod and used in therendermethod with non-null assertions (this.map!). Ensure thatonAddis always called beforerender, and consider adding checks or assertions to guarantee thatthis.mapis not null to prevent potential runtime errors.Also applies to: 15-15
48-55: Consider externalizing model parameters for flexibilityThe hardcoded
modelOrigin,modelAltitude, andscalingvalues in therendermethod limit the ability to reuseCustomLayerImplwith different models or locations. Consider externalizing these parameters to allow dynamic configuration and enhance the reusability of the class.Apply this diff to refactor the code:
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { + constructor( + private modelOrigin: [number, number], + private modelAltitude: number, + private scaling: number + ) {} // ... render(_gl: WebGL2RenderingContext | WebGLRenderingContext, args: maplibregl.CustomRenderMethodInput) { - const modelOrigin: [number, number] = [148.9819, -35.39847]; - const modelAltitude = 0; - const scaling = 10_000.0; // ... - const modelMatrix = this.map!.transform.getMatrixForModel(modelOrigin, modelAltitude); + const modelMatrix = this.map!.transform.getMatrixForModel(this.modelOrigin, this.modelAltitude); // ... - const l = new THREE.Matrix4().fromArray(modelMatrix).scale(new THREE.Vector3(scaling, scaling, scaling)); + const l = new THREE.Matrix4().fromArray(modelMatrix).scale(new THREE.Vector3(this.scaling, this.scaling, this.scaling)); // ... } } - const customLayerImpl = new CustomLayerImpl(); + const customLayerImpl = new CustomLayerImpl([148.9819, -35.39847], 0, 10_000.0);src/content/examples/terrain/Terrain.svelte (1)
17-19: Remove unused 'terrain' variable and UI controlsThe
terrainvariable is declared and manipulated via the UI slider but is not utilized in the code. Since<TerrainControl />provides interactive control over terrain exaggeration, the separateterrainvariable and its associated UI elements are redundant. Consider removing them to streamline the code and avoid confusion.Apply this diff to remove the unused
terrainvariable and UI elements:// Terrain -let hillshade = $state(0.7); -let terrain = $state(1.0); +let hillshade = $state(0.7); // ... <Tabs.Content value="terrain" class="min-h-0 shrink overflow-scroll pt-1"> - <div class="mb-4 flex flex-col items-center space-y-2 px-2"> - <Label for="exaggeration" class="leading-none">Exaggeration</Label> - <Slider type="single" id="exaggeration" min={0} max={2} step={0.01} /> - </div> <div class="mb-3 flex flex-col items-center space-y-2 px-2"> <Label for="hillshade" class="leading-none">Hillshade ({hillshade.toFixed(2)})</Label> <Slider type="single" id="hillshade" bind:value={hillshade} min={0} max={1} step={0.01} /> </div> <!-- Remaining content --> </Tabs.Content>Also applies to: 96-98
src/content/examples/custom-layer/CustomLayer.svelte (1)
44-77: Add error handling for shader compilation and program linkingThe current implementation lacks error checking after shader compilation and program linking. This can make debugging difficult if there are issues with the shaders. Consider adding error handling using
gl.getShaderInfoLogandgl.getProgramInfoLogto capture and log errors, enhancing the robustness of the application.Apply this diff to include error handling:
gl.shaderSource(vertexShader, vertexSource); gl.compileShader(vertexShader); + if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { + console.error('Vertex shader compilation error:', gl.getShaderInfoLog(vertexShader)); + } // Create fragment shader const fragmentShader = gl.createShader(gl.FRAGMENT_SHADER)!; gl.shaderSource(fragmentShader, fragmentSource); gl.compileShader(fragmentShader); + if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { + console.error('Fragment shader compilation error:', gl.getShaderInfoLog(fragmentShader)); + } // Link the two shaders into a WebGL program const program = gl.createProgram(); gl.attachShader(program, vertexShader); gl.attachShader(program, fragmentShader); gl.linkProgram(program); + if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { + console.error('Program linking error:', gl.getProgramInfoLog(program)); + }src/routes/examples/[slug]/+page.ts (1)
23-24: Consider providing default values for optional metadata.While making
descriptionoptional and addingoriginalimproves flexibility, consider providing default values to maintain a consistent user experience when these fields are missing.metadata: { title: string; - description?: string; - original?: string; + description: string = ''; + original: string = ''; };src/content/examples/3d-buildings/Buildings3D.svelte (1)
20-30: Consider color accessibility in the height-based color scheme.The current color interpolation (
#aaccbb→royalblue→purple) might not be easily distinguishable for colorblind users. Consider:
- Using a more accessible color palette
- Adding patterns or textures for better differentiation
- Documenting the color meaning in the UI
src/content/CodeBlock.svelte (1)
20-28: LGTM! Consider adding aria-label to links.The layout changes look good and the added context about styling libraries is helpful. Consider adding aria-labels to the links for better accessibility.
- <a href="https://tailwindcss.com/">Tailwind CSS</a> and - <a href="https://next.shadcn-svelte.com/">shadcn-svelte</a>. + <a href="https://tailwindcss.com/" aria-label="Visit Tailwind CSS website">Tailwind CSS</a> and + <a href="https://next.shadcn-svelte.com/" aria-label="Visit shadcn-svelte website">shadcn-svelte</a>.src/content/examples/hover-styles/HoverStyles.svelte (2)
21-26: Consider debouncing mousemove events.Frequent mousemove events might impact performance. Consider adding debounce.
+ function debounce<T extends (...args: any[]) => any>( + fn: T, + ms = 100 + ): (...args: Parameters<T>) => void { + let timeoutId: ReturnType<typeof setTimeout>; + return (...args: Parameters<T>) => { + clearTimeout(timeoutId); + timeoutId = setTimeout(() => fn(...args), ms); + }; + } + + const handleMouseMove = debounce((ev: any) => { + hoveredFeature = ev.features?.[0]; + lnglat = ev.lngLat; + }, 50); - onmousemove={(ev) => { - hoveredFeature = ev.features?.[0]; - lnglat = ev.lngLat; - }} + onmousemove={handleMouseMove}
36-40: Consider adding loading state for popup.The popup might flash if the feature data is loading. Consider adding a loading state.
{#if hoveredFeature} <FeatureState id={hoveredFeature.id} state={{ hover: true }} /> - <Popup {lnglat} closeButton={false}>{hoveredFeature.properties.STATE_NAME}</Popup> + <Popup {lnglat} closeButton={false}> + {#if hoveredFeature.properties?.STATE_NAME} + {hoveredFeature.properties.STATE_NAME} + {:else} + Loading... + {/if} + </Popup> {/if}
🛑 Comments failed to post (3)
src/content/examples/3d-extrusion-floorplan/Floorplan.svelte (2)
14-16: 🛠️ Refactor suggestion
Add error handling for OpenStreetMap tile loading.
The tile source should handle loading failures gracefully. Consider adding error handling and a fallback tile source.
<RasterTileSource tiles={['https://tile.openstreetmap.org/{z}/{x}/{y}.png']} tileSize={256} minzoom={0} maxzoom={19} + on:error={(e) => { + console.error('Tile loading error:', e); + // TODO: Switch to fallback tile source + }} >Committable suggestion skipped: line range outside the PR's diff.
17-30: 🛠️ Refactor suggestion
Add error handling and loading state for external GeoJSON data.
The GeoJSON source loads data from an external URL without error handling or loading indicators. Consider:
- Adding error handling for failed data loads
- Implementing a loading state
- Validating the GeoJSON data structure
+<script lang="ts"> + let dataError = false; + let isLoading = true; + + async function validateAndLoadData() { + try { + const response = await fetch("https://maplibre.org/maplibre-gl-js/docs/assets/indoor-3d-map.geojson"); + if (!response.ok) throw new Error('Failed to load GeoJSON'); + const data = await response.json(); + isLoading = false; + return data; + } catch (error) { + console.error('Error loading GeoJSON:', error); + dataError = true; + isLoading = false; + return null; + } + } +</script> -<GeoJSONSource data="https://maplibre.org/maplibre-gl-js/docs/assets/indoor-3d-map.geojson"> +<GeoJSONSource data={validateAndLoadData()}>Committable suggestion skipped: line range outside the PR's diff.
src/routes/docs/components/[slug]/+page.ts (1)
15-19: 🛠️ Refactor suggestion
Improve error handling flow.
The current error handling might throw after attempting to execute a non-existent loader. Consider this refactor for better flow:
- const loader = loaders[slug](); - if (!loader) { + if (!loaders[slug]) { error(404, `Component '${slug}' not found`); } + const loader = loaders[slug](); const doc = (await loader).default;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!loaders[slug]) { error(404, `Component '${slug}' not found`); } const loader = loaders[slug](); const doc = (await loader).default;
No description provided.