Skip to content
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

Fix classic environments screen sizes, add type hints, fix pre-commit #998

Conversation

elliottower
Copy link
Contributor

@elliottower elliottower commented Jun 9, 2023

Added screen_height argument to the rlcard envs and RPS env (they have dynamic widths depending on number of players, i used a slightly smaller default height for RPS as it was 350 previously, whereas rlcard was 1000px), added screen_scaling to connect_four because the rendering in that environment is a bit finnicky and only works for certain resolutions (the original ratio is 99 / 86, and any resolutions which multiple both numbers by integers work, but scaling by 2.5 for example results in white space because of rounding), and added screen_height to chess and go (originally used screen_width but it's simpler to have only two args to try out: screen_height for some envs, or for connect four it's screen_scaling, rather than having to remember 3 different ones for different envs)

I also added type hints to the classic envs as I was already going through and adding new args.

Almost all of the files had # noqa which ignores checks on the files globally (i.e., when doing new code it didn't even re-format according to black unless you removed the line), so I replaced that with specific ignores for the two pydocstring errors which are a result of us using the large docstrings at the top of environments to dynamically create the environment pages (starting with an H1 tag instead of full sentence with period).

I did change the default size of RPS to be bigger and go and connect to be slightly smaller and more normalized with everything else, but didn't change underlying envrionments so don't think it should need a new version

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue), Depends on # (pull request)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.
To upload images to a PR -- simply drag and drop or copy paste.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@elliottower
Copy link
Contributor Author

I may try to get dynamic scaling of pygame windows to work so users can just resize or hit maximize to change it without using arguments, think it's possible but may take a bit of work

@elliottower
Copy link
Contributor Author

Also need to look at the render FPS and make the environments go at the same speed, currently connect four for example goes incredibly fast and ends in like half a second, whereas go pauses for a second between moves or something like that, need to make it more consistent

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Does this screen size change effect rgb_array?

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 33aad74 into Farama-Foundation:master Jun 12, 2023
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