Skip to content

feat: implement DeterministicTickContext and add unit test for constr…#22

Merged
JohnLudlow merged 2 commits into
mainfrom
15-advance-deterministicenginerunner-simulationtime-tick-callback-configurable-camera-docsplansphase-2-completionmd-phase-2
May 1, 2026
Merged

feat: implement DeterministicTickContext and add unit test for constr…#22
JohnLudlow merged 2 commits into
mainfrom
15-advance-deterministicenginerunner-simulationtime-tick-callback-configurable-camera-docsplansphase-2-completionmd-phase-2

Conversation

@JohnLudlow
Copy link
Copy Markdown
Owner

@JohnLudlow JohnLudlow commented May 1, 2026

PR #22 Summary

Title: feat: implement DeterministicTickContext and add unit test for construction

Issue: #15 – Advance DeterministicEngineRunner with SimulationTime, tick callback, and configurable camera

Overview

This PR implements Phase 1 of issue #15: the introduction of DeterministicTickContext, a readonly record struct that encapsulates per-step context for deterministic simulation loops. This provides the foundation for planned Phase 2 features (SimulationTime property, callback system, and configurable camera).

Changes

New File:

  • src/GameEngineAdapter.Headless/DeterministicTickContext.cs (18 lines)
    • Readonly record struct with three init-only properties:
      • SimulationTime: TimeSpan representing simulated time at step start (accumulates across multiple Run calls)
      • Rng: Shared Random instance for deterministic outputs
      • Adapter: Reference to HeadlessAdapter for provider/state access
    • Fully documented with XML comments explaining parameter semantics and RNG sharing behavior

Modified File:

  • src/GameEngineAdapter.UnitTests/DeterministicEngineRunnerTests.cs
    • Added test: DeterministicTickContext_CanBeConstructedWithAllParameters (14 lines)
    • Verifies struct construction and property accessibility
    • Existing backward-compatibility tests remain unchanged

Impact

  • Backward Compatible: No breaking changes. Existing DeterministicEngineRunner API unchanged.
  • Value Type Performance: readonly record struct design avoids heap allocation—suitable for performance-sensitive simulation loops
  • Foundation: Enables Phase 2 implementation (Run method with callback parameter accepting DeterministicTickContext)
  • Test Coverage: 1 new test added; all 6 existing backward-compatibility tests pass

Technical Details

The struct uses modern C# readonly record syntax and follows the design specified in the issue #15 implementation plan. The RNG is documented as shared/consumed by callbacks to set expectations for deterministic sequence handling.

PR Stats

  • +37 lines, -1 line
  • 2 files changed
  • 1 commit
  • ✅ Mergeable (clean state)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new deterministic tick “context” type in the Headless adapter layer and introduces a unit test to validate it can be constructed and its members are accessible.

Changes:

  • Introduces DeterministicTickContext as a readonly record struct holding simulation time, RNG, and adapter references.
  • Adds an xUnit test verifying DeterministicTickContext construction and property access.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/GameEngineAdapter.Headless/DeterministicTickContext.cs Adds the new tick context value type and its XML documentation.
src/GameEngineAdapter.UnitTests/DeterministicEngineRunnerTests.cs Adds a unit test covering DeterministicTickContext construction and member exposure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

/// Per-step context provided to the tick callback in a deterministic simulation run.
/// </summary>
/// <param name="SimulationTime">
/// Simulated time at the start of this step. Accumulates across multiple <see cref="DeterministicEngineRunner.Run(int, System.Action{DeterministicTickContext}?)"/> calls.
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

The XML doc <see cref="DeterministicEngineRunner.Run(int, System.Action{DeterministicTickContext}?)"/> cannot be resolved (the overload doesn’t exist, and nullable ? annotations aren’t valid in cref type signatures). This will produce XML doc warnings (e.g., CS1574) and the remark about accumulating across multiple Run calls is currently inaccurate given the existing Run(int) implementation. Update the cref to point at an existing member (or remove the signature) and align the text with the current runner behavior (or implement the referenced overload).

Suggested change
/// Simulated time at the start of this step. Accumulates across multiple <see cref="DeterministicEngineRunner.Run(int, System.Action{DeterministicTickContext}?)"/> calls.
/// Simulated time at the start of this step.

Copilot uses AI. Check for mistakes.
}
}

// Phase 1 tests - will fail until DeterministicTickContext is implemented
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

This comment is now misleading: the PR implements DeterministicTickContext, and this test only validates construction/accessors (so it won’t “fail until implemented”). Please update or remove the “Phase 1 tests” note to reflect the current state.

Suggested change
// Phase 1 tests - will fail until DeterministicTickContext is implemented
// Construction/accessor coverage for DeterministicTickContext

Copilot uses AI. Check for mistakes.
@JohnLudlow
Copy link
Copy Markdown
Owner Author

@copilot rereview

Copy link
Copy Markdown
Contributor

Copilot AI commented May 1, 2026

@copilot rereview

Both review items were already resolved in commit 6251a5d:

  • Removed the invalid <see cref="DeterministicEngineRunner.Run(int, System.Action{DeterministicTickContext}?)"/> from the XML doc (that overload doesn't exist yet)
  • Updated the test comment from "Phase 1 tests" to "Construction/accessor coverage for DeterministicTickContext"

All 6 tests pass. Ready for re-review.

@JohnLudlow JohnLudlow merged commit 66f6161 into main May 1, 2026
14 checks passed
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.

Advance DeterministicEngineRunner — SimulationTime, tick callback, configurable camera (docs/plans/phase-2-completion.md Phase 2)

3 participants