-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PMTiles and Three.js demo #43
Conversation
WalkthroughThe pull request includes several updates to the Changes
Possibly related PRs
🪧 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 (
|
edc1717
to
0450d09
Compare
Deploying svelte-maplibre-gl with
|
Latest commit: |
6368b38
|
Status: | ✅ Deploy successful! |
Preview URL: | https://87cd836c.svelte-maplibre-gl.pages.dev |
Branch Preview URL: | https://pmtiles-and-threejs-demo.svelte-maplibre-gl.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
src/content/examples/threejs-model/content.svelte.md (2)
1-5
: Consider adding more metadata fields for better documentation.The current metadata is good, but consider adding fields like:
tags
for better categorization (e.g., "3D", "three.js", "custom-layer")difficulty
level to help users gauge complexityrequirements
to list dependencies (three.js version)
13-15
: Enhance the documentation with additional content sections.Consider adding:
- A brief introduction explaining the purpose and benefits of using three.js with MapLibre
- Step-by-step explanation of how the implementation works
- Usage examples with different 3D models
- Performance considerations and best practices
Example structure:
# Introduction Brief overview... # Implementation <Demo /> # How it works Step-by-step explanation... # Source Code <CodeBlock content={demoRaw} /> # Additional Examples More use cases...src/content/examples/pmtiles/PMTiles.svelte (1)
23-23
: Consider using more responsive height styling.The current fixed height might not be optimal for all viewport sizes.
Consider using a more responsive approach:
-class="h-[55vh] min-h-[200px]" +class="h-[55vh] min-h-[200px] md:h-[65vh] lg:h-[75vh]"src/content/examples/Index.svelte (1)
Line range hint
4-47
: Consider type safety improvements and data separation.To enhance maintainability and type safety, consider:
- Adding TypeScript interfaces for the navigation structure
- Moving the TOC data to a separate configuration file
Example implementation:
// types/navigation.ts interface NavItem { path: string; title: string; } interface NavSection { title: string; items: Record<string, string>; } // config/navigation.ts export const navigationConfig: NavSection[] = [ { title: 'Basic', items: { '/examples/plain': 'Plain Map', // ... rest of the items } }, // ... rest of the sections ];Then import and use in the component:
<script lang="ts"> import { page } from '$app/stores'; +import type { NavSection } from '../types/navigation'; +import { navigationConfig } from '../config/navigation'; -const toc = [ - { - title: 'Basic', - // ... - } -]; +const toc: NavSection[] = navigationConfig; </script>src/content/examples/threejs-model/CustomLayer.svelte (3)
4-5
: Redundant Imports of 'Matrix4' and 'Vector3' from 'three'Since you've already imported all of 'three' as
THREE
in line 4, importing{ Matrix4, Vector3 }
separately in line 5 is redundant. You can access these classes usingTHREE.Matrix4
andTHREE.Vector3
. Consider removing the redundant import to simplify the code.Suggested change:
import * as THREE from 'three'; -import { Matrix4, Vector3 } from 'three';
39-43
: Unnecessary Non-Null Assertion Operator in 'onAdd' MethodIn the
onAdd
method, you're using the non-null assertion operator!
withmap!.getCanvas()
. Sincemap
is a parameter passed toonAdd
and is guaranteed to be defined, the!
operator is unnecessary. Removing it can improve code readability.Suggested change:
this.renderer = new THREE.WebGLRenderer({ - canvas: map!.getCanvas(), + canvas: map.getCanvas(), context: gl, antialias: true });
34-36
: Add Error Handling for GLTF Model LoadingThe
GLTFLoader
'sload
method currently lacks error handling. If the model fails to load, users won't receive any feedback. Consider adding error and progress callbacks to handle loading failures and to provide status updates.Suggested change:
loader.load( 'https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { this.scene.add(gltf.scene); }, + undefined, + (error) => { + console.error('An error occurred while loading the model:', error); + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json
(2 hunks)src/content/examples/Index.svelte
(1 hunks)src/content/examples/pmtiles/PMTiles.svelte
(1 hunks)src/content/examples/pmtiles/content.svelte.md
(1 hunks)src/content/examples/threejs-model/CustomLayer.svelte
(1 hunks)src/content/examples/threejs-model/content.svelte.md
(1 hunks)src/routes/components/[slug]/+page.svelte
(1 hunks)src/routes/examples/[slug]/+page.svelte
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/content/examples/pmtiles/content.svelte.md
🔇 Additional comments (12)
src/content/examples/threejs-model/content.svelte.md (2)
7-11
: Verify the imported components and dependencies.
The imports look correct, but let's ensure all dependencies are properly set up.
✅ Verification successful
All imported dependencies are properly set up and accessible
The verification confirms:
CustomLayer.svelte
exists in the correct directoryCodeBlock.svelte
is present in the parent directory- The project has
three.js
dependency installed (version 0.170.0)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of required components and three.js dependency
# Check if CustomLayer.svelte exists in the same directory
fd --glob "CustomLayer.svelte" src/content/examples/threejs-model/
# Check if CodeBlock.svelte exists in the parent directory
fd --glob "CodeBlock.svelte" src/content/
# Verify three.js dependency in package.json
rg '"three"' package.json
Length of output: 242
1-15
: Verify integration with documentation navigation.
Ensure this example is properly linked in the documentation navigation structure.
✅ Verification successful
Example is properly integrated in documentation navigation
The example is correctly linked in the documentation navigation structure within src/content/examples/Index.svelte
. It's listed under the examples section with the appropriate title "3D model with three.js" and the correct path "/examples/threejs-model".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the example is properly linked in the navigation
# Look for references to this example in index files
rg -l "threejs-model" src/content/
Length of output: 68
Script:
#!/bin/bash
# Check the content of Index.svelte to verify how the example is linked
rg -A 5 -B 5 "threejs-model" src/content/examples/Index.svelte
Length of output: 377
src/routes/components/[slug]/+page.svelte (2)
14-14
: LGTM! API simplification looks good.
The removal of the explicit shiki
prop simplifies the component usage while maintaining functionality.
14-14
: Verify syntax highlighting functionality
Please ensure that syntax highlighting still works correctly after removing the shiki
prop.
✅ Verification successful
Syntax highlighting is properly configured and functional
The syntax highlighting is properly configured in the project through multiple mechanisms:
- Global MDSvex configuration in
svelte.config.js
handles syntax highlighting using Shiki - A dedicated
CodeBlock.svelte
component provides client-side syntax highlighting with Shiki as fallback - The necessary dependencies (shiki v1.24.0) are properly installed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if shiki configuration has been moved elsewhere
# and verify that syntax highlighting is still properly configured
# Look for shiki configuration in the codebase
rg -l 'shiki' --type svelte --type typescript
# Check if Content component has internal shiki handling
ast-grep --pattern 'export const Content = {
$$$
shiki
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Let's try a broader search for shiki configuration and syntax highlighting
# Search for shiki in all files
rg -l "shiki"
# Search for syntax highlighting related terms
rg -l "highlight|syntax|prism|highlight.js"
# Look for mdsvex configuration which might handle syntax highlighting
rg -l "mdsvex\.config"
Length of output: 539
Script:
#!/bin/bash
# Let's check the content of files that might contain syntax highlighting configuration
cat svelte.config.js
# Check the CodeBlock component since it was found in the shiki search
cat src/content/CodeBlock.svelte
# Check package.json for syntax highlighting related dependencies
cat package.json
Length of output: 5508
src/routes/examples/[slug]/+page.svelte (2)
Line range hint 1-29
: LGTM! Well-structured component
The component follows best practices with proper semantic HTML, SEO handling, and responsive layout.
14-14
: Verify the removal of shiki prop
The removal of the shiki
prop from <data.Content />
might affect syntax highlighting functionality. Please ensure this change is intentional and syntax highlighting still works as expected.
Let's verify this change across similar components:
src/content/examples/pmtiles/PMTiles.svelte (2)
1-4
: LGTM! Good use of TypeScript.
The imports are well-structured and the use of TypeScript types enhances code reliability.
18-19
: LGTM! Clean protocol setup.
The PMTiles protocol registration is implemented correctly.
src/content/examples/Index.svelte (1)
Line range hint 1-70
: Implementation follows best practices!
The code demonstrates high quality through:
- Proper TypeScript integration
- Clean Svelte iteration patterns
- Well-organized Tailwind CSS classes
- Nice UX touch with view transitions
package.json (3)
85-85
: 🛠️ Refactor suggestion
Consider updating Three.js to a more recent version
The added Three.js dependency (^0.170.0) is several versions behind the current stable release. Consider updating to a more recent version for potential bug fixes, performance improvements, and security patches.
79-79
: Consider Svelte 5 beta status implications
The update to Svelte 5.2.10 maintains the project on the beta track. While this enables access to new features, be aware that breaking changes may occur before the stable release.
Line range hint 71-71
: Verify compatibility with pre-release MapLibre GL
Using a pre-release version (5.0.0-pre.8) of maplibre-gl could introduce instability. Please ensure thorough testing of the integration, especially with the new PMTiles and Three.js features.
✅ Verification successful
Let me try to gather more specific information about the MapLibre GL usage and version in this repository.
Let me check the specific PMTiles and Three.js example implementations to understand how they use MapLibre GL features.
Based on the gathered information, I can now provide a final response about the MapLibre GL pre-release version usage:
Pre-release MapLibre GL 5.0.0-pre.8 appears compatible with current implementation
The codebase shows well-structured integration with MapLibre GL, including:
- PMTiles protocol implementation working with vector tiles
- Three.js custom layer successfully utilizing MapLibre GL's 3D rendering capabilities
- Comprehensive set of core features (layers, sources, controls) properly implemented
Both PMTiles and Three.js examples are direct ports of official MapLibre examples, suggesting compatibility with the pre-release version. The implementation follows recommended patterns and shows no signs of instability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in MapLibre GL pre-releases
curl -s https://registry.npmjs.org/maplibre-gl | jq '.versions["5.0.0-pre.8"].dist-tags'
# Look for any migration guides or breaking changes in the repository
rg -i "breaking.*change|migration" $(fd -t f -e md)
Length of output: 1470
Script:
#!/bin/bash
# Check package.json for MapLibre GL version
cat package.json | jq '.dependencies["maplibre-gl"], .devDependencies["maplibre-gl"]'
# Look for MapLibre GL imports and usage
rg -l "maplibre-gl"
# Check for PMTiles and Three.js related files to understand the scope
rg -l "PMTiles|pmtiles"
rg -l "THREE|three.js|threejs"
Length of output: 3805
Script:
#!/bin/bash
# Check PMTiles implementation
cat src/content/examples/pmtiles/PMTiles.svelte
cat src/content/examples/pmtiles/content.svelte.md
# Check Three.js implementation
cat src/content/examples/threejs-model/CustomLayer.svelte
cat src/content/examples/threejs-model/content.svelte.md
# Check if there are any reported issues or warnings in the example code
rg -i "warning|caution|note" src/content/examples/pmtiles/content.svelte.md src/content/examples/threejs-model/content.svelte.md
Length of output: 4901
// Get the layers spec from the OSM style | ||
let layers: LayerSpecification[] = $state.raw([]); | ||
$effect(() => { | ||
fetch('https://tile.openstreetmap.jp/styles/openmaptiles/style.json') | ||
.then((response) => response.json()) | ||
.then((data) => { | ||
layers = data['layers'].filter( | ||
(layer: LayerSpecification) => !('source' in layer) || layer.source === 'openmaptiles' | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling and improve configuration management.
The layer fetching logic could be enhanced for better reliability and maintainability:
- Add error handling for the fetch operation
- Consider moving the URL to a configuration constant
- Add loading state management
- Improve type safety in the filter callback
Consider this implementation:
+const OSM_STYLE_URL = 'https://tile.openstreetmap.jp/styles/openmaptiles/style.json';
+let isLoading = $state(false);
+let error = $state<string | null>(null);
let layers: LayerSpecification[] = $state.raw([]);
$effect(() => {
+ isLoading = true;
+ error = null;
- fetch('https://tile.openstreetmap.jp/styles/openmaptiles/style.json')
+ fetch(OSM_STYLE_URL)
.then((response) => response.json())
.then((data) => {
layers = data['layers'].filter(
- (layer: LayerSpecification) => !('source' in layer) || layer.source === 'openmaptiles'
+ (layer) => !('source' in layer) || layer.source === 'openmaptiles'
);
- });
+ })
+ .catch((err) => {
+ error = `Failed to load map style: ${err.message}`;
+ console.error('Error loading map style:', err);
+ })
+ .finally(() => {
+ isLoading = false;
+ });
});
📝 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.
// Get the layers spec from the OSM style | |
let layers: LayerSpecification[] = $state.raw([]); | |
$effect(() => { | |
fetch('https://tile.openstreetmap.jp/styles/openmaptiles/style.json') | |
.then((response) => response.json()) | |
.then((data) => { | |
layers = data['layers'].filter( | |
(layer: LayerSpecification) => !('source' in layer) || layer.source === 'openmaptiles' | |
); | |
}); | |
}); | |
const OSM_STYLE_URL = 'https://tile.openstreetmap.jp/styles/openmaptiles/style.json'; | |
let isLoading = $state(false); | |
let error = $state<string | null>(null); | |
let layers: LayerSpecification[] = $state.raw([]); | |
$effect(() => { | |
isLoading = true; | |
error = null; | |
fetch(OSM_STYLE_URL) | |
.then((response) => response.json()) | |
.then((data) => { | |
layers = data['layers'].filter( | |
(layer) => !('source' in layer) || layer.source === 'openmaptiles' | |
); | |
}) | |
.catch((err) => { | |
error = `Failed to load map style: ${err.message}`; | |
console.error('Error loading map style:', err); | |
}) | |
.finally(() => { | |
isLoading = false; | |
}); | |
}); |
zoom={3} | ||
center={[138.0, 36.5]} | ||
style={{ | ||
version: 8, | ||
glyphs: 'https://tile.openstreetmap.jp/fonts/{fontstack}/{range}.pbf', | ||
sprite: 'https://tile.openstreetmap.jp/styles/openmaptiles/sprite', | ||
sources: { | ||
openmaptiles: { | ||
type: 'vector', | ||
url: 'pmtiles://https://tile.openstreetmap.jp/static/planet.pmtiles', | ||
attribution: '© OpenMapTiles © OpenStreetMap contributors' | ||
} | ||
}, | ||
layers | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
External resources need attention - fonts endpoint is not accessible
The verification results show:
- The fonts endpoint (tile.openstreetmap.jp/fonts/) returns a 504 Gateway Timeout error
- The sprite endpoint is working correctly (200 OK)
- The PMTiles endpoint is working correctly (200 OK)
Consider:
- Using a different font provider or ensuring the fonts endpoint is properly configured
- Adding error handling for font loading failures in the application
- Implementing a fallback font strategy
🔗 Analysis chain
Improve configuration management and verify external resources.
The MapLibre configuration could benefit from better organization and security considerations:
- Configuration values should be moved to constants
- External resource URLs should be verified for availability and security
Consider this refactoring:
+const MAP_CONFIG = {
+ initialZoom: 3,
+ initialCenter: [138.0, 36.5],
+ glyphsUrl: 'https://tile.openstreetmap.jp/fonts/{fontstack}/{range}.pbf',
+ spriteUrl: 'https://tile.openstreetmap.jp/styles/openmaptiles/sprite',
+ tilesUrl: 'https://tile.openstreetmap.jp/static/planet.pmtiles'
+};
<MapLibre
class="h-[55vh] min-h-[200px]"
- zoom={3}
- center={[138.0, 36.5]}
+ zoom={MAP_CONFIG.initialZoom}
+ center={MAP_CONFIG.initialCenter}
style={{
version: 8,
- glyphs: 'https://tile.openstreetmap.jp/fonts/{fontstack}/{range}.pbf',
- sprite: 'https://tile.openstreetmap.jp/styles/openmaptiles/sprite',
+ glyphs: MAP_CONFIG.glyphsUrl,
+ sprite: MAP_CONFIG.spriteUrl,
sources: {
openmaptiles: {
type: 'vector',
- url: 'pmtiles://https://tile.openstreetmap.jp/static/planet.pmtiles',
+ url: `pmtiles://${MAP_CONFIG.tilesUrl}`,
attribution: '© OpenMapTiles © OpenStreetMap contributors'
}
},
layers
}}
/>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability and security of external resources
# Check if the fonts URL is accessible
curl -I "https://tile.openstreetmap.jp/fonts/Noto%20Sans/0-255.pbf"
# Check if the sprite URL is accessible
curl -I "https://tile.openstreetmap.jp/styles/openmaptiles/sprite.json"
# Check if the PMTiles URL is accessible and returns proper headers
curl -I "https://tile.openstreetmap.jp/static/planet.pmtiles"
Length of output: 7061
'/examples/terrain': '3D Terrain', | ||
'/examples/clusters': 'Clusters', | ||
'/examples/limit-interaction': 'Limit Map Interactions', | ||
'/examples/dynamic-image': 'Dynamic Image', | ||
'/examples/animate-images': 'Animate a Series of Images', | ||
'/examples/video-on-a-map': 'Video on a Map', | ||
'/examples/canvas-source': 'Canvas Source', | ||
'/examples/image-loader': 'Load Images from URLs', | ||
'/examples/fullscreen': 'Fullscreen', | ||
'/examples/geolocate': 'Locate the User', | ||
'/examples/image-loader': 'Load Images from URLs', | ||
'/examples/canvas-source': 'Canvas Source' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Example components exist but need route configuration
The example components are present but with different naming conventions:
terrain/Terrain.svelte
image-loader/Images.svelte
threejs-model/CustomLayer.svelte
pmtiles/PMTiles.svelte
While the content and components exist, the missing +page.svelte
files indicate that the SvelteKit routes aren't properly configured. These files should be created to expose the examples through the navigation structure.
- Create
+page.svelte
files in each example directory that imports and renders the corresponding component:src/content/examples/terrain/+page.svelte
src/content/examples/image-loader/+page.svelte
src/content/examples/threejs-model/+page.svelte
src/content/examples/pmtiles/+page.svelte
🔗 Analysis chain
The navigation structure is well-organized but let's verify the new examples.
The reorganization of examples into categories is logical and improves navigation:
- Moving "Side by Side" to a dedicated "Techniques" category makes it easier to find
- Grouping 3D-related examples (terrain, three.js model) enhances discoverability
- Adding PMTiles to Extensions is appropriate for the protocol integration
Let's verify that all the new example components exist:
Also applies to: 24-28, 34-36, 42-42
Let me generate one more verification to check for the actual example files since we see the content.svelte.md files exist but not the +page.svelte files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all the new example components exist
# Check for new example components
echo "Checking for new example components..."
fd -e svelte "PMTiles.svelte|CustomLayer.svelte" src/content/examples
fd -e md "content.svelte.md" src/content/examples
# Verify the paths mentioned in the navigation
echo "Verifying example paths..."
for path in "/examples/terrain" "/examples/image-loader" "/examples/threejs-model" "/examples/pmtiles"; do
# Remove leading slash and check if the corresponding Svelte file exists
component_path="src/content${path}/+page.svelte"
if [ -f "$component_path" ]; then
echo "✓ Found: $component_path"
else
echo "✗ Missing: $component_path"
fi
done
Length of output: 2926
Script:
#!/bin/bash
# Let's check for the actual example files in these directories
for dir in "terrain" "image-loader" "threejs-model" "pmtiles"; do
echo "Checking contents of src/content/examples/$dir/"
ls -la "src/content/examples/$dir/"
done
# Also check if there are any .svelte files in these directories
echo -e "\nChecking for .svelte files:"
fd -e svelte . src/content/examples/terrain src/content/examples/image-loader src/content/examples/threejs-model src/content/examples/pmtiles
Length of output: 2280
import { Matrix4, Vector3 } from 'three'; | ||
import { GLTFLoader } from 'three/addons/loaders/GLTFLoader.js'; | ||
|
||
let map: maplibregl.Map | undefined = $state.raw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary Initialization of 'map' Variable with '$state.raw()'
The initialization of map
using $state.raw()
is unclear and may lead to unexpected behavior. Since you're binding map
in the MapLibre
component using bind:map
, you can declare map
without initializing it. Consider updating the declaration as follows.
Suggested change:
-let map: maplibregl.Map | undefined = $state.raw();
+let map: maplibregl.Map | undefined;
📝 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.
let map: maplibregl.Map | undefined = $state.raw(); | |
let map: maplibregl.Map | undefined; |
this.camera.projectionMatrix = world.multiply(local); | ||
this.renderer!.resetState(); | ||
this.renderer!.render(this.scene, this.camera); | ||
map!.triggerRepaint(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'map' is Undefined in 'render' Method
In the render
method, you're calling map!.triggerRepaint();
, but map
is not defined within this method or the class scope. This can lead to runtime errors. To fix this, store the map
reference in the class during the onAdd
method so it can be accessed later in render
.
Suggested changes:
Add map
property to the class:
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> {
camera = new THREE.Camera();
scene = new THREE.Scene();
renderer: THREE.WebGLRenderer | null = null;
+ map: maplibregl.Map | null = null;
Store map
in onAdd
:
onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) {
+ this.map = map;
Use this.map
in render
:
- map!.triggerRepaint();
+ this.map!.triggerRepaint();
Committable suggestion skipped: line range outside the PR's diff.
0450d09
to
6d0816a
Compare
6d0816a
to
6368b38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
🧹 Outside diff range and nitpick comments (4)
src/content/examples/pmtiles/PMTiles.svelte (1)
26-38
: Add type safety to the style configuration object.The style configuration object would benefit from proper TypeScript type definitions to ensure type safety and better IDE support.
+import type { StyleSpecification } from 'maplibre-gl'; + +const mapStyle: StyleSpecification = { + version: 8, + glyphs: 'https://tile.openstreetmap.jp/fonts/{fontstack}/{range}.pbf', + sprite: 'https://tile.openstreetmap.jp/styles/openmaptiles/sprite', + sources: { + openmaptiles: { + type: 'vector', + url: 'pmtiles://https://tile.openstreetmap.jp/static/planet.pmtiles', + attribution: '© OpenMapTiles © OpenStreetMap contributors' + } + }, + layers +}; <MapLibre class="h-[55vh] min-h-[200px]" zoom={3} center={[138.0, 36.5]} - style={{ - version: 8, - glyphs: 'https://tile.openstreetmap.jp/fonts/{fontstack}/{range}.pbf', - sprite: 'https://tile.openstreetmap.jp/styles/openmaptiles/sprite', - sources: { - openmaptiles: { - type: 'vector', - url: 'pmtiles://https://tile.openstreetmap.jp/static/planet.pmtiles', - attribution: '© OpenMapTiles © OpenStreetMap contributors' - } - }, - layers - }} + style={mapStyle} />src/content/CodeBlock.svelte (1)
Line range hint
1-57
: Consider enhancing the component's reusability.The component has a solid implementation but could be more flexible:
- Consider accepting the language and theme as props instead of hardcoding 'svelte' and 'github-dark-default'
- The highlighter initialization could be moved to a shared store to avoid re-initialization across multiple instances
Example store implementation:
// stores/syntax-highlighter.ts import { browser } from '$app/environment'; import { createHighlighter, type Highlighter } from 'shiki'; import { writable } from 'svelte/store'; export const createHighlighterStore = () => { const { subscribe, set } = writable<Promise<Highlighter> | undefined>(undefined); if (browser) { set(createHighlighter({ themes: [/* themes */], langs: [/* langs */] })); } return { subscribe }; }; export const highlighter = createHighlighterStore();src/content/examples/threejs-model/CustomLayer.svelte (2)
10-13
: Consider extracting configuration valuesThe model position and rotation values are hardcoded. Consider extracting these into a configuration object or component props for better reusability and maintainability.
+interface ModelConfig { + origin: [number, number]; + altitude: number; + rotate: [number, number, number]; +} + +const DEFAULT_CONFIG: ModelConfig = { + origin: [148.9819, -35.39847], + altitude: 0, + rotate: [Math.PI / 2, 0, 0] +}; + -const modelOrigin: [number, number] = [148.9819, -35.39847]; -const modelAltitude = 0; -const modelRotate = [Math.PI / 2, 0, 0]; +export let config: ModelConfig = DEFAULT_CONFIG;
48-62
: Consider performance optimization for render methodThe render method is called frequently during map interactions. Consider these optimizations:
- Cache Matrix4 instances instead of creating new ones on each render
- Implement render throttling
- Only trigger repaint when necessary
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { + private worldMatrix = new Matrix4(); + private localMatrix = new Matrix4(); + private lastRepaint = 0; + private REPAINT_THROTTLE_MS = 16; // ~60fps + render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) { const scale = modelAsMercatorCoordinate.meterInMercatorCoordinateUnits(); - const world = new Matrix4().fromArray(options.defaultProjectionData.mainMatrix); - const local = new Matrix4() + this.worldMatrix.fromArray(options.defaultProjectionData.mainMatrix); + this.localMatrix .makeTranslation(modelAsMercatorCoordinate.x, modelAsMercatorCoordinate.y, modelAsMercatorCoordinate.z) .scale(new Vector3(scale, -scale, scale)) .multiply(new Matrix4().makeRotationX(modelRotate[0])) .multiply(new Matrix4().makeRotationY(modelRotate[1])) .multiply(new Matrix4().makeRotationZ(modelRotate[2])); - this.camera.projectionMatrix = world.multiply(local); + this.camera.projectionMatrix = this.worldMatrix.multiply(this.localMatrix); this.renderer!.resetState(); this.renderer!.render(this.scene, this.camera); - map!.triggerRepaint(); + + const now = Date.now(); + if (now - this.lastRepaint >= this.REPAINT_THROTTLE_MS) { + map!.triggerRepaint(); + this.lastRepaint = now; + } } }
🛑 Comments failed to post (3)
src/content/examples/threejs-model/CustomLayer.svelte (3)
68-78: 🛠️ Refactor suggestion
Make map configuration customizable and add loading states
The map configuration is hardcoded and there's no visual feedback for model loading states.
+<script lang="ts"> +export let mapStyle = "https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json"; +export let initialZoom = 18; +export let initialPitch = 60; +export let initialCenter: [number, number] = [148.9819, -35.3981]; +</script> <MapLibre bind:map class="h-[55vh] min-h-[300px]" - style="https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json" - zoom={18} - pitch={60} - center={[148.9819, -35.3981]} + style={mapStyle} + zoom={initialZoom} + pitch={initialPitch} + center={initialCenter} antialias > <CustomLayer implementation={customLayerImpl} /> + {#if isModelLoading} + <div class="absolute inset-0 flex items-center justify-center bg-black/50"> + <p class="text-white">Loading 3D model...</p> + </div> + {/if} + {#if modelLoadingError} + <div class="absolute inset-0 flex items-center justify-center bg-black/50"> + <p class="text-red-500">Failed to load 3D model</p> + </div> + {/if} </MapLibre>Committable suggestion skipped: line range outside the PR's diff.
33-36:
⚠️ Potential issueAdd error handling for model loading
The GLTF model loading lacks error handling and loading state management. This could lead to silent failures and poor user experience.
const loader = new GLTFLoader(); +let modelLoadingError = $state(false); +let isModelLoading = $state(true); + loader.load( 'https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', - (gltf) => { + (gltf) => { + isModelLoading = false; this.scene.add(gltf.scene); + }, + undefined, + (error) => { + console.error('Error loading model:', error); + isModelLoading = false; + modelLoadingError = true; } );Committable suggestion skipped: line range outside the PR's diff.
15-63:
⚠️ Potential issueAdd cleanup method for proper resource management
The class should implement cleanup to prevent memory leaks and ensure proper disposal of Three.js resources.
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { // ... existing code ... + + onRemove() { + if (this.renderer) { + this.renderer.dispose(); + this.renderer = null; + } + + // Dispose geometries and materials + this.scene.traverse((object) => { + if (object instanceof THREE.Mesh) { + object.geometry.dispose(); + if (object.material instanceof THREE.Material) { + object.material.dispose(); + } + } + }); + } }📝 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.class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { camera = new THREE.Camera(); scene = new THREE.Scene(); renderer: THREE.WebGLRenderer | null = null; renderingMode = '3d' as const; onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) { // create two three.js lights const directionalLight1 = new THREE.DirectionalLight(0xffffff); directionalLight1.position.set(0, -70, 100).normalize(); this.scene.add(directionalLight1); const directionalLight2 = new THREE.DirectionalLight(0xffffff); directionalLight2.position.set(0, 70, 100).normalize(); this.scene.add(directionalLight2); // load a glTF model const loader = new GLTFLoader(); loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { this.scene.add(gltf.scene); }); // use the MapLibre GL JS map canvas for three.js this.renderer = new THREE.WebGLRenderer({ canvas: map!.getCanvas(), context: gl, antialias: true }); this.renderer.autoClear = false; } render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) { const scale = modelAsMercatorCoordinate.meterInMercatorCoordinateUnits(); const world = new Matrix4().fromArray(options.defaultProjectionData.mainMatrix); const local = new Matrix4() .makeTranslation(modelAsMercatorCoordinate.x, modelAsMercatorCoordinate.y, modelAsMercatorCoordinate.z) .scale(new Vector3(scale, -scale, scale)) .multiply(new Matrix4().makeRotationX(modelRotate[0])) .multiply(new Matrix4().makeRotationY(modelRotate[1])) .multiply(new Matrix4().makeRotationZ(modelRotate[2])); this.camera.projectionMatrix = world.multiply(local); this.renderer!.resetState(); this.renderer!.render(this.scene, this.camera); map!.triggerRepaint(); } onRemove() { if (this.renderer) { this.renderer.dispose(); this.renderer = null; } // Dispose geometries and materials this.scene.traverse((object) => { if (object instanceof THREE.Mesh) { object.geometry.dispose(); if (object.material instanceof THREE.Material) { object.material.dispose(); } } }); } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
.github/workflows/release.yml (1)
9-9
: LGTM! Good security practice adding environment specification.Adding the
environment: npm
specification is a security enhancement that allows better management of deployment secrets and enables optional protection rules for the npm publishing process.Consider adding environment protection rules in GitHub (like required reviewers) for additional security if not already configured.
src/content/examples/threejs-model/CustomLayer.svelte (2)
10-13
: Document model parameters and add type safetyThe model position and rotation parameters lack documentation and type safety.
-const modelOrigin: [number, number] = [148.9819, -35.39847]; -const modelAltitude = 0; -const modelRotate = [Math.PI / 2, 0, 0]; +// Coordinates for the Australian Parliament House in Canberra +const modelOrigin: [number, number] = [148.9819, -35.39847]; +const modelAltitude: number = 0; +const modelRotate: [number, number, number] = [Math.PI / 2, 0, 0];
71-71
: Consider making the map style URL configurableThe map style URL is hardcoded. Consider making it a component prop for better reusability.
+<script lang="ts"> + export let mapStyle = "https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json"; +</script> - style="https://basemaps.cartocdn.com/gl/voyager-gl-style/style.json" + style={mapStyle}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.github/workflows/release.yml
(1 hunks)package.json
(3 hunks)src/content/CodeBlock.svelte
(1 hunks)src/content/examples/Index.svelte
(1 hunks)src/content/examples/pmtiles/PMTiles.svelte
(1 hunks)src/content/examples/pmtiles/content.svelte.md
(1 hunks)src/content/examples/threejs-model/CustomLayer.svelte
(1 hunks)src/content/examples/threejs-model/content.svelte.md
(1 hunks)src/routes/components/[slug]/+page.svelte
(1 hunks)src/routes/examples/[slug]/+page.svelte
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- src/content/CodeBlock.svelte
- src/content/examples/Index.svelte
- src/content/examples/pmtiles/PMTiles.svelte
- src/content/examples/pmtiles/content.svelte.md
- src/content/examples/threejs-model/content.svelte.md
- src/routes/components/[slug]/+page.svelte
- src/routes/examples/[slug]/+page.svelte
🔇 Additional comments (2)
.github/workflows/release.yml (1)
Line range hint 9-28
: Verify npm environment configuration
The workflow depends on proper environment setup in GitHub repository settings.
src/content/examples/threejs-model/CustomLayer.svelte (1)
8-8
: Remove unnecessary initialization with $state.raw()
The initialization of map
using $state.raw()
is unclear and may lead to unexpected behavior since the variable is bound in the MapLibre component.
-let map: maplibregl.Map | undefined = $state.raw();
+let map: maplibregl.Map | undefined;
center={[148.9819, -35.3981]} | ||
antialias | ||
> | ||
<CustomLayer implementation={customLayerImpl} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error boundary for the custom layer
The custom layer should be wrapped in an error boundary to handle potential rendering failures gracefully.
+<script lang="ts">
+ let hasError = false;
+
+ function handleError(error: Error) {
+ console.error('Custom layer error:', error);
+ hasError = true;
+ }
+</script>
- <CustomLayer implementation={customLayerImpl} />
+ {#if !hasError}
+ <CustomLayer
+ implementation={customLayerImpl}
+ on:error={handleError}
+ />
+ {:else}
+ <div class="error-message">
+ Failed to load 3D model layer
+ </div>
+ {/if}
Committable suggestion skipped: line range outside the PR's diff.
const loader = new GLTFLoader(); | ||
loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { | ||
this.scene.add(gltf.scene); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling for model loading
The GLTF model loading lacks error handling and loading state management.
const loader = new GLTFLoader();
-loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => {
- this.scene.add(gltf.scene);
-});
+loader.load(
+ 'https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf',
+ (gltf) => {
+ this.scene.add(gltf.scene);
+ },
+ (progress) => {
+ console.log((progress.loaded / progress.total * 100) + '% loaded');
+ },
+ (error) => {
+ console.error('Error loading model:', error);
+ }
+);
📝 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.
const loader = new GLTFLoader(); | |
loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { | |
this.scene.add(gltf.scene); | |
}); | |
const loader = new GLTFLoader(); | |
loader.load( | |
'https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', | |
(gltf) => { | |
this.scene.add(gltf.scene); | |
}, | |
(progress) => { | |
console.log((progress.loaded / progress.total * 100) + '% loaded'); | |
}, | |
(error) => { | |
console.error('Error loading model:', error); | |
} | |
); |
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { | ||
camera = new THREE.Camera(); | ||
scene = new THREE.Scene(); | ||
renderer: THREE.WebGLRenderer | null = null; | ||
|
||
renderingMode = '3d' as const; | ||
|
||
onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) { | ||
// create two three.js lights | ||
const directionalLight1 = new THREE.DirectionalLight(0xffffff); | ||
directionalLight1.position.set(0, -70, 100).normalize(); | ||
this.scene.add(directionalLight1); | ||
|
||
const directionalLight2 = new THREE.DirectionalLight(0xffffff); | ||
directionalLight2.position.set(0, 70, 100).normalize(); | ||
this.scene.add(directionalLight2); | ||
|
||
// load a glTF model | ||
const loader = new GLTFLoader(); | ||
loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { | ||
this.scene.add(gltf.scene); | ||
}); | ||
|
||
// use the MapLibre GL JS map canvas for three.js | ||
this.renderer = new THREE.WebGLRenderer({ | ||
canvas: map!.getCanvas(), | ||
context: gl, | ||
antialias: true | ||
}); | ||
|
||
this.renderer.autoClear = false; | ||
} | ||
|
||
render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) { | ||
const scale = modelAsMercatorCoordinate.meterInMercatorCoordinateUnits(); | ||
const world = new Matrix4().fromArray(options.defaultProjectionData.mainMatrix); | ||
const local = new Matrix4() | ||
.makeTranslation(modelAsMercatorCoordinate.x, modelAsMercatorCoordinate.y, modelAsMercatorCoordinate.z) | ||
.scale(new Vector3(scale, -scale, scale)) | ||
.multiply(new Matrix4().makeRotationX(modelRotate[0])) | ||
.multiply(new Matrix4().makeRotationY(modelRotate[1])) | ||
.multiply(new Matrix4().makeRotationZ(modelRotate[2])); | ||
|
||
this.camera.projectionMatrix = world.multiply(local); | ||
this.renderer!.resetState(); | ||
this.renderer!.render(this.scene, this.camera); | ||
map!.triggerRepaint(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add resource cleanup method
The class should implement proper cleanup of Three.js resources to prevent memory leaks.
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> {
camera = new THREE.Camera();
scene = new THREE.Scene();
renderer: THREE.WebGLRenderer | null = null;
+ map: maplibregl.Map | null = null;
renderingMode = '3d' as const;
onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) {
+ this.map = map;
// ... existing code ...
}
+ onRemove() {
+ // Dispose of Three.js resources
+ this.scene.traverse((object) => {
+ if (object instanceof THREE.Mesh) {
+ object.geometry.dispose();
+ if (object.material instanceof THREE.Material) {
+ object.material.dispose();
+ }
+ }
+ });
+ this.renderer?.dispose();
+ this.renderer = null;
+ this.map = null;
+ }
render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) {
// ... existing code ...
- map!.triggerRepaint();
+ this.map!.triggerRepaint();
}
}
📝 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.
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { | |
camera = new THREE.Camera(); | |
scene = new THREE.Scene(); | |
renderer: THREE.WebGLRenderer | null = null; | |
renderingMode = '3d' as const; | |
onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) { | |
// create two three.js lights | |
const directionalLight1 = new THREE.DirectionalLight(0xffffff); | |
directionalLight1.position.set(0, -70, 100).normalize(); | |
this.scene.add(directionalLight1); | |
const directionalLight2 = new THREE.DirectionalLight(0xffffff); | |
directionalLight2.position.set(0, 70, 100).normalize(); | |
this.scene.add(directionalLight2); | |
// load a glTF model | |
const loader = new GLTFLoader(); | |
loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { | |
this.scene.add(gltf.scene); | |
}); | |
// use the MapLibre GL JS map canvas for three.js | |
this.renderer = new THREE.WebGLRenderer({ | |
canvas: map!.getCanvas(), | |
context: gl, | |
antialias: true | |
}); | |
this.renderer.autoClear = false; | |
} | |
render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) { | |
const scale = modelAsMercatorCoordinate.meterInMercatorCoordinateUnits(); | |
const world = new Matrix4().fromArray(options.defaultProjectionData.mainMatrix); | |
const local = new Matrix4() | |
.makeTranslation(modelAsMercatorCoordinate.x, modelAsMercatorCoordinate.y, modelAsMercatorCoordinate.z) | |
.scale(new Vector3(scale, -scale, scale)) | |
.multiply(new Matrix4().makeRotationX(modelRotate[0])) | |
.multiply(new Matrix4().makeRotationY(modelRotate[1])) | |
.multiply(new Matrix4().makeRotationZ(modelRotate[2])); | |
this.camera.projectionMatrix = world.multiply(local); | |
this.renderer!.resetState(); | |
this.renderer!.render(this.scene, this.camera); | |
map!.triggerRepaint(); | |
} | |
} | |
class CustomLayerImpl implements Omit<maplibregl.CustomLayerInterface, 'id' | 'type'> { | |
camera = new THREE.Camera(); | |
scene = new THREE.Scene(); | |
renderer: THREE.WebGLRenderer | null = null; | |
map: maplibregl.Map | null = null; | |
renderingMode = '3d' as const; | |
onAdd(map: maplibregl.Map, gl: WebGL2RenderingContext) { | |
this.map = map; | |
// create two three.js lights | |
const directionalLight1 = new THREE.DirectionalLight(0xffffff); | |
directionalLight1.position.set(0, -70, 100).normalize(); | |
this.scene.add(directionalLight1); | |
const directionalLight2 = new THREE.DirectionalLight(0xffffff); | |
directionalLight2.position.set(0, 70, 100).normalize(); | |
this.scene.add(directionalLight2); | |
// load a glTF model | |
const loader = new GLTFLoader(); | |
loader.load('https://maplibre.org/maplibre-gl-js/docs/assets/34M_17/34M_17.gltf', (gltf) => { | |
this.scene.add(gltf.scene); | |
}); | |
// use the MapLibre GL JS map canvas for three.js | |
this.renderer = new THREE.WebGLRenderer({ | |
canvas: map!.getCanvas(), | |
context: gl, | |
antialias: true | |
}); | |
this.renderer.autoClear = false; | |
} | |
onRemove() { | |
// Dispose of Three.js resources | |
this.scene.traverse((object) => { | |
if (object instanceof THREE.Mesh) { | |
object.geometry.dispose(); | |
if (object.material instanceof THREE.Material) { | |
object.material.dispose(); | |
} | |
} | |
}); | |
this.renderer?.dispose(); | |
this.renderer = null; | |
this.map = null; | |
} | |
render(_gl: WebGL2RenderingContext | WebGLRenderingContext, options: maplibregl.CustomRenderMethodInput) { | |
const scale = modelAsMercatorCoordinate.meterInMercatorCoordinateUnits(); | |
const world = new Matrix4().fromArray(options.defaultProjectionData.mainMatrix); | |
const local = new Matrix4() | |
.makeTranslation(modelAsMercatorCoordinate.x, modelAsMercatorCoordinate.y, modelAsMercatorCoordinate.z) | |
.scale(new Vector3(scale, -scale, scale)) | |
.multiply(new Matrix4().makeRotationX(modelRotate[0])) | |
.multiply(new Matrix4().makeRotationY(modelRotate[1])) | |
.multiply(new Matrix4().makeRotationZ(modelRotate[2])); | |
this.camera.projectionMatrix = world.multiply(local); | |
this.renderer!.resetState(); | |
this.renderer!.render(this.scene, this.camera); | |
this.map!.triggerRepaint(); | |
} | |
} |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
Content
component across multiple pages by removing unnecessary props.