Add independent review reports for Minimap system (Rounds 8, 9, and 26)
- Round 8 report highlights improvements in architecture, editor usability, and data robustness, with a total score of 80/100. - Round 9 report focuses on editor extension capabilities, identifying issues with room data indexing and layout editing, resulting in a score of 76/100. - Round 26 report evaluates the system against commercial standards, noting new issues and confirming previous fixes, with a score of 95.8/100.
This commit is contained in:
204
Docs/Review/Minimap_Review_Round25_Independent.md
Normal file
204
Docs/Review/Minimap_Review_Round25_Independent.md
Normal file
@@ -0,0 +1,204 @@
|
||||
# 小地图系统 R25 独立评审报告
|
||||
|
||||
**评审轮次**：Round 25（R24 修复后全面重审）
|
||||
**评审基准**：成熟 2D Metroidvania 游戏的商业级标准；编辑器工具须对开发者/策划可用友好
|
||||
|
||||
---
|
||||
|
||||
## 一、各维度评分
|
||||
|
||||
| 维度 | 满分 | 得分 | 变化 |
|
||||
|------|------|------|------|
|
||||
| 架构设计 & 解耦合 | 25 | 24.5 | ±0 |
|
||||
| 性能 & 运行时效率 | 20 | 19.5 | ±0 |
|
||||
| 编辑器工具质量 | 15 | 14.5 | ±0 |
|
||||
| 代码质量 & 可维护性 | 15 | 13.5 | -0.5 |
|
||||
| 存档 & 持久化 | 10 | 9.0 | -0.5 |
|
||||
| 可扩展性 | 10 | 9.5 | ±0 |
|
||||
| 功能完整性 | 5 | 5.0 | ±0 |
|
||||
| **综合** | **100** | **95.5** | **-2.0** |
|
||||
|
||||
> R24 得分：97.5 → R25 得分：95.5（发现 1 Critical + 2 Normal 问题）
|
||||
|
||||
---
|
||||
|
||||
## 二、确认正常的设计（R24 修复全部落地）
|
||||
|
||||
| 项 | 状态 |
|
||||
|----|------|
|
||||
| TeleportService 完整重建（135 行） | ✅ |
|
||||
| RequestTeleport 源 RoomId 改为 IPlayerPositionProvider.CurrentRoomId | ✅ |
|
||||
| MapInputHandler 删除 _zoom 双重状态，OnScroll 直接读写 _zoomTarget.localScale.x | ✅ |
|
||||
| MapLayoutEditorWindow 搜索框 ✕ 清除按钮 | ✅ |
|
||||
| 执行顺序链：MapManager(-700)→MapPlayerTracker(-600)→MapPinManager(-500)→TeleportService(-400)→UI(0) | ✅ |
|
||||
| ISaveable 防御性拷贝三处对称 | ✅ |
|
||||
| _servicesReady 短路（MapPanel & MinimapHUD 对称） | ✅ |
|
||||
| MapRoomDataSO.ChooseDisplayIcon DRY 消除 | ✅ |
|
||||
| MapServiceExtensions 集中 BuildRegionDict / ResolveRegionDisplayName | ✅ |
|
||||
| 三对象池：MapPanel（Cell/Exit/Pin）；MinimapHUD（Cell/Pin） | ✅ |
|
||||
| 发现动画协程自清理（R20-N1 RunRevealAnim） | ✅ |
|
||||
| MinimapHUD O(viewRadius²) 空间索引裁剪 | ✅ |
|
||||
| UnsubscribeServices 有意不对称注释说明 | ✅ |
|
||||
|
||||
---
|
||||
|
||||
## 三、新发现问题
|
||||
|
||||
### C1 — Critical：TeleportService.CanTeleportTo 逻辑反转（Fail-Open 安全漏洞）
|
||||
|
||||
**文件**：`Assets/_Game/Scripts/World/Map/TeleportService.cs`
|
||||
**行号**：L81–82
|
||||
|
||||
**问题**：
|
||||
```csharp
|
||||
_mapSvc ??= ServiceLocator.GetOrDefault<IMapService>();
|
||||
return _mapSvc == null || _mapSvc.IsExplored(roomId); // ← BUG
|
||||
```
|
||||
|
||||
当 `_mapSvc` 查询失败（服务未注册、场景切换中等）时，返回 `true`（允许传送）。
|
||||
而注释明确写道：**"需要玩家已探索过目标房间（仅已知位置才允许传送）"**。
|
||||
逻辑与意图完全相反：
|
||||
- 实际行为：`_mapSvc == null` → **无条件放行**（Fail-Open）
|
||||
- 预期行为：`_mapSvc == null` → **拒绝传送**（Fail-Safe）
|
||||
|
||||
**影响**：玩家可在 MapService 不可用时（存档切换、热重载等边缘时机）跳过探索校验，传送到从未到达过的房间，破坏探索核心机制。
|
||||
|
||||
**修复**：
|
||||
```csharp
|
||||
return _mapSvc != null && _mapSvc.IsExplored(roomId);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### N1 — Normal：MapPanel._subs 声明但永远为空（死代码）
|
||||
|
||||
**文件**：`Assets/_Game/Scripts/World/Map/MapPanel.cs`
|
||||
**行号**：L76（声明）、L109（Clear）
|
||||
|
||||
**问题**：
|
||||
```csharp
|
||||
private readonly CompositeDisposable _subs = new(); // 永远不会有内容
|
||||
// ...
|
||||
private void OnDisable()
|
||||
{
|
||||
_subs.Clear(); // ← 空操作
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
MapPanel 的所有事件订阅均通过直接 `+=` / `-=` 方式管理（在 `SubscribeServices` / `UnsubscribeServices` 中），没有任何订阅通过 `.AddTo(_subs)` 加入。`_subs.Clear()` 是一个完全的空操作。
|
||||
|
||||
**影响**：轻微；不影响运行时行为，但
|
||||
1. 误导后续维护者（以为订阅已通过 CompositeDisposable 管理）
|
||||
2. 在 OnDisable 中清理了并不存在的订阅，逻辑意图不清晰
|
||||
|
||||
**修复选项 A（推荐）**：删除 `_subs` 字段和 `_subs.Clear()` 调用，保持现有 `+=` 管理方式即可（已有 `UnsubscribeServices` 妥善处理）。
|
||||
|
||||
---
|
||||
|
||||
### N2 — Normal：MapInputHandler 缺少 _zoomTarget 配置校验
|
||||
|
||||
**文件**：`Assets/_Game/Scripts/World/Map/MapInputHandler.cs`
|
||||
**行号**：L21 注释、L97 & L382（MapPanel.CurrentZoom）
|
||||
|
||||
**问题**：
|
||||
- `MapInputHandler._zoomTarget` 注释写"通常为 _roomContainer（格子根节点）"
|
||||
- `MapPanel.CurrentZoom` 明确读取 `_roomContainer.localScale.x`
|
||||
- `MapInputHandler.OnScroll` 写入 `_zoomTarget.localScale`
|
||||
- 若在 Inspector 中将 `_zoomTarget` 配置为非 `_roomContainer` 的节点，两者的 scale 将静默分裂：`CurrentZoom` 读到的是旧值，`OnScroll` 写入的是新值，缩放操作功能性损坏但无任何报错
|
||||
|
||||
**影响**：仅在配置错误时触发，但属于"配置错误无提示"的隐患，在 Prefab 调试时难以定位。
|
||||
|
||||
**修复**：在 Awake 中添加运行时校验警告：
|
||||
```csharp
|
||||
private void Awake()
|
||||
{
|
||||
_panel = GetComponent<MapPanel>();
|
||||
#if UNITY_EDITOR || DEVELOPMENT_BUILD
|
||||
if (_zoomTarget != null && _panel != null)
|
||||
{
|
||||
// MapPanel.CurrentZoom 读取 _roomContainer，要求 _zoomTarget 与其为同一节点
|
||||
Debug.LogWarning("[MapInputHandler] _zoomTarget 应配置为 MapPanel 的 _roomContainer（格子根节点），" +
|
||||
"否则 CurrentZoom 与缩放操作将读写不同节点导致状态分裂。", this);
|
||||
}
|
||||
#endif
|
||||
}
|
||||
```
|
||||
|
||||
> 更优做法：提供 `public void SetZoomTarget(RectTransform t)` 由 MapPanel.OnEnable 注入，彻底消除配置依赖。
|
||||
|
||||
---
|
||||
|
||||
## 四、各文件详细状态
|
||||
|
||||
### MapPanel.cs
|
||||
- 架构：五层解耦（Interface → ServiceLocator → C# Events → UI → Object Pool）✅
|
||||
- 性能：三对象池 + PinsVersion 脏检查 + _servicesReady 短路 ✅
|
||||
- **死代码**：`_subs` 字段（N1）⚠️
|
||||
- 其余：对象池三种（Cell/Exit/Pin）均正确入池、出池、OnDestroy 销毁 ✅
|
||||
|
||||
### MinimapHUD.cs
|
||||
- O(viewRadius²) 空间索引裁剪 ✅
|
||||
- _servicesReady 短路 + 跨场景重连（null 服务引用 + 重置标志）✅
|
||||
- GC 友好：复用 `_toRemove`、`_roomsInViewBuffer`、`_newlyAddedBuffer` ✅
|
||||
- 无新问题 ✅
|
||||
|
||||
### MapInputHandler.cs
|
||||
- R24-N2 修复落地：无独立 `_zoom` 字段，OnScroll 直接读写 `_zoomTarget.localScale.x` ✅
|
||||
- **配置隐患**：缺少 `_zoomTarget` 校验（N2）⚠️
|
||||
|
||||
### TeleportService.cs
|
||||
- R24 完整重建架构正确：ISaveable / ITeleportService / 单例保护 ✅
|
||||
- **C1 逻辑反转**：`CanTeleportTo` 在 `_mapSvc == null` 时 Fail-Open ❌
|
||||
|
||||
### MapManager.cs
|
||||
- 执行顺序 (-700) / ISaveable / IMapService ✅
|
||||
- `GetRoomsByRegion` 懒加载缓存 ✅
|
||||
- `NotifyDatabaseChanged` 同步清空区域缓存 ✅
|
||||
|
||||
### MapPinManager.cs (MapPin.cs)
|
||||
- 执行顺序 (-500) / ISaveable / IPinService ✅
|
||||
- PinsVersion 脏版本号 ✅
|
||||
|
||||
### MapServiceExtensions.cs
|
||||
- 无状态扩展方法集 ✅
|
||||
- GetVisibility / CreatePinAtWorldPos / BuildRegionDict / ResolveRegionDisplayName ✅
|
||||
|
||||
### MapLayoutEditorWindow.cs（编辑器）
|
||||
- 拖拽/缩放/验证/搜索/图例/Play Mode 玩家点 ✅
|
||||
- ✕ 清除按钮（R24-N3）✅
|
||||
- MatchesRoomType RoomType 枚举搜索（R23-N2）✅
|
||||
- GUIStyle 缓存（避免 60fps × 100 房间重分配）✅
|
||||
- Undo.undoRedoPerformed 注册 ✅
|
||||
|
||||
---
|
||||
|
||||
## 五、修复优先级
|
||||
|
||||
| 编号 | 严重度 | 文件 | 影响 | 是否立即修复 |
|
||||
|------|--------|------|------|-------------|
|
||||
| C1 | 🔴 Critical | TeleportService.cs | Fail-Open 逻辑反转，破坏探索机制 | 是 |
|
||||
| N1 | 🟡 Normal | MapPanel.cs | 死代码误导维护 | 推荐 |
|
||||
| N2 | 🟡 Normal | MapInputHandler.cs | 配置错误无提示 | 推荐 |
|
||||
|
||||
---
|
||||
|
||||
## 六、评分历史
|
||||
|
||||
| 轮次 | 综合评分 | 主要变化 |
|
||||
|------|---------|---------|
|
||||
| R17 | 93.0 | 初轮大规模重构后 |
|
||||
| R18 | 93.8 | 平移/缩放 + 协程修复 |
|
||||
| R19 | 93.8 | 持平 |
|
||||
| R20 | 94.2 | DRY + 协程自清理 |
|
||||
| R21 | 95.0 | ServicesReady 对称 |
|
||||
| R22 | 96.5 | BuildRegionDict 集中 |
|
||||
| R23 | 97.5 | MatchesRoomType + 执行顺序 |
|
||||
| R24 | 97.5 | TeleportService 重建（但遗留 C1 逻辑错误）|
|
||||
| **R25** | **95.5** | 发现 C1（-1.5）+ N1（-0.25）+ N2（-0.25）|
|
||||
|
||||
> R25 评分低于 R24，因 R24 重建 TeleportService 时引入了 C1 逻辑反转错误（fail-open vs fail-safe），该问题在 R24 评审中被遗漏。
|
||||
|
||||
---
|
||||
|
||||
*R25 评审完成时间：2025*
|
||||
Reference in New Issue
Block a user