258 lines
8.2 KiB
Markdown
258 lines
8.2 KiB
Markdown
# Phase 2D: LineOfSightManager.cpp Cleanup - Complete ✅
|
||
|
||
**Date**: 2025-10-25
|
||
**Status**: ✅ COMPLETE - Minor cleanup applied
|
||
**File**: `src/modules/Playerbot/AI/Combat/LineOfSightManager.cpp`
|
||
**Build Result**: Zero errors, clean compilation
|
||
|
||
---
|
||
|
||
## Executive Summary
|
||
|
||
LineOfSightManager.cpp was found to be **already well-optimized** with spatial grid queries from PHASE 1 deadlock fixes. Only minor cleanup was performed to remove redundant alive checks and update phase labels for consistency.
|
||
|
||
### Key Results:
|
||
- **ObjectAccessor calls**: 3 → 3 (no reduction - already optimized with spatial grid)
|
||
- **Redundant checks removed**: 3 IsAlive() calls eliminated
|
||
- **Phase labels updated**: "DEADLOCK FIX" → "PHASE 1 & 2D" (4 occurrences)
|
||
- **Build status**: Clean compilation, zero errors
|
||
|
||
---
|
||
|
||
## Changes Made
|
||
|
||
### 1. Removed Redundant IsAlive() Checks
|
||
|
||
**Functions Updated**:
|
||
- `GetVisibleEnemies()` - Lines 338-340
|
||
- `GetVisibleAllies()` - Lines 380-383
|
||
- `CheckUnitBlocking()` - Lines 646-651
|
||
|
||
**Pattern**:
|
||
|
||
**Before**:
|
||
```cpp
|
||
// Query nearby creature GUIDs (lock-free!)
|
||
std::vector<ObjectGuid> nearbyGuids = spatialGrid->QueryNearbyCreatureGuids(
|
||
_bot->GetPosition(), maxRange);
|
||
|
||
// Resolve GUIDs to Unit pointers
|
||
for (ObjectGuid guid : nearbyGuids)
|
||
{
|
||
::Unit* unit = ObjectAccessor::GetUnit(*_bot, guid);
|
||
if (!unit || !unit->IsAlive()) // REDUNDANT - spatial grid only returns alive creatures
|
||
continue;
|
||
|
||
// Process unit...
|
||
}
|
||
```
|
||
|
||
**After**:
|
||
```cpp
|
||
// Query nearby creature GUIDs (lock-free!)
|
||
std::vector<ObjectGuid> nearbyGuids = spatialGrid->QueryNearbyCreatureGuids(
|
||
_bot->GetPosition(), maxRange);
|
||
|
||
// Resolve GUIDs to Unit pointers
|
||
for (ObjectGuid guid : nearbyGuids)
|
||
{
|
||
// PHASE 2D: Spatial grid already returns only alive creatures, no need for IsAlive() check
|
||
::Unit* unit = ObjectAccessor::GetUnit(*_bot, guid);
|
||
if (!unit) // Only null check needed
|
||
continue;
|
||
|
||
// Process unit...
|
||
}
|
||
```
|
||
|
||
**Impact**: Eliminates 3 redundant IsAlive() checks per function call.
|
||
|
||
---
|
||
|
||
### 2. Updated Phase Labels
|
||
|
||
**Changed**: All "DEADLOCK FIX" comments → "PHASE 1 & 2D"
|
||
|
||
**Affected Lines**: 316, 358, 575, 623
|
||
|
||
**Reason**: Consistency with current Phase 2D work and to acknowledge both PHASE 1 (spatial grid migration) and PHASE 2D (cleanup) efforts.
|
||
|
||
---
|
||
|
||
## Analysis Findings
|
||
|
||
### Why No ObjectAccessor Reduction?
|
||
|
||
All 3 ObjectAccessor calls were **already necessary**:
|
||
|
||
1. **All 3 calls return Unit* or std::vector<Unit*>** - API contract requires Unit* pointers
|
||
2. **All 3 calls already use spatial grid GUID queries** - Lock-free queries from PHASE 1
|
||
3. **QueryNearbyCreatureGuids() already filters for alive creatures** - IsAlive() check was redundant
|
||
|
||
### Functions That MUST Call ObjectAccessor
|
||
|
||
- `GetVisibleEnemies()` - Returns std::vector<Unit*>
|
||
- `GetVisibleAllies()` - Returns std::vector<Unit*>
|
||
- `CheckUnitBlocking()` - Returns bool, but needs Unit* for ToPet(), position checks
|
||
|
||
All these functions have API contracts that require Unit* pointers for processing.
|
||
|
||
---
|
||
|
||
## Performance Impact
|
||
|
||
### ObjectAccessor Call Frequency
|
||
|
||
**Before**: 3 calls per LOS query × 5-10 Hz = 15-30 calls/sec (estimated)
|
||
**After**: 3 calls per LOS query × 5-10 Hz = 15-30 calls/sec
|
||
**Reduction**: 0 calls/sec (file already optimized with spatial grid)
|
||
|
||
### Redundant Check Elimination
|
||
|
||
**Before**: 3 IsAlive() checks per function call
|
||
**After**: 0 IsAlive() checks per function call (spatial grid validation only)
|
||
**Benefit**: Minor CPU savings in IsAlive() validation
|
||
|
||
**FPS Impact**: <0.5% (micro-optimization, not measurable)
|
||
|
||
---
|
||
|
||
## Roadmap Discrepancy
|
||
|
||
### Expected vs. Actual
|
||
|
||
**Roadmap Estimate** (PHASE2_NEXT_STEPS_ROADMAP.md):
|
||
- Expected reduction: 2 calls (66%)
|
||
- Expected FPS impact: Part of 8-12% Phase 2D gain
|
||
|
||
**Actual Result**:
|
||
- Actual reduction: 0 calls (0%)
|
||
- Actual FPS impact: <0.5% (redundant check cleanup only)
|
||
|
||
**Root Cause**: File had already been optimized in PHASE 1 (deadlock fix - spatial grid migration).
|
||
|
||
---
|
||
|
||
## Build Validation
|
||
|
||
### Build Command
|
||
```bash
|
||
cd /c/TrinityBots/TrinityCore/build
|
||
cmake --build . --target worldserver --config RelWithDebInfo -- -m
|
||
```
|
||
|
||
### Build Result
|
||
```
|
||
playerbot.vcxproj -> C:\TrinityBots\TrinityCore\build\src\server\modules\Playerbot\RelWithDebInfo\playerbot.lib
|
||
worldserver.vcxproj -> C:\TrinityBots\TrinityCore\build\bin\RelWithDebInfo\worldserver.exe
|
||
```
|
||
|
||
### Warnings
|
||
- None (clean build)
|
||
|
||
### Compilation Errors
|
||
✅ **ZERO ERRORS**
|
||
|
||
---
|
||
|
||
## PHASE 1 Deadlock Fix Documentation
|
||
|
||
**Key Comments** (Lines 316, 358, 575, 623):
|
||
```cpp
|
||
// PHASE 1 & 2D: Use lock-free spatial grid instead of Cell::VisitGridObjects
|
||
```
|
||
|
||
**Background**:
|
||
LineOfSightManager.cpp was part of the PHASE 1 deadlock fix, where Cell::VisitGridObjects (lock-heavy grid iteration) was replaced with DoubleBufferedSpatialGrid queries (lock-free GUID queries).
|
||
|
||
**PHASE 1 Changes**:
|
||
- GetVisibleEnemies(): Cell::VisitGridObjects → QueryNearbyCreatureGuids
|
||
- GetVisibleAllies(): Cell::VisitGridObjects → QueryNearbyCreatureGuids
|
||
- CheckObjectBlocking(): Cell::VisitGridObjects → QueryNearbyGameObjectGuids
|
||
- CheckUnitBlocking(): Cell::VisitGridObjects → QueryNearbyCreatureGuids
|
||
|
||
---
|
||
|
||
## Lessons Learned
|
||
|
||
### 1. PHASE 1 Files Have Spatial Grid Queries
|
||
|
||
LineOfSightManager.cpp, like TargetScanner.cpp, was fully migrated to spatial grid queries in PHASE 1. These files have:
|
||
- Lock-free GUID queries instead of Cell::VisitGridObjects
|
||
- Comments labeled "DEADLOCK FIX" or "PHASE 1"
|
||
- No ObjectAccessor elimination opportunity (must resolve GUIDs to Unit*)
|
||
|
||
### 2. Spatial Grid Queries Return Only Alive Entities
|
||
|
||
**QueryNearbyCreatureGuids()** pre-filters for alive creatures, making IsAlive() checks redundant:
|
||
|
||
```cpp
|
||
// Spatial grid query (internal filtering):
|
||
std::vector<ObjectGuid> guids = spatialGrid->QueryNearbyCreatureGuids(pos, range);
|
||
// Only returns GUIDs of creatures with IsAlive() == true
|
||
|
||
// Caller resolves GUIDs:
|
||
for (ObjectGuid guid : guids)
|
||
{
|
||
Unit* unit = ObjectAccessor::GetUnit(*bot, guid);
|
||
if (!unit->IsAlive()) // ALWAYS REDUNDANT!
|
||
continue;
|
||
}
|
||
```
|
||
|
||
**Lesson**: Always check spatial grid query implementation before adding IsAlive() checks.
|
||
|
||
### 3. Return Type Constraints Persist Across All Phases
|
||
|
||
Even with PHASE 1 spatial grid optimization, functions returning Unit* or vector<Unit*> MUST call ObjectAccessor to satisfy API contracts.
|
||
|
||
**PHASE 1 Optimization**: Reduced lock contention by querying GUIDs instead of iterating objects
|
||
**PHASE 2D Opportunity**: Remove redundant validation checks after GUID resolution
|
||
|
||
---
|
||
|
||
## Comparison to Similar Files
|
||
|
||
### Already-Optimized Combat Files Pattern
|
||
|
||
| File | ObjectAccessor Calls | PHASE 1 Work | Phase 2D Work |
|
||
|------|---------------------|--------------|---------------|
|
||
| TargetScanner.cpp | 0 | Full migration (returns GUIDs) | Label update only |
|
||
| LineOfSightManager.cpp | 3 | Spatial grid queries | Redundant check cleanup |
|
||
| BotThreatManager.cpp | 14 | Snapshot validation | Redundant check cleanup |
|
||
| TargetSelector.cpp | 4 | Snapshot validation | Redundant check cleanup |
|
||
|
||
**Pattern**: Files with "DEADLOCK FIX", "PHASE 1", or "PHASE 5B" labels are already optimized with spatial grid or snapshot patterns.
|
||
|
||
---
|
||
|
||
## Conclusion
|
||
|
||
LineOfSightManager.cpp required only **minor cleanup** (remove 3 redundant checks, update 4 labels). The file is already well-optimized with PHASE 1 spatial grid queries.
|
||
|
||
**Key Takeaway**: PHASE 1 deadlock fixes eliminated lock contention but didn't eliminate ObjectAccessor calls for functions returning Unit*. Phase 2D cleanup focuses on removing redundant validation checks.
|
||
|
||
---
|
||
|
||
## Next Steps
|
||
|
||
Move to the next file in Phase 2D priority:
|
||
|
||
1. ✅ **ThreatCoordinator.cpp** (23 calls) - COMPLETE (65% reduction)
|
||
2. ✅ **BotThreatManager.cpp** (14 calls) - COMPLETE (cleanup only)
|
||
3. ✅ **GroupCombatTrigger.cpp** (5 calls) - COMPLETE (40% reduction)
|
||
4. ✅ **TargetSelector.cpp** (4 calls) - COMPLETE (cleanup only)
|
||
5. ✅ **TargetScanner.cpp** (3 calls) - COMPLETE (already 100% optimized in PHASE 1)
|
||
6. ✅ **LineOfSightManager.cpp** (3 calls) - COMPLETE (cleanup only)
|
||
7. ⏳ **InterruptAwareness.cpp** (3 calls) - NEXT
|
||
8. ⏳ **CombatBehaviorIntegration.cpp** (3 calls)
|
||
|
||
---
|
||
|
||
**Status**: ✅ LINEOFSIGHTMANAGER COMPLETE (Cleanup Only)
|
||
**Next**: InterruptAwareness.cpp (3 calls)
|
||
|
||
---
|
||
|
||
**End of LineOfSightManager.cpp Summary**
|