-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
[Proposal] New rendering API #2540
Comments
I agree for fast throughput envs we need a different rendering mechanism and thus some changes are warranted. Here are some thoughts.
Do you mean "does not"? This is not necessarily true. For example, the following can extract a smooth video. env = gym.make(gym_id)
env = gym.wrappers.RecordEpisodeStatistics(env)
env = gym.wrappers.RecordVideo(env, f"videos/{run_name}")
env = NoopResetEnv(env, noop_max=30)
env = MaxAndSkipEnv(env, skip=4)
env = EpisodicLifeEnv(env)
if "FIRE" in env.unwrapped.get_action_meanings():
env = FireResetEnv(env)
env = ClipRewardEnv(env)
env = gym.wrappers.ResizeObservation(env, (84, 84))
env = gym.wrappers.GrayScaleObservation(env)
env = gym.wrappers.FrameStack(env, 4) Also, I think the existing APIs have a few benefits
openaigym.video.0.1.video007000_c1d7901b.2.mp4Perhaps an alternative way to handle the fast throughput envs is to introduce a |
@vwxyzjn there are still issues even if you appropriately stack the wrappers. As @jkterry1 pointed out, things like frameskip become a problem, especially with the human rendering mode. When stacking wrappers the final call to What I really want to see is more control over the Using this method for rendering doesn't change much from a wrapper standpoint. You can still override Furthermore, this method of rendering would align better with the vector environment API. Calling |
After thinking about this more, I do think that it makes more sense to make this an argument to init to reset. @RedTachyon ? |
I'm fine with it being an argument to init/reset, but we really need to put some more thought into what One question would be how to actually make it happen in environments. Will we require every environment to copy-paste a similar boilerplate to handle that logic? Or should we connect some of these things in the base Env class, and have people call And one more doubt - I'm very much not a fan of using a string as an argument for that, can we just make it into an enum? This would make it a tad more annoying to use (i.e. you have to import the enum), but so much more readable and understandable, if the intention is to support only these two (or any other predefined) rendering modes. Perhaps two separate settings? One boolean for "render_human" and one for "render_rgb"? (maybe with better names) |
I thought about this more. I'd like to make render an argument to init, that accepts strings. When an environment steps in rendered mode, it does literally whatever it wants to, which can handle the huge diversity of things. The environment can then have .collect_render() called to have it return whatever it wants to return if it should return things. The only boiler plate code that I'm aware of is for caching and returning either stacks of numpy arrays, gifs, or videos. The way to get around boiler plate code would be to provide a util where, if the environment gives it the arrays it does everything else. This would basically be identical to a compatibility wrapper for the existing render function. |
@RedTachyon @JesseFarebro does that work for you guys? |
This method would allow the environments to still to popups or literally whatever else they could want to, it entirely hands it off to the environment, which I think is the way. |
Why would the init argument take strings? Afaik the rendering modes that really appeared anywhere are:
The "return X" options will anyways require a Another question is - what if the environment needs some additional setup to do RGB array rendering? Should this setup only be performed if we set the init parameter? Overall I'd definitely focus on having more convenience/simplicity for this use case as I'm guessing it's more frequently used (i.e. that's the method you use for actually recording stuff) |
We discussed this on call, but in essence the proposal is for the init argument to cause literally whatever the environment wants, with a few generally standard strings, and a standard method for collecting returns from render. This is about as honest to the heterogeneity of these environments as possible |
Earlier today I made some proposals for modifying the new rendering API to make it more flexible on #2671. I'm re-posting them here so that we can keep the discussion on #2671 focused on @younik's specific PR. Here's the basic proposal:
It was also suggested by @younik that with this approach, it may not even be necessary to include the
If we want to support rendering in these environments then it might be necessary to keep the Thanks in advance for considering my input and I'm very willing to contribute code to making sure fine-grained rendering is possible in Gym 1.0. |
Closing in favor of #2671 |
As mentioned in #2524, the rendering API is the last breaking change I think should be made.
Right now, the rendering API has a few problems problems:
render()
after each step does allow you to extract a smooth videoWhat I think would be a solution to all these problems that would also be more user friendly is the following:
render
an argument toinit
, defaulting torender=None
, where people can specify if they want to render, and if so how, at initialization. This could also be put in reset instead of init, I'm not certain which makes more sense..collect()
, though I'm open to better names for the function.We could also create a Gym wrapper, say
gym.wrappers.legacy_render
that makes calls to existing.render()
methods and supports common render arguments (presumablylive
,video
andframes
), such that there old environments wouldn't even have to change their code to still be useable.The text was updated successfully, but these errors were encountered: