Fix NPC title stacking issue when players leave visible range

[Bkm016]

Fixes #110

This PR addresses the issue where NPC titles (holograms) were not properly removed when players left the NPC's visible range, causing multiple titles to stack when players re-entered the area.

Changes

• Enhanced hologram cleanup logic: Improved the remove() method to handle edge cases and synchronize both lookup maps properly
• Strengthened title creation: Added proper cleanup of existing holograms before creating new ones
• Improved player quit handler: Added safe navigation and memory leak prevention
• Fixed dual lookup map synchronization: Ensures playerLookup and entityLookup stay in sync

Root Cause

The issue was in the TraitTitle class where the hologram cleanup logic had synchronization problems between the dual lookup maps when players moved out of the NPC's visible range. This could result in incomplete cleanup, leaving stale hologram references that caused duplication when players re-entered the area.

Testing

• Paper 1.12.2 compatibility maintained
• Logic validated for proper error handling
• Memory leak prevention implemented

Generated with Claude Code

[claude]

Claude finished @Bkm016's task —— View job

---

Code Review Complete ✅

I've analyzed the TraitTitle.kt file changes that fix the NPC title stacking issue. Here's my comprehensive review:

Todo List:

• Read and analyze the modified TraitTitle.kt file
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

---

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Excellent improvements overall! The code follows Kotlin conventions well and demonstrates solid understanding of concurrency patterns.

Strengths:

• ✅ Proper use of ConcurrentHashMap for thread-safe dual lookup maps
• ✅ Good separation of concerns with clear method responsibilities
• ✅ Comprehensive cleanup logic that handles edge cases
• ✅ Safe navigation operators (?.) used appropriately
• ✅ Clear, descriptive method and variable names
• ✅ Good use of Kotlin idioms like computeIfAbsent

Minor suggestions:

• Consider extracting magic numbers like 0.25 (default title height) to named constants
• The nested map structures could benefit from type aliases for readability:

  typealias PlayerHologramMap = MutableMap<String, AdyeshachHologram>
  typealias EntityHologramMap = MutableMap<String, AdyeshachHologram>

Bug Analysis 🐛

Key fixes implemented correctly:

• ✅ Dual map synchronization: Lines 117-123 properly maintain both playerLookup and entityLookup in sync
• ✅ Comprehensive cleanup: remove() method (lines 169-189) now handles edge cases by checking both lookup maps
• ✅ Memory leak prevention: Empty map cleanup in onQuit() (lines 83-85) and remove() (lines 183-188)
• ✅ Race condition prevention: Using computeIfAbsent prevents concurrent modification issues

Potential edge case:

• Lines 174-180: The fallback logic hologram1 ?: hologram2 assumes both lookups reference the same hologram instance. This is correct based on the creation logic, but could be fragile if the code evolves.

Performance Considerations ⚡

Good performance practices:

• ✅ ConcurrentHashMap provides efficient concurrent access
• ✅ Early returns prevent unnecessary processing (line 111, 138)
• ✅ Batch operations in player quit handler are efficient
• ✅ Proper cleanup prevents memory accumulation

Scheduling concern:

• Line 52: The update task runs every 100ms (5 seconds). For servers with many NPCs and players, this could impact performance. Consider making the interval configurable or implementing a smarter update strategy that only processes entities with dynamic content.

Security Assessment 🔒

No security issues identified. The code:

• ✅ Safely handles concurrent access patterns
• ✅ Properly validates input (empty message check on line 110)
• ✅ Uses safe navigation to prevent NPEs
• ✅ No exposure of sensitive information

Test Coverage 📋

Missing test coverage: No unit tests are included with these changes. Consider adding tests for:

• Dual lookup map synchronization scenarios
• Player quit edge cases
• Concurrent access patterns
• Memory leak prevention
• Hologram cleanup on entity removal

Technical Deep Dive 🔍

Root cause fix analysis:
The original issue was in the remove() method where incomplete cleanup could leave stale references. The new implementation (lines 169-189) solves this by:

1. Dual removal: Removing from both lookup maps independently
2. Fallback cleanup: If one lookup fails, the other serves as backup
3. Container cleanup: Empty maps are removed to prevent memory leaks
4. Atomic operations: Each lookup operation is atomic via ConcurrentHashMap

Create method improvements (lines 102-124):

• Proactive cleanup before creating new holograms prevents duplication
• Defensive programming with old hologram cleanup (lines 118, 123)

Recommendation: ✅ APPROVE

This PR successfully addresses the title stacking issue with well-engineered solutions. The dual lookup approach with proper synchronization is the right architectural choice for this use case. The code quality is high and follows best practices for concurrent programming.

Minor follow-up suggestions:

1. Add unit tests for the edge cases
2. Consider making the update interval configurable
3. Monitor performance impact of the 100ms update cycle in production

---