Conversation
kimgh06
commented
Sep 10, 2025
- Divide tilemap to Opened tile map, and dynamic tilemap
- Setting render range smaller
- Refactor values to constant values
… and maintainability
…onnect and message sending logic
…tion for improved performance
… improving readability
…culations for better accuracy
…or checking logic for better accuracy
…roved performance and readability
… logic for improved performance and clarity
…ement for improved performance and clarity
…in Play component for improved clarity and maintainability
…r improved clarity and maintainability
…Play component for improved clarity and maintainability
…pe for improved type safety and consistency
…mponent for improved type safety and maintainability
…nent for improved performance and clarity
…nent to enhance performance and stability
…ence in Play component for cleaner code
…sRender components with high-resolution canvas setup and improved texture handling
…r pattern calculation and remove unused setCachingTiles prop in CanvasRenderComponent
…CanvasRender components for improved clarity and performance
… rendering quality
…nced rendering precision
…texture selection logic in Tilemap component for improved safety and clarity
… by implementing a worker for large payloads and enhancing hex to byte conversion for improved performance
… component for improved rendering accuracy and quality
…cation in Play component for improved performance and clarity
…mponents, enhancing user interaction and rendering capabilities
… a fixed pixelated style and optimizing color gradient generation for improved visual quality
… timing check to prevent excessive requests and utilizing useRef for state management
…t and implement switch-case for keyboard event handling to enhance code clarity
…ponent by utilizing useRef for persistent texture storage, enhancing performance and visual fidelity
…ne visible bounds computation, texture selection, and tile edge snapping for improved code clarity and performance
There was a problem hiding this comment.
Pull Request Overview
This PR significantly refactors tilemap rendering for improved performance by separating opened and dynamic tilemaps, implementing more efficient rendering strategies, and consolidating constants across the codebase.
Key changes include:
- Complete restructuring of tilemap rendering with optimized texture generation, pooled sprite management, and web worker support for tile parsing
- Consolidation of type definitions, constants, and utilities into centralized modules
- Performance optimizations across components including memory-efficient DOM updates and high-resolution canvas rendering
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/makePath2d.ts | New utility for Path2D creation and array processing |
| src/utils/canvas.ts | Canvas rendering helper for filling paths with colors |
| src/types/position.ts | Type definitions for coordinates and directions |
| src/types/message.ts | WebSocket message types and event constants |
| src/types/canvas.ts | Canvas-related types including vector images and tile content |
| src/constants/cursor.ts | Cursor color mappings and directional arrays |
| src/constants/click.ts | Click type constants |
| src/components/tilemap/index.tsx | Major refactor with texture caching, sprite pooling, and worker support |
| src/components/canvas/index.tsx | High-resolution canvas setup and optimized rendering |
| src/app/play/page.tsx | Web worker implementation for tile parsing and performance improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| const tempCanvas = document.createElement('canvas'); | ||
| const tileMinializedSize = Math.sqrt(tileSize / 5); | ||
| const tileMinializedSize = 4; // fixed small size for pixelated look |
There was a problem hiding this comment.
The variable name 'tileMinializedSize' contains a typo. It should be 'tileMiniaturizedSize' or 'tileMinimizedSize'.
| texture.baseTexture.wrapMode = WRAP_MODES.CLAMP; | ||
| texture.baseTexture.setSize(tileMinializedSize, tileMinializedSize); | ||
| texture.baseTexture.resolution = 0; | ||
| texture.baseTexture.resolution = 0.001; |
There was a problem hiding this comment.
The magic number 0.001 should be defined as a named constant to explain its purpose and make it easier to maintain.
| outer.eventMode = 'none' as unknown as never; | ||
| outer.cullable = true; | ||
| const inner = new PixiSprite(); | ||
| inner.roundPixels = true; | ||
| inner.eventMode = 'none' as unknown as never; |
There was a problem hiding this comment.
The type assertion 'as unknown as never' suggests an API compatibility issue. This should be properly typed or the underlying type issue should be addressed rather than using unsafe type assertions.
| outer.eventMode = 'none' as unknown as never; | |
| outer.cullable = true; | |
| const inner = new PixiSprite(); | |
| inner.roundPixels = true; | |
| inner.eventMode = 'none' as unknown as never; | |
| outer.eventMode = 'none' as PixiSprite['eventMode']; | |
| outer.cullable = true; | |
| const inner = new PixiSprite(); | |
| inner.roundPixels = true; | |
| inner.eventMode = 'none' as PixiSprite['eventMode']; |
| ctx.imageSmoothingEnabled = true; | ||
| ctx.imageSmoothingQuality = 'high'; | ||
| // 🚀 HIGH QUALITY: Text rendering optimization (if supported) | ||
| if ('textRenderingOptimization' in ctx) ctx.textRenderingOptimization = 'optimizeQuality'; |
There was a problem hiding this comment.
The property 'textRenderingOptimization' doesn't exist on CanvasRenderingContext2D. This should be removed or properly feature-detected with correct property names.
| if ('textRenderingOptimization' in ctx) ctx.textRenderingOptimization = 'optimizeQuality'; |
| const workerRef: { current: Worker | null } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerRef as { current: Worker | null }) || { | ||
| current: null, | ||
| }; | ||
| const workerInitRef: { current: boolean } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerInit as { current: boolean }) || { | ||
| current: false, | ||
| }; | ||
| const generationRef: { current: number } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerGen as { current: number }) || { | ||
| current: 0, | ||
| }; | ||
| (globalThis as unknown as Record<string, unknown>)._tileWorkerRef = workerRef; | ||
| (globalThis as unknown as Record<string, unknown>)._tileWorkerInit = workerInitRef; | ||
| (globalThis as unknown as Record<string, unknown>)._tileWorkerGen = generationRef; |
There was a problem hiding this comment.
Using globalThis to store worker references is an anti-pattern that can cause memory leaks and state pollution. Consider using a proper singleton pattern or module-level variable instead.
| const workerRef: { current: Worker | null } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerRef as { current: Worker | null }) || { | |
| current: null, | |
| }; | |
| const workerInitRef: { current: boolean } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerInit as { current: boolean }) || { | |
| current: false, | |
| }; | |
| const generationRef: { current: number } = ((globalThis as unknown as Record<string, unknown>)._tileWorkerGen as { current: number }) || { | |
| current: 0, | |
| }; | |
| (globalThis as unknown as Record<string, unknown>)._tileWorkerRef = workerRef; | |
| (globalThis as unknown as Record<string, unknown>)._tileWorkerInit = workerInitRef; | |
| (globalThis as unknown as Record<string, unknown>)._tileWorkerGen = generationRef; | |
| // Inline Worker: parse tiles off main thread for large payloads | |
| const workerRef = useRef<Worker | null>(null); | |
| const workerInitRef = useRef<boolean>(false); | |
| const generationRef = useRef<number>(0); |
| let c0 = hex.charCodeAt(p), c1 = hex.charCodeAt(p + 1); | ||
| let n0 = c0 <= 57 ? c0 - 48 : c0 <= 70 ? c0 - 55 : c0 - 87; | ||
| let n1 = c1 <= 57 ? c1 - 48 : c1 <= 70 ? c1 - 55 : c1 - 87; |
There was a problem hiding this comment.
The magic numbers 57, 70, 48, 55, 87 should be defined as named constants (e.g., CHAR_CODE_9, CHAR_CODE_F, etc.) to improve code readability and maintainability.
| width={windowWidth * devicePixelRatio} | ||
| height={windowHeight * devicePixelRatio} |
There was a problem hiding this comment.
The devicePixelRatio multiplication is repeated multiple times. Consider extracting this calculation into variables like 'canvasWidth' and 'canvasHeight' to reduce duplication and improve maintainability.