FF1: Bizhawk Client and APWorld Support

[Rosalie-A]

Please format your title with what portion of the project this pull request is
targeting and what it's changing.

ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"

What is this fixing or adding?

Converts FF1 to using the Bizhawk Client instead of its bespoke client and connector, with updated documentation.

Also makes the few changes needed to allow it to be an APWorld instead of a folder in lib/worlds, mostly for my convenience.

How was this tested?

Ran through an entire game, checked every location to make sure it sent. Received items, both naturally and through admin console, and confirmed they arrived properly. Also put this up for testing in the Discord, and no issues have been reported as of the latest version (whether this means no testers or no issues is, as always, up in the air).

If this makes graphical changes, please attach screenshots.

[qwint]

pure code review, did not run the client at all nor play any games, also only briefly reviewed docs changes and full Client.py code

You will also need to update inno_setup.iss to remove the old frozen executable (and likely the old folder since you're moving to .apworld format too) but other than and my pkgutil comments looks good

[qwint]

Suggested change

	file = pkgutil.get_data(__name__, os.path.join("data", "items.json")).decode("utf-8")
	file = pkgutil.get_data(__name__, "data/items.json").decode("utf-8")

ref #4232
for this and the other similar suggestion (also there's no need for the os import after this change)

[qwint]

Suggested change

	file = pkgutil.get_data(__name__, os.path.join("data", "locations.json"))
	file = pkgutil.get_data(__name__, "data/locations.json")

[qwint]

it doesn't seem like bizhawk client allows for easily changing GameManager.logging_pairs before the ui is built, which would be required for this to display in the client ui,
they should still all log to debug/terminal and to log file, so if that's the intention this should be fine,
but I wanted to clarify this point

[qwint]

not certain there aren't more required inno_setup changes, but the updates fixed every issue that existed to my knowledge

[beauxq]

Rosalie-A#72

[Zunawe]

Without looking too hard at the exact behavior, this function looks suspicious.

Your game watcher runs over the course of many frames. status_a, status_b, and status_c necessarily represent values from 3 distinct frames, in order, but not necessarily adjacent. Between the retrieval of status_c and the time this function returns, the player may have spent 200 frames or more doing whatever they want. And every read and write you do after this function returns will also be on new frames which happen an arbitrary amount of time in the future. This is why it's tricky to convert one of the old connector scripts; those scripts have the luxury of knowing that everything they do will happen on the same frame.

What you probably want is to find a way to use guarded_read and guarded_write while checking these values. Because your information is outdated before your python code knows what it is. The guards give you a way to ask the Lua script to abort what it was about to do if it sees something out of place.

[wildham0]

so 0xF2 is the value used by bizhawk to initialize sram, the game overwrites it right at boot, so the 3 0xF2 checks are only necessary to see if a rom is loaded
A is never 0x00 during gameplay (ie past the menu screen) (it's the first letter of the first character's name)
so while the values might be slightly out of syncs for a few frames, it shouldn't really matter

[Zunawe]

I see.

There's still a possibility that someone resets their game after this function is called but before the game watcher has returned, right? The unintended side effects can be a headache to identify if you don't lock down your canaries (ask me how I know). A couple users may report that some random locations sent while they were playing, or someone didn't receive an item but also they have 200 of an unrelated item.

Also, despite the name of the client, I'd recommend being careful about making BizHawk-specific assumptions. Maybe someone finds another NES emulator that supports scripting or plugins and hooks it up to the interface BizHawk Client uses. If you can generalize and future proof against that, it might save you in the future.

[wildham0]

you're right it's still better to put some guarding measures in

as for bizhawk specific values, unfortunately i don't think there's any memory space that's safe to check right now, so any emulator agnostic solution would probably require the upstream rando to reserve a few bytes for that specific purpose

[Rosalie-A]

so there's a few problems at play here.

1. Bizhawk Client doesn't have a guarded read/write for this situation that I can see. IOW, I can only check a byte against an allowed value, not against a list of allowed values or against a list of forbidden values. So for the check that the game has been loaded -- that the first character of the first character's name has been set to a valid value -- I can't do a guarded read/write against without doing a lock() operation for each read and write. Which if that's the recommendation in this case, sure, but it doesn't feel right.

1.5) There's not actually any better value to check for if the game has started (i.e. is not at the title screen). There's no game mode or state variable that gets set so as far as I can tell the name character check is truly the most accurate way to determine if we're ingame.

2. All that said, I can't actually see a bad situation arising due to a mismatch between game state and expected game state. Going to the title screen clears out all of the working area of SRAM. For our purposes, this is "no locations checked, no items received". So, the way I see things, there are four possible situations:

• We're at the title screen, and the game watcher thinks we're at the title screen. No issues here: we don't send anything.
• We're ingame, and the game watcher thinks we're ingame. Again, no issues here: this is normal operation.
• We're ingame, and the game watcher thinks we're at the title screen. We don't send or receive anything. Since the watcher only runs a few times a second, this period of "nothing happening" is invisible to the player. No problem here either.
• Finally, we're at the title screen, and the game watcher thinks we're ingame. This is the only "problem" situation in that a) we'll be sending out checks that we don't have in the current data...except to have flagged those checks we need to have accessed them in the first place, so there's no discrepancy here. Or b) we try to receive items that we've already received and add them to the various inventories. This is invisible to the player and all of this will be overwritten as soon as "continue" is pressed, resuming normal operation.

In other words, there exists basically* no point in the execution of the game where the work RAM we care about is not either zeroed out or at a valid value. (*: "basically" just because of that fraction of a frame at cold startup before the WRAM section is zeroed out, but this is far shorter a time than it takes for the connector script to connect and verify the game)

In summary: there's no performant way to perform a guarded read/write with the values FF1 provides and the current state of the Bizhawk Client as I see it, and there's no way I can see that we can have an invalid situation in the first place.

[Zunawe]

You're right, you can't do checks except against a specific value, but you could (at least for character name) read that value and then use that as the guard. If it's zero, skip out early, if it's nonzero, save it and use it as a guard.

If there's no state where you might accidentally read garbage or write in such a way that the player loses items, I think that's probably fine.

[Rosalie-A]

"read the character's name and use that as the guard" is so exceedingly obvious I feel silly for not thinking of it. I'm sure that there's no way to get into a bad state, but better safe than sorry. I'll get that implemented.

[wildham0]

so while it's guaranteed to be non-zero, when the player switch his party around the name will change (ie if you put the 3rd character in 1st position, then the name at that address will be changed to the 3rd character)
the name isn't destroyed tho so you could check all 4 characters and see if one name match (0x102, 0x142, 0x182, 0x1C2)

[beauxq]

Since this is changing how it gets built for the frozen installer, it would be good to make sure this gets tests with the frozen version.

[Rosalie-A]

Seemed to work fine. I built with setup.py, generated with the built Generate executable, connected with the built Bizhawk executable, and was able to send and receive items. In other words, it all seems to have worked. If there's any more testing to be done on the frozen build I'm not aware of, I'm all ears.

[qwint]

one thing to double check is that the inno setup changes correctly deleted your root exe, and to see if you need to (i'm almost certain you do) add a deletion line for "lib/worlds/Final Fantasy" to make sure you were running the apworld version and not the installed-previously folder version

[Rosalie-A]

FF1 was already in installdelete.iss like the rest of the worlds so I didn't need to add anything there, and I can confirm that running the proper installer does delete the old client and old world folder (while adding the new APWorld) as desired and expected.