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

[Feature] Adding human render to gym wrapper #944

Merged
merged 11 commits into from
Sep 20, 2022

Conversation

vincentpierre
Copy link
Contributor

Motivation and Context

  • Removing HabRenderWrapper since it seems it only adds extra metrics to the rendering (these metrics should be added to the task configuration if they are needed). Please advise
  • Using pygame to render the environment in human mode in a similar manner to interactive play. Made it so that if pygame is not installed, human render mode will not be usable. Please advise.
  • Future? Remove the HabitatRenderXXX default gym tasks. These add the third RGB camera as an observation so I find adding Render to the name is misleading. Thoughts?

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • 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 change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@vincentpierre vincentpierre requested a review from ASzot September 9, 2022 19:06
@vincentpierre vincentpierre self-assigned this Sep 9, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 9, 2022
Copy link
Contributor

@ASzot ASzot left a comment

Choose a reason for hiding this comment

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

Looks good, but we should have test for human render mode.

@vincentpierre
Copy link
Contributor Author

Looks good, but we should have test for human render mode.

YES x 1000.
I think I would like some advice on that front. I could add pygame to CI to try to make a test there BUT it seems hard to make sure rendering works properly. At best we can test for errors but it will be hard to test the rendering is actually working properly...

@ASzot
Copy link
Contributor

ASzot commented Sep 12, 2022

I think a good first step is adding pygame to CI and then just checking for errors.

@vincentpierre
Copy link
Contributor Author

Ok, I added a test. Looks like pygame is already on CI.

@vincentpierre
Copy link
Contributor Author

I do not think pygame cannot run on CI because there is no monitor. This will make this feature very hard to test on CI. Do you have suggestions?

@vincentpierre vincentpierre marked this pull request as ready for review September 15, 2022 19:25
@vincentpierre
Copy link
Contributor Author

I will be using Mock to replace pygame in the test. This way I will avoid having to open the pygame window on the CI machine. It will also prevent having the pygame window open when testing locally, which would shift the cursor focus (would be unnecessarily distracting)

@vincentpierre vincentpierre changed the title adding human render to gym wrapper [Feature] Adding human render to gym wrapper Sep 15, 2022
Copy link
Contributor

@mathfac mathfac left a comment

Choose a reason for hiding this comment

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

Overall looks good, some light comments.

self._screen = pygame.display.set_mode(
[frame.shape[1], frame.shape[0]]
)
draw_frame = np.transpose(frame, (1, 0, 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

That's great to add a comment like (C, H, W) -> (H, C, W) for transpose operations.

)
draw_frame = np.transpose(frame, (1, 0, 2))
draw_frame = pygame.surfarray.make_surface(draw_frame)
self._screen.fill((0, 0, 0)) # type: ignore[attr-defined]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid magic unnamed numerical consts:

Suggested change
self._screen.fill((0, 0, 0)) # type: ignore[attr-defined]
BLACK_COLOR = (0, 0, 0)
self._screen.fill(BLACK_COLOR) # type: ignore[attr-defined]

@vincentpierre vincentpierre merged commit 56889cd into facebookresearch:main Sep 20, 2022
@vincentpierre vincentpierre deleted the pygame_gym_render branch September 20, 2022 17:00
dannymcy pushed a commit to dannymcy/habitat-lab that referenced this pull request Jul 8, 2024
* adding human render to gym wrapper

* adding rendering to the test

* adding logging to CI to debug the error

* using moduleskip

* mocking pygame to not have a pygame display during testing

* reloading module

* less verbose CI

* removing useless method calls

* changing the default to human for render just like the gym interface does

* addressing comments

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
HHYHRHY pushed a commit to SgtVincent/EMOS that referenced this pull request Aug 31, 2024
* adding human render to gym wrapper

* adding rendering to the test

* adding logging to CI to debug the error

* using moduleskip

* mocking pygame to not have a pygame display during testing

* reloading module

* less verbose CI

* removing useless method calls

* changing the default to human for render just like the gym interface does

* addressing comments

Co-authored-by: vincentpierre <vincentpierre@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants