From 14774a801f8937840f372258139c04a3af8622bf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 20:38:40 +0000 Subject: [PATCH] fix(rl): credit hit reward to the action that caused the collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GameSimulator.Update runs UpdateBall (which sets paddle.HasHitBall) before UpdatePaddles applies the current frame's action. When HasHitBall is true in frame N, the collision happened with the paddle position established by action N-1, not the action just buffered in this frame's Decide(). RegisterReward(+1) was attaching the only positive signal to buffer[Count-1] — the unrelated action just decided — turning the hit reward into pure noise and explaining the persistent policy collapse (paddle always drifts up or down without learning to track the ball). Add RegisterRewardForPreviousAction that writes to buffer[Count-2] and use it for the hit-step reward. Terminal reward in EndEpisode still uses the last action (correct: it credits the action that preceded the miss). Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014E7V6Etbi1qzrTDpFbtsp2 --- .../Agents/AIReinforcementAgent.cs | 18 ++++++++++++++++++ src/PingPongAI.App/MainWindow.xaml.cs | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/PingPongAI.AI/Agents/AIReinforcementAgent.cs b/src/PingPongAI.AI/Agents/AIReinforcementAgent.cs index e23dd53..59fc10e 100644 --- a/src/PingPongAI.AI/Agents/AIReinforcementAgent.cs +++ b/src/PingPongAI.AI/Agents/AIReinforcementAgent.cs @@ -117,6 +117,24 @@ public void RegisterReward(double reward) _episodeBuffer[_episodeBuffer.Count - 1].Reward += reward; } + // Attaches a reward to the action recorded one tick earlier rather + // than the most recent one. Required when the simulator's update + // order makes a consequence observable only on the next tick: + // GameSimulator.Update runs UpdateBall (collision detection, sets + // paddle.HasHitBall) BEFORE UpdatePaddles applies the current + // frame's action. So when HasHitBall is true in frame N, the + // collision was caused by the paddle position established by + // action N-1, not the action just buffered in Decide() this frame. + // Crediting +1 to Count-1 would attribute the only positive signal + // to a random unrelated action — the policy never learns. + public void RegisterRewardForPreviousAction(double reward) + { + if (_episodeBuffer.Count < 2) + return; + + _episodeBuffer[_episodeBuffer.Count - 2].Reward += reward; + } + // Closes the current ralli: attaches the terminal reward to the // last step, computes discounted returns backward, and runs one // policy-gradient pass per step. diff --git a/src/PingPongAI.App/MainWindow.xaml.cs b/src/PingPongAI.App/MainWindow.xaml.cs index d3393ca..f27c173 100644 --- a/src/PingPongAI.App/MainWindow.xaml.cs +++ b/src/PingPongAI.App/MainWindow.xaml.cs @@ -239,8 +239,11 @@ private void UpdateRL(IPongAgent agent, GameState previousState, GameState curre : currentState.RightPaddle; // Step reward: +1 every frame the paddle hit the ball. + // Credit the *previous* action because UpdateBall (which sets + // HasHitBall) runs before UpdatePaddles inside GameSimulator — + // the collision used paddle position from the prior tick. if (paddle.HasHitBall) - rl.RegisterReward(+1.0); + rl.RegisterRewardForPreviousAction(+1.0); // Ralli end: score changed between the snapshot and the current state. bool leftScored = currentState.LeftScore > previousState.LeftScore;