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

More control over offscreen dimensions and scene geometries #731

Merged
merged 16 commits into from
Oct 11, 2023

Conversation

guyazran
Copy link
Contributor

@guyazran guyazran commented Oct 2, 2023

Description

Offscreen Renderers do not allow control of viewer dimensions or the maximum number of geometries in the scene. We fix this issue with minimal implementation to address #730. This is motivated by the need to have multiple renderers at different resolutions and to render cluttered scenes

Fixes #730

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (none required)
  • My changes generate no new warnings
  • 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

gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
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.

@guyazran Thanks for the PR
@Kallinteris-Andreas This looks good to me, are you happy for me to merge?

tests/envs/mujoco/test_mujoco_rendering.py Outdated Show resolved Hide resolved
gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
@Kallinteris-Andreas
Copy link
Collaborator

also need to add max_geom args to

def __init__(
self,
model_path,
frame_skip,
observation_space: Space,
render_mode: Optional[str] = None,
width: int = DEFAULT_SIZE,
height: int = DEFAULT_SIZE,
camera_id: Optional[int] = None,
camera_name: Optional[str] = None,
default_camera_config: Optional[Dict[str, Union[float, int]]] = None,
):
if MUJOCO_IMPORT_ERROR is not None:
raise error.DependencyNotInstalled(

and
self.mujoco_renderer = MujocoRenderer(
self.model, self.data, default_camera_config, self.width, self.height
)

@guyazran
Copy link
Contributor Author

guyazran commented Oct 3, 2023

@Kallinteris-Andreas I have addressed all of your comments in my latest push (4 commits). all tests and pre-commits pass

@guyazran
Copy link
Contributor Author

guyazran commented Oct 3, 2023

added another commit to use the actual DEFAULT_SIZE value from the mujoco_env module in my new tests. no need for my redundant defaults.

gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
gymnasium/envs/mujoco/mujoco_rendering.py Outdated Show resolved Hide resolved
tests/envs/mujoco/test_mujoco_rendering.py Show resolved Hide resolved
Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

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

Summary of the changes this PR adds:

  • max_geom argument for mujocoEnv & MujocoRenderer
  • a test for MuJoCo rendering
  • a few extra comments

Note: does not add "More control over offscreen dimensions" unlike what the title says, since that functionality was already present

TODO (after this PR): add support for multi-camera rendering to fix #730 (I will do that)

@guyazran thanks
@pseudo-rnd-thoughts merge

@pseudo-rnd-thoughts
Copy link
Member

Could you also look at the ignored errors

Exception ignored in: <function WindowViewer.__del__ at 0x7f8f90ce4c10>
Traceback (most recent call last):
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 356, in __del__
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 349, in free
AttributeError: 'NoneType' object has no attribute 'get_current_context'
Exception ignored in: <function WindowViewer.__del__ at 0x7f8f90ce4c10>
Traceback (most recent call last):
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 356, in __del__
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 349, in free
AttributeError: 'NoneType' object has no attribute 'get_current_context'
Exception ignored in: <function WindowViewer.__del__ at 0x7f8f90ce4c10>
Traceback (most recent call last):
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 356, in __del__
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 349, in free
AttributeError: 'NoneType' object has no attribute 'get_current_context'
Exception ignored in: <function WindowViewer.__del__ at 0x7f8f90ce4c10>
Traceback (most recent call last):
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 356, in __del__
  File "/usr/local/gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py", line 349, in free
AttributeError: 'NoneType' object has no attribute 'get_current_context'

@guyazran
Copy link
Contributor Author

guyazran commented Oct 5, 2023

like I said in a previous note, by removing the optional width and height we cannot create a MujocoRenderer object for offscreen rendering. Still, I forgot to consider this in my own tests. fixed in f8ae8f4

@guyazran
Copy link
Contributor Author

guyazran commented Oct 5, 2023

the ignored exceptions mentioned by @pseudo-rnd-thoughts here are still present. I don't why they are occurring and I am not able to reproduce them on my local machine. I'm going to wildly guess that this is because I didn't close the viewers after usage. I address this in 89db1c3

@guyazran
Copy link
Contributor Author

guyazran commented Oct 5, 2023

according to the workflows running in my fork, the ignored exceptions are now gone.

@guyazran
Copy link
Contributor Author

Is there a reason the change is not being merged? Are there any other issues I need to fix?

@pseudo-rnd-thoughts
Copy link
Member

Apologies, I completely forgot about it, merging now

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit f255122 into Farama-Foundation:main Oct 11, 2023
11 checks passed
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.

[Proposal] Support custom width, height, and scene geometries in the MuJoCo renderer
3 participants