Skip to content
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

Add easy way to add plugins (game wrappers) to Plugin Manager #230

Open
hexiro opened this issue Sep 7, 2022 · 11 comments
Open

Add easy way to add plugins (game wrappers) to Plugin Manager #230

hexiro opened this issue Sep 7, 2022 · 11 comments

Comments

@hexiro
Copy link

hexiro commented Sep 7, 2022

Hello! I've been attempted to add the WIP Pokemon Red Plugin to my local project, but I'm finding it's quite difficult due to the way the PyBoy and PluginManager classes are structured. Would you be willing to accept a PR that makes the design more modular, and allows the user to something like the following?

game = pyboy.PyBoy(PATH, game_wrapper=True)
game.plugin_manager.add_plugin(PLUGIN)

Additionally, inside the plugin manager, there is a lot of .enabled() calls from plugins, but it appears as though that function will return the same result everytime. This can be seen by the fact that the PluginManager class has extra attributes to cache the result for all of them.

# example:
self.window_sdl2 = WindowSDL2(pyboy, mb, pyboy_argv)
self.window_sdl2_enabled = self.window_sdl2.enabled()
self.window_open_gl = WindowOpenGL(pyboy, mb, pyboy_argv)
self.window_open_gl_enabled = self.window_open_gl.enabled()

I personally think this would be structured better if the enabled function in each plugin was changed into a _enabled helper function that is called in the initializer and set to an enabled attribute, so the attribute .enabled would be available and these extra (plugin name)_enabled attributes wouldn't be needed.

If there's conflicts with evaluating enabled state before hand, that's okay as that doesn't impact the user at the end of the day. What does though, is the ability to add custom plugins, so let me know what you think about these suggestions! :)

@hexiro hexiro changed the title Easy way to add plugins (game wrappers) to Plugin Manager Add easy way to add plugins (game wrappers) to Plugin Manager Sep 7, 2022
@Baekalfen
Copy link
Owner

Would you be willing to accept a PR that makes the design more modular

Absolutely! But the problem is, that Cython is very strict on what you can do in runtime vs. compile time. It is definitely possible to make all of this more dynamic, but I initially postponed doing it, because it requires us to dynamically load external compiled modules.

Additionally, inside the plugin manager, there is a lot of .enabled()

This is related to Cython. Again, it's difficult to do tricks in runtime while keeping the Cython performance. But it would be awesome to see it become more dynamic without losing significant performance (slight performance hit is acceptable).

Have a look at manager_gen.py. That's the code that generates all the _enabled = ... code.

@Baekalfen
Copy link
Owner

Long-term goal would be to install plugins from other PyPi packages -- say pip install pyboy-pokered-wrapper -- and have it available. Inspiration could be found in pytest on this.

@hexiro
Copy link
Author

hexiro commented Sep 8, 2022

Do you mean installing the package makes it automatically available, or you have to add it? Regardless, I think this could be a step in the right direction for the time being. If I made these changes would you accept the PR?

@Baekalfen
Copy link
Owner

If you can refactor the plugin code to dynamically locate and load plugins at run-time while using Cython -- while not sacrificing any meaningful amount performance -- then I'll definitely accept the PR.

You can start by dynamically loading the plugins in the pyboy/plugins/ directory -- likely by deleting manager_gen.py and modifying manager.py. We can wait until a second iteration to add plugins from third-parties.

And in a third iteration, we can possibly look at locating plugins installed from PyPi packages. As inspiration, in pytest, you'll see it listing third-party plugins that it found (notice xdist-2.5.0 installed with pip install pytest-xdist):

$ pytest code
===================================================================== test session starts ======================================================================
platform darwin -- Python 3.10.6, pytest-7.1.3, pluggy-1.0.0
rootdir: /Users/mads
plugins: xdist-2.5.0, forked-1.4.0
collected 0 items

@hexiro
Copy link
Author

hexiro commented Sep 8, 2022

If you can refactor the plugin code to dynamically locate and load plugins at run-time while using Cython -- while not sacrificing any meaningful amount performance -- then I'll definitely accept the PR.

You can start by dynamically loading the plugins in the pyboy/plugins/ directory -- likely by deleting manager_gen.py and modifying manager.py. We can wait until a second iteration to add plugins from third-parties.

And in a third iteration, we can possibly look at locating plugins installed from PyPi packages. As inspiration, in pytest, you'll see it listing third-party plugins that it found (notice xdist-2.5.0 installed with pip install pytest-xdist):

$ pytest code
===================================================================== test session starts ======================================================================
platform darwin -- Python 3.10.6, pytest-7.1.3, pluggy-1.0.0
rootdir: /Users/mads
plugins: xdist-2.5.0, forked-1.4.0
collected 0 items

sounds good

@hexiro
Copy link
Author

hexiro commented Sep 8, 2022

Hello, what is PYTEST_SECRETS_KEY meant to be set to? Thanks

@Baekalfen
Copy link
Owner

It's to access ROMs used for tests, but only I have the key as to not distribute the ROMs illegally

@Baekalfen
Copy link
Owner

You should be able to run the other tests without problems

@hexiro
Copy link
Author

hexiro commented Sep 8, 2022

You should be able to run the other tests without problems

is there a way I can make it skip the tests that require it? a lot of tests are failing with that being the error.

@Baekalfen
Copy link
Owner

Check out the newest commit on master

@hexiro
Copy link
Author

hexiro commented Sep 9, 2022

cool 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants