feat: Movement Animation Foundation with A* Integration (TD_060)

[Coelancanth]

Summary

Complete movement animation foundation (TD_060) with A* pathfinding integration, plus comprehensive technical analysis of animation-related issues (TD_061, TD_062). This PR establishes the foundation for smooth, visually-correct movement with proper fog of war updates.

Changes Included

TD_060: Movement Animation Foundation ✅ COMPLETE

• Implemented non-blocking AnimateMovementAsync using CallDeferred pattern
• Cell-by-cell animation along exact A* pathfinding routes
• Perfect synchronization between preview dots and actual movement
• Path calculated BEFORE domain state changes to avoid self-blocking
• VERIFIED: All files exist, build clean (0 warnings/errors), 688/692 tests passing

TD_062: Sprite Clipping Fix (Approved for Implementation)

• Problem Identified: Linear interpolation causes sprites to cut through walls at corners
• Solution Designed: Sub-cell waypoint system with 30% offset at direction changes
• Architecture: Uses ADR-006 Animation Event Bridge pattern for callback control
• Complexity: Reduced to 2.5h by following established patterns

TD_061: Progressive FOV Updates (Correctly Diagnosed)

• CRITICAL CORRECTION: Issue is FOV updates, NOT camera smoothing
• Real Problem: Field of View updates instantly at destination, revealing areas before actor arrives
• Expected Behavior: FOV should update cell-by-cell during movement animation
• Solution: Reuse TD_062's callback pattern for progressive fog of war revelation

Technical Implementation

Files Modified (TD_060)

• Views/ActorView.cs - Non-blocking AnimateMovementAsync with CallDeferred
• src/Darklands.Presentation/Views/IActorView.cs - Added interface method
• src/Darklands.Presentation/Presenters/ActorPresenter.cs - HandleActorMovedWithPathAsync
• src/Darklands.Presentation/Presenters/GridPresenter.cs - Pre-calculates A* path
• src/Darklands.Presentation/Presenters/IActorPresenter.cs - Updated interface

Architecture Alignment

• ✅ ADR-006: Direct Godot usage in View layer (no unnecessary abstraction)
• ✅ ADR-010: UIEventBus integration for movement completion events
• ✅ Clean separation: Logic provides paths, rendering handles visual details

Key Technical Insights

Animation System Patterns

1. Callback-Driven Animation: IMovementAnimationEvents enables precise control
2. Sub-Cell Waypoints: Elegant solution for corner navigation without complex math
3. Progressive FOV Updates: Game mechanic correctness over visual polish

Godot Integration Lessons

• Tween + async/await deadlock: Use CallDeferred for thread-safe operations
• Path timing critical: Calculate BEFORE domain state changes
• Animation callbacks essential: Enable cross-layer coordination without coupling

Future Work Enabled

This foundation enables:

• VS_012: Vision-Based Movement System (unblocked)
• TD_061: Progressive FOV implementation using callback pattern
• TD_062: Corner-aware sprite movement using waypoint system

Testing Results

• Build: 0 warnings, 0 errors (verified twice)
• Tests: 688/692 passing (4 skipped, 0 failed)
• Manual: Movement animation works smoothly with A* paths
• Integration: Animation matches preview dots perfectly

🤖 Generated with Claude Code

[claude]

🔍 Code Review for PR #66: Movement Animation Foundation

Summary

This PR successfully implements smooth movement animation for actors in the grid-based game, integrating with the existing A* pathfinding system. The implementation demonstrates good architectural separation between domain and presentation layers.

✅ Strengths

1. Architecture Alignment

• Excellent adherence to ADR-006 (direct Godot usage in View layer)
• Clean separation of concerns between domain state and visual representation
• Proper use of the presenter pattern to coordinate between layers

2. Non-blocking Animation Solution

• Smart use of CallDeferred to prevent main thread deadlocks
• Queue-based approach ensures thread safety
• Avoids the common Godot pitfall of await ToSignal causing freezes

3. Path Pre-calculation Strategy

• Calculating A* path BEFORE domain state changes prevents self-blocking issues
• Ensures preview dots and actual movement use identical paths
• Good fallback to straight-line movement when pathfinding fails

⚠️ Areas for Improvement

1. Magic Numbers

// In ProcessPendingPathAnimations
var segmentDuration = 1.0f / animData.Speed;  // Consider extracting to const

Consider defining animation constants at the class level for easier tuning.

2. Logging Verbosity

The implementation has extensive logging at Info level which might flood production logs:

_logger.Log(LogLevel.Information, LogCategory.Gameplay,
    "[CREATING TWEENS] Starting from index {StartIndex}...");

Consider using Debug level for detailed animation logs, reserving Info for key events only.

3. Error Recovery

While there's good try-catch handling, consider what happens if an animation fails mid-sequence. The actor's visual position might become desynced from domain state.

🐛 Potential Issues

1. Concurrent Animation Race Condition

If multiple move commands arrive quickly for the same actor, the queue processing might create overlapping tweens. Consider canceling existing tweens before starting new ones:

// Suggestion: Store and cancel previous tween
if (_activeTweens.TryGetValue(actorId, out var existingTween))
{
    existingTween.Kill();
}

2. Memory Management

The Queue<PendingPathAnimation> could theoretically grow unbounded if animations are queued faster than processed. Consider adding a maximum queue size check.

3. Path Validation

The path animation assumes all positions in the path are valid. Consider adding bounds checking:

if (gridPos.X < 0 || gridPos.Y < 0 || gridPos.X >= GridWidth || gridPos.Y >= GridHeight)
{
    _logger.LogWarning("Invalid path position: {0}", gridPos);
    continue;
}

🎯 Performance Considerations

1. Tween Creation Overhead: Creating multiple tween properties in a loop is fine for small paths, but might cause frame drops with very long paths (>20 tiles). Consider batching or limiting visible animation segments.

2. Lock Contention: The lock (_pendingPathAnimations) could become a bottleneck under heavy load. The current implementation is fine for typical gameplay, but consider lock-free structures if performance becomes an issue.

🔒 Security Review

No security concerns identified. The implementation properly validates actor IDs and handles missing actors gracefully.

📈 Test Coverage

With 688/692 tests passing and only 4 skipped (no failures), the test coverage appears solid. The manual testing confirms the animation works smoothly.

💡 Suggestions for Future Improvements

1. Animation Easing: Consider adding easing curves to make movement feel more natural:

   tween.SetTrans(Tween.TransitionType.Sine);
   tween.SetEase(Tween.EaseType.InOut);

2. Animation Events: The groundwork for IMovementAnimationEvents mentioned in TD_062 would be valuable for coordinating with other systems (sound, particles, etc.)

3. Performance Metrics: Consider adding timing metrics to track animation performance and detect when animations are falling behind.

✅ Verdict: APPROVED WITH SUGGESTIONS

This is a solid implementation that correctly solves the movement animation problem while maintaining clean architecture. The non-blocking approach using CallDeferred is particularly clever and avoids common Godot pitfalls. The suggested improvements are mostly for edge cases and future robustness rather than critical issues.

The PR successfully completes TD_060 and provides a strong foundation for the sprite clipping fix (TD_062) and camera follow feature (TD_061).

Great work on navigating the threading complexities and maintaining architectural integrity! 🎉

---

Review generated by Claude Code