Closed aaronsnoswell closed 5 years ago
Hi @aaronsnoswell , thanks a lot for pointing out this problem and for this PR ! It looks great, I just have two tiny discussions:
I think it's better to put render option to render()
function, say in kwargs, instead of __init__
, in this case, probably it's not necessary to have docstring for constructor. (in gym.Env
class they already have docstring for the explanation of APIs)
Perhaps it can be a good idea to create a OpenCV-based class, something like OpenCVImageViewer
and call this class within the option in the same style of SimpleImageViewer
. This would make the code clean and consistent.
What do you think ?
Hi @zuoxingdong, your suggestions sound good - I'll make both changes and re-submit in the next day or so for re-reivew :)
Ok, please see the updated changes - I've broken out a separate OpenCVImageViewer
class now, and the option argument is in the render()
function. I couldn't put it in the kwargs
because that gets passed to the physics render() method :)
Hi @zuoxingdong :) I believe the code was already PEP8 compliant, but I removed the extra whitespace to match the existing coding style. Let me know if there is anything else. Thanks!
LGTM, thank you very much @aaronsnoswell for this PR ! Let's merge it now.
This PR adds a workaround to fix #1 by creating an alternate human render window option, based on the python-opencv library.
I've tried to make minimal changes, but I did add some spacing and comments in the render function and constructor to clarify what the new code does.
Looking forward to your comments and/or merging this PR :)