Skip to content

fix(gfootball): only increment elapsed_step when episode is not done#418

Closed
hobostay wants to merge 1 commit into
sail-sg:mainfrom
hobostay:fix/gfootball-elapsed-step-counter
Closed

fix(gfootball): only increment elapsed_step when episode is not done#418
hobostay wants to merge 1 commit into
sail-sg:mainfrom
hobostay:fix/gfootball-elapsed-step-counter

Conversation

@hobostay

Copy link
Copy Markdown
Contributor

Fix the elapsed_step counter in GfootballOracle to only increment when the episode is still active, not after the episode is already done. Previously elapsed_step was incremented on every step including the done step, causing it to be one higher than expected at episode end.

@hobostay
hobostay force-pushed the fix/gfootball-elapsed-step-counter branch from 13d68c7 to 9920efd Compare May 22, 2026 15:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 13d68c779b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 299 to +302
if self._step >= self._max_episode_steps:
done = True

self._elapsed_step += 1
else:
self._elapsed_step += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Increment elapsed_step on horizon terminal step

elapsed_step is intended to track the number of external step() calls, but this change skips the increment when _step >= _max_episode_steps. On episodes that end by time limit, the terminal transition is still a real action step, so the oracle now reports an elapsed_step that is one too small on the last step. That creates an off-by-one mismatch against EnvPool’s step-count semantics and can break gfootball alignment checks for tasks that truncate at horizon.

Useful? React with 👍 / 👎.

@Trinkle23897

Copy link
Copy Markdown
Collaborator

I checked this against the pinned upstream Google Research Football source. Upstream FootballEnvCore.step() increments its step-call counter before evaluating terminal conditions, including the self._step >= self._env.config.game_duration horizon check. steps_left is derived directly from the engine info.step.

So the terminal transition is supposed to count toward EnvPool’s public elapsed_step. The current native behavior and the previous oracle behavior are aligned with upstream; this PR changes only the oracle to skip the terminal transition, which makes the align test expect 3000 while the native env correctly reports 3001 on horizon truncation.

I also verified this locally by restoring the unconditional increment and running:

bazel test --test_output=errors --config=test --spawn_strategy=local -- //envpool/gfootball:gfootball_align_test

Result: //envpool/gfootball:gfootball_align_test passed in 243.2s.

Recommendation: do not merge this as-is. Either close it, or replace it with a small comment documenting the upstream step-counting semantics so this does not look like an off-by-one bug later.

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.

2 participants