fixing dm_control rendering bug when human mode is used

[spiglerg]

There seems to be a bug at present, due to the fact that dm_control environments re-compile their physics (including the Model and Data structures) on each reset (to allow for, e.g., domain randomization). As such, rendering stops working after the first reset(). This commit fixes the problem by re-creating a new Viewer in the "human" mode.

Note that this problem is only related to "human" mode rendering, since the other pixel-based renderings call self._env.physics directly instead, which is guaranteed to be the latest (correct) object.

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 #90

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

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

[elliottower]

Thanks a ton for this, I don’t know enough about how dm control works personally to comment on if there’s a better way to do it than the updating of pointers but it doesn’t seem too hacky to me. Maybe @pseudo-rnd-thoughts has an idea about it?

Also curious if the tests I added in my other PR get fixed by this, don’t think we’ll add them officially as they require manual testing but for debugging and making sure the rendering works I think they may be useful to check an isolated problem. But it sounds like you’ve done some testing of your own which is good

[spiglerg]

I am not sure about the tests -- I am trying to set everything up locally to run them and see.

The original problem was due to https://github.com/deepmind/dm_control/blob/main/dm_control/composer/environment.py#L359 (calling recompile_physics from reset) which then frees and re-instantiate the ._physics object (which contains the new 'model' and 'data'): https://github.com/deepmind/dm_control/blob/main/dm_control/composer/environment.py#L250

[elliottower]

Makes sense, appreciate it. Approved the workflows so we’ll see if those pass (i imagine so because it wasn’t a problem caught by our tests before)

On Wed, Jul 26, 2023 at 11:26 AM spiglerg @.***> wrote:

I am not sure about the tests -- I am trying to set everything up locally to run them and see.

The original problem was due to https://github.com/deepmind/dm_control/blob/main/dm_control/composer/environment.py#L359 (calling recompile_physics from reset) which then frees and re-instantiate the ._physics object (which contains the new 'model' and 'data'):

https://github.com/deepmind/dm_control/blob/main/dm_control/composer/environment.py#L250

— Reply to this email directly, view it on GitHub https://github.com/Farama-Foundation/Shimmy/pull/104#issuecomment-1652038988, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVPVA2S5JFMSZARQ74SNPLXSEZLLANCNFSM6AAAAAA2YX3MSM . You are receiving this because you commented.Message ID: @.***>

[spiglerg]

Awesome. :)