Add vector.HumanRendering wrapper
#880
Conversation
…arama-Foundation#872, Farama-Foundation#233) Corrected code to pass tests, and tests to test code! Also corrected some bugs in vectorized Cartpole Doctest: fixed comments Documentation was missing for vector.HumanRendering Relax 'render_mode' further, does not need to be correct until actual rendering starts (i.e. env.reset() is called)
|
@pseudo-rnd-thoughts @Kallinteris-Andreas please please review |
|
@pseudo-rnd-thoughts @Kallinteris-Andreas |
| @@ -510,7 +510,7 @@ def render(self): | |||
| for _ in range(self.num_envs) | |||
| ] | |||
| if self.clocks is None: | |||
| self.clock = [pygame.time.Clock() for _ in range(self.num_envs)] | |||
There was a problem hiding this comment.
Why is cartpole.py being changed on a PR about adding a new wrapper? Shouldn't this be a separate PR?
There was a problem hiding this comment.
Had to, as cartpole.py had bugs making it impossible to add tests of vector rendering (and it was used in testing of the existing regular rendering)
There was a problem hiding this comment.
Can you make a separate PR with just those fixes, where it can be reviewed alone, (then after merging you can rebase this PR)
There was a problem hiding this comment.
If it is acceptable to do it without creating related tests, and let the tests arrive with the rendering commit.
I changed the tests and added quite a number of tests for both HumanRenderer.
(but in that case the CartPole commit will be so small that it is easy to review)
Note: the changed method in CartPoleVectorEnv is render() [and it can not have ever worked] so it is not unrelated to this commit.
There was a problem hiding this comment.
The testing of VecCartPole's rgb_array render_mode, should be independent of human rendering testing
There was a problem hiding this comment.
why is the name of the folder base_
There was a problem hiding this comment.
It is internal routines moved from both HumanRendering and the new vector.HumanRendering
should not be used directly (and _base was not possible to use at it gave errors)
As these routines are used in both HumanRendering and vector.HumanRendering they cannot be placed in HumanRendering - creates a circular dependency, could be placed in vector.HumanRendering but it would be strange for regular HumanRendering calling it - thus 'base_'
| from gymnasium.error import DependencyNotInstalled | ||
|
|
||
|
|
||
| ALL_ACCEPTABLE_RENDER_MODES = ["rgb_array", "rgb_array_list"] |
There was a problem hiding this comment.
Should also support "depth_array" (edit: perhaps a separate PR)
There was a problem hiding this comment.
Sorry I have taken so long to review this PR
As this implementation is for vector.HumanRendering, could you only modify the code related to the vector version (unless there is necessary changes for testing)
It seems to add complexity with the base_.rendering code, could you remove this for the wrappers.rendering and wrappers.vector.rendering
I understand this removes duplicated code but this breaks the project conventions
| for state, screen, clock in zip(self.state, self.screens, self.clocks): | ||
| x = self.state.T | ||
|
|
||
| for state, screen, clock, x in zip( |
There was a problem hiding this comment.
I believe this will fail for num_envs>4 as zip uses the shortest iterate and self.state has shape (N, 4) so self.state.T is (4, N).
Could you confirm if this does is work for num_envs>4?
pseudo-rnd-thoughts
changed the title
vector.HumanRendering wrapper
|
Closing in favor of #1013 |
Corrected code to pass tests, and tests to test code! Also corrected some bugs in vectorized Cartpole
Doctest: fixed comments
Documentation was missing for vector.HumanRendering
Relax 'render_mode' further, does not need to be correct until actual rendering starts (i.e. env.reset() is called)
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 #872
Fixes #873
Related to #233
Type of change
Please delete options that are not relevant.
Screenshots
Checklist:
pre-commitchecks withpre-commit run --all-files(seeCONTRIBUTING.mdinstructions to set it up)