refactor: unify world configuration in shared package#71
Conversation
Previously, zone exit notification only fired when entering a new zone. Now correctly handles three cases: - Zone → Gap: Shows exit notification immediately - Zone → Zone: Shows both exit and enter notifications - Gap → Zone: Shows enter notification only
Add centralized world configuration in shared package: - ZONE_BOUNDS: zone boundary definitions - DEFAULT_SPAWN_POINT: spawn location (1024, 1024) - MAP_CONFIG: map dimensions (64x64 tiles, 2048x2048 pixels) - ZONE_COLORS: zone color palette for UI - DEBUG_COLORS: debug overlay colors - ZONE_IDS: list of all zone identifiers - getZoneBoundsMap(): helper to get Map<ZoneId, ZoneBounds>
- ZoneSystem: import ZoneBounds and getZoneBoundsMap from shared - GameRoom: import DEFAULT_SPAWN_POINT from shared - Remove duplicated zone bounds definitions from server
- GameScene: import ZONE_BOUNDS, ZONE_COLORS, DEBUG_COLORS, DEFAULT_SPAWN_POINT from shared for debug overlay - Minimap: import ZONE_BOUNDS, ZONE_COLORS, MAP_CONFIG, ZONE_IDS from shared for minimap rendering - Remove duplicated zone definitions from client - Enhanced F3 debug mode shows zones, collision, spawn point
- Add scripts/sync-maps.sh to sync world pack map to server/client - Integrate sync-maps into build process (runs before pnpm -r build) - Sync server village.json with world pack grid_town_outdoor.json to fix collision mismatch (was 64x52, now 64x64 with correct tiles) This ensures server and client always use the same map data.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This PR is quite large. Consider breaking it into smaller PRs for easier review. |
Summary of ChangesHello @ComBba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the game's world configuration by centralizing critical constants such as map dimensions, zone boundaries, and spawn points into a shared package. This unification addresses a significant problem where server and client map data diverged, leading to gameplay inconsistencies like players walking through walls. The changes also introduce a robust map synchronization mechanism to prevent future discrepancies and enhance the developer experience with a more informative debug overlay, improving overall game stability and maintainability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 🎯 개요이 PR은 공유 월드 구성(맵 차원, 영역 경계, 스폰 지점)을 새로운 변경사항
예상 코드 리뷰 노력🎯 4 (복합) | ⏱️ ~45분 연관될 수 있는 PR
제안 레이블
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client/src/game/scenes/GameScene.ts (1)
967-1001:⚠️ Potential issue | 🟡 Minor
currentZone(null)과previousZone(undefined) 간 null/undefined 혼용.Line 968에서
currentZone은?? null로 인해ZoneId | null타입이고,previousZone(line 61)은ZoneId | undefined타입입니다. Line 1001에서this.previousZone = currentZone는null을undefined필드에 할당합니다.또한 line 970에서
currentZone === this.previousZone비교 시, 초기 상태에서null !== undefined이므로 첫 프레임에서 불필요하게 change 로직에 진입합니다 (실질적 부작용은 없지만 비일관적).null/undefined 중 하나로 통일하는 것을 권장합니다:
♻️ 제안된 변경
private checkZoneChange(entity: Entity): void { - const currentZone = (entity as unknown as { currentZone?: ZoneId }).currentZone ?? null; - - if (currentZone === this.previousZone) return; - - if (this.previousZone && !currentZone) { + const currentZone = (entity as unknown as { currentZone?: ZoneId }).currentZone; + + if (currentZone === this.previousZone) return; + + if (this.previousZone && !currentZone) {이렇게 하면 둘 다
ZoneId | undefined로 일관되게 됩니다.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 16-17: The package.json "sync-maps" script calls a Bash-only
script ("bash scripts/sync-maps.sh"), which breaks on Windows; rewrite the shell
script as a cross-platform Node.js script (e.g., scripts/sync-maps.js) that
performs the same sync steps and errors with non-zero exit codes, then update
the "sync-maps" entry to run that Node script (and keep the "build" script
chaining to "pnpm run sync-maps && pnpm -r build"); reference the existing
"sync-maps" script name and the "build" script so you replace the Bash
dependency with a Node implementation that works on Windows, macOS, and Linux.
In `@packages/client/src/game/scenes/GameScene.ts`:
- Around line 625-638: Add a proper private class field instead of using a
type-unsafe cast: declare private _debugBlockedCount = 0; on the GameScene
class, then replace the hacky assignment `(this as unknown as {
_debugBlockedCount: number })._debugBlockedCount = blockedCount;` with
`this._debugBlockedCount = blockedCount;` so references to _debugBlockedCount
use the real private field rather than an unsafe cast.
- Around line 1187-1192: The updateDebugInfoPanel method is destroying and
recreating the entire debug panel every frame causing GC and allocation churn;
change it to create the panel once and only update text values: ensure
createDebugInfoPanel() constructs the Container/Graphics/Text objects and stores
references (e.g., this.debugInfoPanel, this.debugTextFields or similar) as
fields, call createDebugInfoPanel() only when debug is first enabled (or when
debugInfoPanel is null), and modify updateDebugInfoPanel() to simply call a new
refreshDebugInfoText() that updates existing Text objects via setText() instead
of destroying/creating the panel; keep references to individual Text nodes so
layout/redraw is minimal.
In `@packages/shared/src/world.ts`:
- Around line 3-8: The ZoneBounds type is duplicated; remove the local type in
WorldPackLoader.ts and import the shared definition instead. In
WorldPackLoader.ts add ZoneBounds to the existing import from the shared package
(so import { ZoneBounds } from 'packages/shared' or the existing shared module
import), then delete the local ZoneBounds type block (the lines currently
defining x, y, width, height). Ensure any references in functions/methods in
WorldPackLoader.ts continue to use ZoneBounds after the import.
🧹 Nitpick comments (7)
packages/shared/src/world.ts (2)
57-64:ZONE_IDS를ZONE_BOUNDS에서 파생하는 것을 고려하세요.
ZONE_IDS가 수동으로 관리되고 있어ZoneId타입이나ZONE_BOUNDS키와 불일치할 가능성이 있습니다.ZONE_BOUNDS에서 자동으로 파생하면 유지보수 부담을 줄일 수 있습니다.♻️ 제안된 변경
-export const ZONE_IDS: readonly ZoneId[] = [ - 'plaza', - 'north-block', - 'west-block', - 'east-block', - 'south-block', - 'lake', -] as const; +export const ZONE_IDS: readonly ZoneId[] = Object.keys(ZONE_BOUNDS) as ZoneId[];
66-68:getZoneBoundsMap()은 호출마다 새Map을 생성합니다.현재 사용 빈도가 낮아 문제가 되지 않지만, 핫 패스에서 반복 호출될 경우 불필요한 할당이 발생할 수 있습니다. 필요 시 모듈 레벨 캐싱을 고려하세요.
packages/client/src/ui/Minimap.ts (1)
83-119: 물/도로 영역의 hard-coded 타일 좌표가 남아있습니다.zone 데이터는 shared 상수에서 가져오지만, 물(lines 83-93)과 도로(lines 95-119) 영역의 타일 좌표는 여전히 매직 넘버로 하드코딩되어 있습니다. 향후 맵 변경 시 함께 업데이트가 필요한 부분입니다. 이번 PR 범위를 넘어서지만 참고용으로 남깁니다.
packages/client/src/game/scenes/GameScene.ts (2)
558-558: Static analysis:forEach콜백에서 불필요한 반환값.
label.destroy()의 반환값이 arrow function에 의해 암묵적으로 반환됩니다. 중괄호를 추가하여 의도를 명확히 하세요.♻️ 제안된 변경
- this.debugZoneLabels.forEach(label => label.destroy()); + this.debugZoneLabels.forEach(label => { label.destroy(); });
583-586:Entity타입에currentZone을 반복적으로 unsafe cast하고 있습니다.
(entity as unknown as { currentZone?: ZoneId }).currentZone패턴이 3곳(lines 584-586, 756-758, 968)에서 반복됩니다.Entity타입에currentZone필드가 실제로 존재하지만 타입 정의에 누락된 것이라면, 타입을 확장하는 것이 근본적 해결책입니다. 그렇지 않다면 최소한 헬퍼 함수로 추출하여 cast를 한 곳에서만 관리하세요.Also applies to: 755-758, 968-968
scripts/sync-maps.sh (2)
1-26: 대상 디렉토리 존재 여부 확인이 없습니다.
cp전에 대상 디렉토리가 존재하지 않으면 실패합니다. 특히 fresh clone이나 CI 환경에서 발생할 수 있습니다.mkdir -p로 안전하게 보호하세요.♻️ 제안된 변경
echo "Syncing maps from world pack..." +mkdir -p "$(dirname "$SERVER_MAP")" +mkdir -p "$(dirname "$CLIENT_MAP")" cp "$SOURCE_MAP" "$SERVER_MAP" cp "$SOURCE_MAP" "$CLIENT_MAP"
7-9: 소스와 대상 파일명 불일치에 대한 문서화를 권장합니다.소스 맵은
grid_town_outdoor.json이지만 대상은village.json입니다. 이 의도적인 이름 차이에 대해 스크립트 내 주석으로 이유를 명시하면 향후 유지보수에 도움이 됩니다.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that centralizes world configuration into a shared package, improving consistency between the client and server. It also includes a crucial map synchronization script to fix collision issues and enhances the F3 debug overlay. A security review found no vulnerabilities. However, there are areas for improvement regarding performance in the new debug panel, type safety, and script robustness.
| private updateDebugInfoPanel(): void { | ||
| if (!this.debugEnabled || !this.debugInfoPanel) return; | ||
|
|
||
| this.debugInfoPanel.destroy(); | ||
| this.createDebugInfoPanel(); | ||
| } |
There was a problem hiding this comment.
Calling destroy() and createDebugInfoPanel() on every frame within the update loop is highly inefficient. This will lead to performance issues and garbage collection stutter due to constant object creation and destruction.
A more performant approach is to create the panel and its text elements once, and then only update the text content each frame. I recommend the following refactor:
- Add a class property
private debugInfoTexts: Phaser.GameObjects.Text[] = [];. - In
createDebugInfoPanel, create the text objects and push them tothis.debugInfoTexts. This function should only be called once when the debug view is enabled (e.g., insideupdateCollisionDebug). - In
updateDebugInfoPanel, you would then get the new data and update the.textproperty of eachPhaser.GameObjects.Textobject inthis.debugInfoTexts.
| } | ||
| }); | ||
| }); | ||
| (this as unknown as { _debugBlockedCount: number })._debugBlockedCount = blockedCount; |
There was a problem hiding this comment.
Using a type assertion (this as unknown as ...) to set _debugBlockedCount suggests that the property is missing from the class definition. This bypasses TypeScript's type safety and makes the code harder to maintain and reason about.
To fix this, please declare _debugBlockedCount as a private property at the top of the GameScene class:
private _debugBlockedCount = 0;This will provide proper type-checking and improve code clarity.
| private checkZoneChange(entity: Entity): void { | ||
| const currentZone = (entity as unknown as { currentZone?: ZoneId }).currentZone; | ||
| if (!currentZone) return; | ||
| const currentZone = (entity as unknown as { currentZone?: ZoneId }).currentZone ?? null; |
There was a problem hiding this comment.
The type assertion (entity as unknown as { currentZone?: ZoneId }) is used here and in other places to access currentZone. This suggests that the Entity type definition is missing this property. Repeatedly using unknown bypasses type safety and makes the code less maintainable.
The server-side EntitySchema appears to have this property. To improve type safety, the client-side Entity type (likely defined in src/network/ColyseusClient.ts) should be updated to include currentZone?: ZoneId;.
scripts/sync-maps.sh
Outdated
| echo " -> Client: $CLIENT_MAP" | ||
| echo "" | ||
| echo "MD5 checksums:" | ||
| md5 "$SOURCE_MAP" "$SERVER_MAP" "$CLIENT_MAP" 2>/dev/null || md5sum "$SOURCE_MAP" "$SERVER_MAP" "$CLIENT_MAP" |
There was a problem hiding this comment.
The use of md5 ... || md5sum ... is a clever way to handle command differences between macOS and Linux. However, redirecting stderr with 2>/dev/null can hide legitimate errors if the md5 command fails for reasons other than not being found.
A more robust approach is to explicitly check for the command's existence first. This makes the script's behavior clearer and avoids suppressing potentially useful error messages.
Consider replacing this line with:
if command -v md5sum &> /dev/null; then
md5sum "$SOURCE_MAP" "$SERVER_MAP" "$CLIENT_MAP"
elif command -v md5 &> /dev/null; then
md5 "$SOURCE_MAP" "$SERVER_MAP" "$CLIENT_MAP"
else
echo "Warning: Could not find md5 or md5sum to verify checksums."
fi- GameScene: add private _debugBlockedCount field (type-safe) - GameScene: optimize debug panel to update text instead of recreating - GameScene: remove unnecessary type assertions for Entity.currentZone - WorldPackLoader: import ZoneBounds from shared, remove duplicate type - sync-maps.sh: improve md5 command detection with explicit checks - sync-maps.mjs: add cross-platform Node.js version for Windows support - package.json: use Node.js script for sync-maps (Windows compatible)
1 participant
Summary
@openclawworld/sharedProblem
Server was using an outdated
village.json(64x52, 277 blocked tiles) while client had the correct map (64x64, 546 blocked tiles). This caused:Solution
1. Unified World Constants (
packages/shared/src/world.ts)ZONE_BOUNDS: Zone boundary definitionsDEFAULT_SPAWN_POINT: Spawn location (1024, 1024)MAP_CONFIG: Map dimensions (64x64 tiles, 2048x2048 pixels)ZONE_COLORS,DEBUG_COLORS: UI color palettesZONE_IDS: List of all zone identifiers2. Server Integration
ZoneSystem: UsesgetZoneBoundsMap()from sharedGameRoom: UsesDEFAULT_SPAWN_POINTfrom shared3. Client Integration
GameScene: Uses shared constants for F3 debug overlayMinimap: Uses shared constants for zone rendering4. Map Synchronization
scripts/sync-maps.shto sync world pack map to server/clientpnpm buildruns sync first)Commits
feat(shared): Add unified world configuration constantsrefactor(server): Use shared world constantsrefactor(client): Use shared world constantsbuild: Add map synchronization mechanismTesting
pnpm typecheckpassespnpm testpasses (958 tests)pnpm buildsucceedsF3 Debug Mode Enhancement
Press F3 in-game to see:
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선사항