Potential fix for Montezuma Revenge segmentation fault#670
Closed
Sebastian-Griesbach wants to merge 2 commits intoFarama-Foundation:masterfrom
Closed
Potential fix for Montezuma Revenge segmentation fault#670Sebastian-Griesbach wants to merge 2 commits intoFarama-Foundation:masterfrom
Sebastian-Griesbach wants to merge 2 commits intoFarama-Foundation:masterfrom
Conversation
Unable to reliably reproduce error thus unable to verify that this fixes it
Author
|
I missed that someone already made a Pull request for this... #668 withdrawing mine. |
|
The funny thing is I only got involved because of Lucas Beyer's tweet |
Author
Lol, thank you for sharing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #11 has been a long standing issue. I recently ran into segmentation faults when using Montezuma Revenge such that it seemed reasonable to assume that this is the same issue.
However here is a problem. I am unable to verify that this snippet is causing the issue. Because I am unable to reliably reproduce the bug in the first place. The description of it in the issue no longer seems to be working, yet I can regularly observer segmentation faults only when using Montezuma Revenge as an environment.
Heavily assisted by an coding agent I could identify this potential issue.
Here are fives variables
myPOSP0,myPOSP2,myPOSM0, myPOSM1andmyPOSBL`.These variables are supposed to be constrained to the range [0,159], the original code uses two if cases to check if first it is larger than 160 if so subtract 160 and if it is smaller then 0 to add 160. This does not work if the value as already deviated so much that adding or subtracting 160 is not enough, e.g. if the value would be 350, we would subtract 160 and arrive at 190 which will then not be changed anymore and later on causes a segmentation fault.
I propose to change this to use modulo operations instead such that this operation will land in the intended interval regardless of how far away the incoming value is.
While I can not guarantee that this actually fixes the segmentation fault I'd argue that this is keeping the intended behavior and is excluding an edge cases. Even if this is not fixing the segmentation fault there should be no adverse effects connected to fixing this little oversight.
Looking forward to hear what you think.