MapViewer fro Camera

[nighca]

close #2147.

[niupilot]

PR Review for #2172

Starting code review for the map editor implementation...

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze new map editor components
• Review modifications to existing editor/preview components
• Check code quality and best practices
• Look for potential bugs and issues
• Verify documentation consistency
• Provide comprehensive review feedback

---

🔍 Code Review Summary

This PR implements a comprehensive map editor feature with physics support and camera functionality. Overall, the implementation shows good architectural patterns, but there are several areas for improvement.

🏗️ Architecture & Design

✅ Strengths:

• Good separation of concerns: Map editor is properly isolated in its own module structure
• Consistent file organization: Following existing patterns with components, models, and utilities
• Vue 3 Composition API: Proper use of modern Vue patterns with <script setup>
• Reusable components: Smart extraction of shared viewer components (NodeTransformer, SpriteNode)

⚠️ Areas for Improvement:

1. Code duplication between MapViewer.vue and StageViewer.vue:

   • Both files have nearly identical viewport logic, zoom handling, and drag controls
   • Consider extracting shared functionality into composables or mixins
   • Lines like viewport calculations, mouse event handlers are duplicated

2. Missing type safety in several places:

   • spx-gui/src/components/editor/map-editor/MapViewer.vue:220 - console.warn with dynamic values could be type-checked
   • Missing proper TypeScript interfaces for camera scroll notifications

🐛 Potential Issues

Critical Issues:

1. Incomplete implementation in basic config components:

   <!-- MapBasicConfig.vue:16 -->
   <div style="height: 200px">Basic Config for map</div>

   • MapBasicConfig.vue and SpriteBasicConfig.vue are just placeholders
   • This could be confusing for users expecting functional controls

2. TODO comments indicate unfinished work:

   // spx-gui/src/components/editor/map-editor/map-viewer/MapViewer.vue:91
   function getValidMapPos(pos: Pos) {
     return pos // TODO
   }

Minor Issues:

3. Inconsistent naming patterns:

   • CameraScrollNotifyFn vs other type naming conventions
   • Some functions use camelCase while others don't follow consistent patterns

4. Missing error boundaries:

   • spx-gui/src/components/editor/map-editor/map-viewer/MapViewer.vue:298 - No null checks for stage operations
   • Camera scroll logic could fail if stage is not ready

🎯 Best Practices & Code Quality

Good Practices:

• Proper Vue 3 reactivity with computed, ref, and reactive
• Clean component prop definitions with TypeScript
• Good use of Konva.js for canvas operations
• Proper cleanup in drag end handlers

Suggestions for Improvement:

1. Extract shared viewport logic:

   // Consider creating: composables/useViewport.ts
   export function useViewport(containerSize, mapSize) {
     // Shared zoom, pan, and viewport calculations
   }

2. Add proper error handling:

   // spx-gui/src/components/editor/map-editor/map-viewer/MapViewer.vue:298
   function handleContextMenu(e: KonvaEventObject<MouseEvent>) {
     e.evt.preventDefault()
     
     if (!stageRef.value?.getStage()) {
       console.warn('Stage not ready for context menu')
       return
     }
     // ... rest of implementation
   }

3. Complete the basic config components:

   • Implement actual form controls for map size, physics settings
   • Add proper validation and change handlers

🔧 Model Changes Review

Project Model (spx-gui/src/models/project/index.ts):

• ✅ Good addition of camera follow functionality
• ✅ Proper cleanup when sprites are removed
• ⚠️ defaultViewportSize should be configurable rather than hardcoded

Stage Model (spx-gui/src/models/stage.ts):

• ✅ Clean physics configuration structure
• ✅ Proper typing for physics properties
• ⚠️ Physics defaults could be more explicit (current defaults to { enabled: false })

📚 Documentation & Consistency

Issues Found:

1. Missing documentation for new public APIs:

   • Camera follow functionality needs documentation
   • Physics configuration options need examples

2. CLAUDE.md references non-existent files:

   • References @.github/instructions/*.md files that don't exist
   • Should either create these files or remove references

🎯 Specific Recommendations

1. High Priority:

   • Complete the placeholder implementations in MapBasicConfig.vue and SpriteBasicConfig.vue
   • Add proper error handling to prevent runtime crashes
   • Extract shared viewport logic to reduce duplication

2. Medium Priority:

   • Add comprehensive TypeScript interfaces for all new types
   • Implement proper validation for physics and camera settings
   • Add unit tests for new model methods

3. Low Priority:

   • Improve naming consistency across the codebase
   • Add JSDoc comments for public methods
   • Consider adding animation transitions for camera movements

🔍 Security & Performance

• ✅ No security concerns identified
• ✅ Proper cleanup of intervals and event listeners
• ⚠️ Consider debouncing zoom operations for better performance
• ⚠️ Large sprite lists might impact rendering performance

🎯 Overall Assessment

This is a solid foundation for the map editor feature. The architecture is sound and follows Vue.js best practices. However, several components are incomplete placeholders that should be implemented before merging to avoid user confusion.

Recommendation: Request changes to complete the basic config components and add proper error handling before approval.

---

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

[qiniu-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[nighca]

这次把 custom-transformer、Nodetransformer、SpriteNode 等比较容易抽出来的做了下复用，在src/components/editor/common/viewer/ 目录下

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[qiniu-ci]

This PR has been deployed to the preview environment. You can explore it using the preview URL.

Warning

Please note that deployments in the preview environment are temporary and will be automatically cleaned up after a certain period. Make sure to explore it before it is removed. For any questions, contact the XBuilder team.

[Overu]

这里的Pos指的是spx的Sprite的x, y？

[nighca]

类型 Pos 本身是跟坐标系无关的，不管在哪个坐标系里的位置，都可以是 Pos 类型的数据

如注释

/** Check if given position (in game) is in viewport */
function inViewport({ x, y }: Pos) {

所述，这里 inViewport 的参数 { x, y }: Pos 预期是游戏世界（舞台）坐标系的，如 sprite 的 x、y