Skip to content

Conversation

@tryuan99
Copy link
Member

Resolves #203. During the simulation destruction, agents might already be destroyed, but the SimMonitor still wants to log one last telemetry data point for them. IAgent is an interface independent of MonoBehaviour, so the null check does not return correctly for fake-null objects. Instead, the agent has to be cast to AgentBase, which will then invoke the fake-null-compatible comparison with null.

@tryuan99 tryuan99 requested a review from daniellovell January 30, 2026 23:44
@stacklane-pr-stack-visualizer
Copy link

stacklane-pr-stack-visualizer bot commented Jan 30, 2026

🧱 Stack PR · Base of stack

Stack Structure:

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly addresses an issue with logging telemetry for destroyed agents by using a cast to AgentBase to handle Unity's 'fake-null' objects. The changes also improve efficiency by combining filters into a single LINQ Where clause and removing an unnecessary .ToList() call. I've added one suggestion to use OfType<T>() which can make the code more idiomatic and readable. Overall, this is a good improvement.

@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

The RecordTelemetry method in SimMonitor.cs was refactored to filter agents in a single pass using OfType() with a combined predicate (non-null, not terminated, and activeInHierarchy). The telemetry writer null check is performed before retrieving time. The loop now iterates directly over the filtered agents, removing the prior inner activeInHierarchy check and the intermediate list creation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • daniellovell
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main change: casting agents to MonoBehaviour type for proper null checking, which is the core fix for the null reference issue.
Description check ✅ Passed The description clearly explains the problem (fake-null issue with IAgent interface), the solution (casting to AgentBase), and references the linked issue #203.
Linked Issues check ✅ Passed The PR directly addresses issue #203 by casting agents to AgentBase (MonoBehaviour-derived type) to enable proper null checking for destroyed objects, preventing MissingReferenceException during SimMonitor teardown.
Out of Scope Changes check ✅ Passed All changes focus on the null-checking fix in RecordTelemetry; the refactoring to use OfType() filtering is a logical and necessary implementation detail directly supporting the core objective.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch titan/fix_telemetry_null

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Logging] Missing reference exception when destroying logging

2 participants