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

BizHawkClient: Add BizHawkClient #1978

Merged
merged 29 commits into from
Oct 3, 2023
Merged

Conversation

Zunawe
Copy link
Collaborator

@Zunawe Zunawe commented Jul 15, 2023

What is this fixing or adding?

Adds a generic client that can communicate with BizHawk. Similar to SNIClient, but for arbitrary systems and doesn't have an intermediary application like SNI. An example implementation of a client for Pokemon Emerald can be found here. It has a block of code at the top related to importing the client as an apworld, but that's not necessary during development. See the Discord thread for more info.

From what I can tell, this is at least as stable as (and often more stable than) any existing BizHawk client/connector when it comes to setup. Switching ROMs, pausing emulation, starting the script before loading a ROM, and closing and reopening the script or client can cause problems in existing scripts, but recover here.

Merging this doesn't require any specific clients to migrate, but obviously there may be some initial confusion among players about which games can and can't use this.

How was this tested?

Integrating Pokemon Emerald and running a couple beta tests. Emerald doesn't need every function, and is the only integrated client on the test branch, so some behaviors were only tested in development.

No currently merged games have been migrated and tested, but I paid attention to their existing Lua scripts to try to cover necessary functionality.

@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jul 19, 2023
@beauxq
Copy link
Collaborator

beauxq commented Aug 24, 2023

I would prefer to see a python bizhawk communication library that is separate from any of the AP client application stuff.
It would have nothing from CommonClient, nothing from kivy, etc. - just the communication with bizhawk and nothing else.
I imagine code like this:

system = await bizhawk.get_system()
await bizhawk.set_message_interval(2)
result = await bizhawk.read(read_list)

So this could be used with any python client, and a BizHawkClient could be made separately to use this.

It seems like being forced into BizHawkClient would make it more difficult/messy to support non-bizhawk systems,
whereas a bizhawk communication library could more easily be mixed with non-bizhawk systems.


class BizHawkClient(abc.ABC, metaclass=AutoBizHawkClientRegister):
system: ClassVar[str]
"""The system that the game this client is for runs on"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pokémon Red and Blue can be run on two different systems: GB and SGB. I need to be able to register my game with either of those two systems being reported

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Edge cases all the way down. I can make it a Union[str, List[str]] and just add duplicate references I suppose. And you can detect the system through the async BizHawk functions if you need to. I'll take a closer look tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that will affect all Gameboy games, and is likely a situation unique to Gameboy games, so perhaps just looking for GB system client handlers when SGB is reported would be a good solution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about, like, GBA in NDS, or someone going crazy and implementing a client that works for one game ported to two systems like Twilight Princess (but something on BizHawk supported systems).

Changed in d706c3f. You can now specify either a str or a tuple[str, ...]. So in this case ("GB", "SGB"). I'd rather have it explicit for all multi-system games instead of implicitly allowing certain systems to mix just in case there's ever a difference.

@Alchav
Copy link
Contributor

Alchav commented Sep 3, 2023

In my attempt to get started on adding Pokémon Red and Blue to this, I ran into this when calling read in validate_rom to check the rom name:

  File "C:\src\Archipelago\worlds\_bizhawk\__init__.py", line 278, in read
    return await guarded_read(ctx, read_list, [])
  File "C:\src\Archipelago\worlds\_bizhawk\__init__.py", line 243, in guarded_read
    res = await send_requests(ctx, [{
  File "C:\src\Archipelago\worlds\_bizhawk\__init__.py", line 108, in send_requests
    if ctx.streams is None:
AttributeError: 'BizHawkClientContext' object has no attribute 'streams'

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 3, 2023

This was a recent change related to beauxq's comment that had discussion in Discord.

read doesn't take an instance of BizHawkClientContext, it takes an instance of BizHawkContext. So you should be doing bizhawk.read(ctx.bizhawk_ctx, [])


To more fully explain the changes made, since there's no record of the discussion here except the commits,

The functions related to talking directly to the connector script were split into worlds/_bizhawk/__init__.py. Those are the functions like bizhawk_read, which became read, bizhawk_set_message_interval became set_message_interval, etc... The client superclass and metaclass are in worlds/_bizhawk/client.py. And the context and code required to run the client are in worlds/_bizhawk/context.py.

So it should now be possible to import just the functions to talk to BizHawk. I'll update the link in the original message for the example client just after I post this.

@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 3, 2023

Considering even I managed to forget to use ctx.bizhawk_ctx instead of just ctx, I'd be open to suggestions for clearer names.

@Berserker66 Berserker66 merged commit bc11c9d into ArchipelagoMW:main Oct 3, 2023
12 checks passed
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Adds a generic client that can communicate with BizHawk. Similar to SNIClient, but for arbitrary systems and doesn't have an intermediary application like SNI.
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Adds a generic client that can communicate with BizHawk. Similar to SNIClient, but for arbitrary systems and doesn't have an intermediary application like SNI.
@Zunawe Zunawe deleted the bh-client branch May 23, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants