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

Make VideoRecorder backward-compatible to gym<0.23 #2678

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

vwxyzjn
Copy link
Contributor

@vwxyzjn vwxyzjn commented Mar 9, 2022

This PR makes VideoRecorder backward-compatible to gym<0.23. See #2675.

@vwxyzjn
Copy link
Contributor Author

vwxyzjn commented Mar 10, 2022

Hey @RedTachyon could you help review this please?

'`env.metadata["video.frames_per_second"] is marked as deprecated and will be replaced with `env.metadata["render_fps"]` '
"see https://github.com/openai/gym/pull/2654 for more details"
)
self.frames_per_sec = self.backward_compatible_frames_per_sec
Copy link
Contributor

@witoong623 witoong623 Mar 29, 2022

Choose a reason for hiding this comment

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

These assignment will create a new bug if an user implements only new render_fps.
What if we use get('render_fps') so that when it is not available, it is None, and then if the user doesn't have that value, then we go back to use video.frames_per_second (which default to 30) and warn the user.
I'm happy to make PR to fix this if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@witoong623 if you could make a PR for this, that'd be great

Copy link
Contributor

Choose a reason for hiding this comment

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

This was fixed in #2703

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.

4 participants