Adding timeout termination to PitFall#667
Adding timeout termination to PitFall#667Sebastian-Griesbach wants to merge 3 commits intoFarama-Foundation:masterfrom
Conversation
| int timer_seconds = readRam(&system, 0xD9); | ||
| // Game terminates when: (1) all lives lost and logo screen shown, OR (2) timer runs out (00:00) | ||
| bool timer_expired = (timer_minutes == 0 && timer_seconds == 0); | ||
| m_terminal = (lives_byte == 0 && logo_timer != 0) || timer_expired; |
There was a problem hiding this comment.
Thanks for the PR, whats the difference between the logo_timer and the actual timer that you've extracted?
Also, could this timer_exprired feature be enabled or disabled
There was a problem hiding this comment.
I think the logo_timer controls the timing of the scrolling of the logo. When it's active it scrolls between "Activision" and "Copyright 1982", this is not directly related to the in game timer. It just happens to co-inside with gameover which I assume is why this was originally used.
How should I add the mentioned option? As I understand I can not directly add an argument to the environment initialization. I can use a game "mode" that takes an int, is that the intended way?
|
I used the mode argument to add the option for the original behavior. You can test this but running the environment and counting the steps. The timer only start after the first not no-op action that's why we need to take another action in the first step. Here is an example for how to test this: In mode 0 the environment terminates after 20 minutes (18000 steps), |
|
So looking at the failed test, it seems that using the mode is not the correct way of implementing this option. Could you give me a hint @pseudo-rnd-thoughts what would be the intended way to implement such an option instead? I could also do it via a "difficulty" but that also seems hacky. |
|
My current best suggestion would be to drop the Option. As the fixed behavior is the intended behavior already present in the documentation. This would simplify the PR a lot. I can not think of a reason why someone would intentionally want to keep this bugged behavior unless for comparing to old results in which case one should anyways always use the same version the other method was run on. Let me know how to proceed, I'd be happy to resolve this issue outside of my local installation. |
It seems that the current code actually does not contain any intended detection for the Timeout.
logo_timeris non zero when the Activision Logo scrolls in the bottom left.This is happening
Apparently the
logo_timerwas put here because thelives_bytesalone can temporarily take on a zero value during the death transition.The current Termination condition
m_terminalthus only checks if all lives have been lost. There is no detection for the timer reaching zero. I added two variables tracking minutes and seconds and added it to the termination condition.I don't see how to reasonably add tests for this as the timer only exists
I wrote a little script that records the last view frames of a episode to visually confirm that the timer runs out right before timeout termination which I can provide if needed.
Edit: I did not know that I can not add issue references in the title: #654, #268