-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: adding info - is_success #208
Conversation
For now I'll update the code to set I will open another issue to update the examples such that they return the correct value, once this and the linked MRs are merged. |
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.
Hey, thanks for adding this feature! I haven't reviewed it in-depth yet, so this is just some initial feedback.
I will open another issue to update the examples such that they return the correct value, once this and the linked MRs are merged.
I wonder if it might be better to keep this backwards compatible, such that it doesn't break existing environments, this makes it easier for people to upgrade (or use older plugin versions with the current GDRL). While not all updates are backward compatible, when it's easy to do it can be an advantage.
At some future point we can remove this backward compatibility. Since you already implemented the backward compatibility, I would personally recommend it to stay for now.
Co-authored-by: Ivan-267 <[email protected]>
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.
I think this one is fine - but before we merge this, I'd like to check the plugin and make some suggestions there.
I'll push the docs to this PR as it's connected to the plugin PR, no need to start a new one. |
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 one should be ready - pending review by @edbeeching
Again, thanks very much for implementing this.
It wasn’t a big deal. Thanks for doing the tutorial and being so tedious 🙏 |
Removes the unnecessary resetting of is_success to false (the auto reset could be delayed based on action_repeat, so it's better to specify true or false every time when setting done = true to make sure it's up to date).
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.
Thanks, LGTM
Adding the ability to pass game info to the Python code, specifically if a run
is_success
. This gives more info to the developer when a simulation ends and they want to know the outcome.This MR links to:
And is blocked by: