-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Render API #2671
Render API #2671
Conversation
# Conflicts: # gym/envs/toy_text/frozen_lake.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks generally good, the only thing this is missing is a test that runs all of the environment with all of these rendering modes. Could you add this?
@pseudo-rnd-thoughts thanks for the comments; I added the tests for rendering |
# Conflicts: # gym/utils/env_checker.py
I can't run the example given in comment, and I'm sure my gym version is the latest (0.24.1), the error message is |
The new render API is not in the release version yet, if you want to use it, you should install gym from source |
That makes sense. Looking forward to the formal released version. |
Why did the authors decide to change the render API -- the removal of argument In addition, there seems many bugs around: As discussed in the previous comments and #2825, I think despite the new default/API, we could maintain the backward compatibility. Could you please reconsider adding it back unless the breaking change and removal of the argument is really necessary? |
Hello, for a detailed explanation of why this render API was introduced see #2540
The constructor is properly implemented in source; as I answered in the issue, we mistakenly merge the update documentation before the release, sorry about that. Also, #2889 is the expected behaviour, I will answer your issue soon. |
If the maintainers intended such a breaking change, this should be done in v1.0 release or after some enough deprecation/announcement period. I feel release of this new API went a bit hasty without enough discussion. I would argue that we should keep the backward compatibility as much as possible. As a minority opinion, with all due respect I don't think the new API is necessarily better --- I understand the purpose and intention discussed since #2540 but they could've been implemented in a different way without hurting the sensible interface design and backward compatibility. All that said, I do understand that in some cases breaking changes and radical changes are necessary. However, this should be done rather carefully with all other relevant changes together. For example, it is said (#2891) |
Even if it had already been merged, I would be sincerely happy to implement a better proposal that will address the problems explained in #2540
What do you mean? Legacy codes still work (with just a warning), the current changes are backward compatible |
The current behavior and API is different from older versions in the following sense. I would appreciate if you can explain why you would think this is backward compatible. The changes are "on purpose", so in my view it is obvious that we broke backward compatibility:
This change is perhaps OK, because it throws error rather than behaving wrongly. Users can straightforwardly switch from positional arguments to (forced) keyword arguments (at the acceptable cost of code migration). Or we could improve the wrapper to pass The real problem is when
The additional parameters to renderer seems buggy at this point (#2891), so I can't tell right now whether it will behave the same (i.e. overriding the rendering options specified in the constructor when provided). But I can guess those keyword arguments won't have any effects, especially when no errors are thrown and keyword arguments are silently ignored: currently
so those arguments to As I mentioned in another thread (#2889), we should have a different rendering mode for the multiple-frames-since-the-last-call-for-frame-skips behavior other than |
I looked at the code and figured out what @younik meant by backward compatibility. >>> env = gym.make("HalfCheetah-v4")
>>> env.reset();
>>> env.render(mode='rgb_array', width=300, height=300).shape
(480, 480, 3) The only change is that the env wrapper requires keyword argument (no positional arguments, i.e., However, I think |
@wookayin sorry, I was answering you, but your message make me realize that the mode as argument was broken and I worked for the fix (#2893) So, all old-style code work as before.
In general, I think we want to encourage the use of |
@younik @pseudo-rnd-thoughts I still think the new API introduced here doesn't make much sense. For the sake of consistency, this should have been renamed as something like
UPDATE: This already has been addressed, in #3040 (unreleased yet). Per the release note v0.26.0 (#3056),
I'm supportive of this change. For a record/historic purpose, let me put a pointer here for those who might find this issue in the future. |
New render API
Following this discussion: #2540
This PR extends the
render()
method, allowing the user to specifyrender_mode
during the instantiation of the environment. The default value ofrender_mode
is None; in that case, the user can call render with the preferred mode as before. In this way, these changes are backwards compatible.In case
render_mode != None
, the argument mode of.render()
is ignored.In case
render_mode = "human"
, the rendering is handled by the environment without needing to call.render()
.With other render modes,
.render()
returns the results as before. We also introduce_list
mode that returns aList
with all the renders since the last.reset()
or.render()
. For example, withrender_mode = "rgb_array_list"
,.render()
returns aList
ofnp.ndarray
, while withrender_mode = "ansi"
aList[str]
.TODO
Examples
Example of backward compatibility: