feat: add automated performance benchmark harness

[MichaelFisher1997]

Summary

• Add a benchmark harness that records FPS, CPU/GPU timing, draw calls, vertices, and chunk counts to JSON.
• Wire benchmark mode through the existing app flow and keep headless runs stable with safe-mode settings.
• Add comparison tooling plus a GitHub Actions benchmark workflow targeting dev.

Validation

• nix develop --command zig build test
• nix develop --command zig build benchmark -Dbenchmark-duration=3 -Dbenchmark-output=/tmp/benchmark-results/test.json under headless Weston

[github-actions]

Now I have a comprehensive view of the PR. Let me provide the full review:

---

📋 Summary

PR #445 adds an automated performance benchmark harness to ZigCraft, including a benchmark runner, GitHub Actions workflow, comparison tooling, and camera path system for reproducible FPS/GPU timing measurements.

No linked issues found in PR body.

The code is well-structured with proper memory management patterns, good separation of concerns, and integrates cleanly with the existing app flow. Build passes successfully. However, there is one notable cleanup change in frame_manager.zig that removes dry_run guards without explanation.

---

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

✅ All previously reported critical issues have been resolved. (No previous automated reviews on file.)

---

⚠️ High Priority Issues (Should Fix)

None identified.

---

💡 Medium Priority Issues (Nice to Fix)

No issues requiring fixes. The benchmark implementation is solid.

---

ℹ️ Low Priority Suggestions (Optional)

[LOW] src/engine/graphics/vulkan/frame_manager.zig:93-118 - Undocumented behavioral change to fence synchronization

Confidence: High

Description: The dry_run conditionals around vkWaitForFences, vkResetFences, and submitGuarded were removed. Previously in dry-run/headless mode, these calls were skipped. Now they always execute. The PR description does not mention this change.

Impact: Low - The change appears correct (fences must be properly waited on/reset before reusing command buffers regardless of dry_run mode), but lack of documentation suggests it may have been incidental rather than intentional.

Suggested Fix: If intentional, add a comment explaining why dry_run no longer needs special fence handling. If unintentional, restore the conditionals.

---

[LOW] src/engine/graphics/rhi_types.zig:177 - Removed debug assertion (minor cleanup)

Confidence: Low

Description: The assertion std.debug.assert(tile_id != Vertex.LOD_TILE_ID) was removed from encodeMeta. This was a debug-only check. The initLOD function correctly uses LOD_TILE_ID directly, so the removal is correct, but it's a tangential cleanup not described in the PR.

Impact: None - This is a legitimate removal since LOD vertices legitimately use the sentinel value.

---

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9/10	benchmark.zig cleanly handles only benchmark state, waypoints, and result aggregation
Open/Closed	9/10	New benchmark system extends existing RHI/game flow without modifying core systems
Liskov Substitution	8/10	RHI interface properly extended with getDrawCallCount; BenchmarkRunner integrates via interface
Interface Segregation	9/10	Small IDeviceQuery interface with focused getDrawCallCount addition
Dependency Inversion	9/10	BenchmarkRunner depends on abstractions (GpuTimingResults, WorldStats) not concrete implementations
Average	8.8

---

🎯 Final Assessment

Overall Confidence Score: 87%

Confidence Breakdown:

• Code Quality: 88% (well-structured, proper allocator usage, clean patterns)
• Completeness: 85% (full benchmark flow implemented, but no unit tests for benchmark.zig)
• Risk Level: 15% (low - frame_manager change is the only non-obvious modification)
• Test Coverage: 70% (build passes, but no dedicated benchmark unit tests)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0 (8.8)
• Overall confidence >= 60% (87%)
• No security concerns
• Build passes (zig build test succeeds)

Verdict:

MERGE

The benchmark harness is well-implemented with proper memory management, clean integration into the app lifecycle, and correct RHI extension. The only non-trivial change is the frame_manager fence synchronization cleanup which appears correct but undocumented.

---

{
  "reviewed_sha": "9907242ebfffc41d3e33abd8d54044f4dc22ba19",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 87,
  "recommendation": "MERGE"
}

opencode session  |  github run