bzier / gym-mupen64plus

An OpenAI Gym environment wrapper for the Mupen64Plus N64 emulator
MIT License
91 stars 39 forks source link

Add support for Super Smash Bros #63

Closed bcoopers closed 6 years ago

bcoopers commented 6 years ago

I think pretty much everything is working at this point.

bzier commented 6 years ago

Awesome, thank you for this. I will most likely have time to review the changes this weekend. You can expect that I may have a few suggestions for you to tweak before I accept the pull request. Assuming there isn't anything significant, it may just be like naming/formatting preferences, comments, organization, etc. Or, who knows, maybe it'll be perfect as-is ;)

Thanks again for taking this on.

bzier commented 6 years ago

This is work towards #59

bcoopers commented 6 years ago

Cool sounds good to me.

bcoopers commented 6 years ago

@kevinhughes27 You can run health_parser.py as main() and then it will run on all the screenshots and report the results,

bzier commented 6 years ago

@bcoopers Changes are looking great, thanks for the quick turnaround. I'll pull them down and verify soon.

bcoopers commented 6 years ago

Alright I think I've addressed all the comments, let me know if I missed something, or if you see anything else.

bzier commented 6 years ago

@bcoopers Nice, I think you covered everything. I pointed out one more new concern, but I'm about ready to call this good and merge it in. This was some awesome work.

I guess there's one other thing I should mention. For a long time, I've been running this environment in Docker containers. I have been putting off fully documenting that and so haven't merged it into the main branch yet, but you can see the current-ish version on the dockerize branch. I would like to test this out with my Docker container quickly before wrapping this up. There's one more tweak I can think of that may be necessary, but let me try it out and I'll let you know.

EDIT: I might as well just have you do it either way... There is a setup.py file in the top-level directory that includes the Python dependencies. Can you add opencv to the list, at the appropriate version that you are using? On my system that is opencv==3.1.0. If you're using a different version, feel free to specify that instead.

bzier commented 6 years ago

@bcoopers Wow, I just discovered that I can commit directly to your fork. Apparently you must have set the setting when you created the PR. If I had known that was even an option, I would have just made a bunch of the little changes myself. Sorry for making you go through and do them. But hey, I learned something new today.

bcoopers commented 6 years ago

Fixed the frame_skip + 1 issue, and did the setup.py change you requested (my OpenCV version was 3.2.0 fyi)

bzier commented 6 years ago

@bcoopers Sorry I've been slow to get back to you on this. When I tried the opencv=3.2.0 package pip wasn't able to find it. However, I was able to get it working with opencv-python==3.4.1.15 and that doesn't seem to require having the system package python-opencv installed as mentioned in your update to the README file. Is there any way you would be able to test and confirm if that works for you as well? I am not sure why there is a difference in the package that you have versus the one I was able to find, but I'd like to sort it out and make sure we have maximum compatibility before I merge things in. Here's the link to where I found the package.

bcoopers commented 6 years ago

Done, it works with 3.4.1.15 now.

bzier commented 6 years ago

Thank you. I appreciate all the work you put into this :)

bcoopers commented 6 years ago

Thanks for the review!