Skip to content

Add ability to set RAM #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 12, 2020
Merged

Conversation

ivylee
Copy link

@ivylee ivylee commented Nov 12, 2020

setRAM allows changing the Atari RAM values by their index.

ale = multi_agent_ale_py.ALEInterface()
ram = ale.getRAM()  # getRAM returns a numpy uint8 array with current RAM values
ale.setRAM(np.array(0x18, dtype='uint8'), np.array(0, dtype='uint8'))  # sets RAM value at index 24 to 0 
ram = ale.getRAM()  # RAM is updated

This is useful for implementing overrides of game state, e.g. introducing randomness or external rules for the game.

Related stale PRs: Farama-Foundation#247, Farama-Foundation#232. We would like to contribute our changes upstream. It seems the best option is to include the changes in this repo as it is used in our project.

@benblack769
Copy link

This is a relatively minor change, so it should be fine to merge.

However, I have two questions:

  1. Why is this needed? I understand wanting to get additional information from the RAM, but I no use cases come to mind for setting values. The PRs mentioned don't seem to be clear about this either (there is a broken link in one).
  2. As far as design, it makes more sense to me for the setRAM function to accept integers instead of numpy arrays. The setRAM method can then just do ordinary bounds checking to make sure it is in the 0-255 range. Does this make sense to you?
ale.setRAM(0x18, 0)  # sets RAM value at index 24 to 0 

@ivylee
Copy link
Author

ivylee commented Nov 12, 2020

  1. For example, in Space Invaders game, we can randomly perturb the location of the bullets fired. It's a form of handicap.
  2. Absolutely. Made the change in 611849d.

@benblack769
Copy link

Looks good.

We are intending to eventually try to merge this whole project into the main ALE project, so we will likely make a small PR with this change into that project soon (so the huge PR down the road will have fewer API changes).

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