Add support for building a Virtual Console patch

[Rangi42]

@mid-kid's https://github.com/mid-kid/pokecrystal/commit/b8b567d0492d73084a5a9f2ecdf0d0556fcb02f8 adds support for a Virtual Console patch. @yenatch rejected it in PR #484 in 2018, saying: "It's interesting and probably useful. It's a lot of bloat for a niche feature, so I don't think it should be merged either. I get that it becomes even less useful by not being part of everyone's repo even if they aren't aware it's there. I guess it's not too much work to remove, you'd just kill all the .VC labels and _CRYSTALVC ifdefs. I'm still undecided."

By now both members are no longer around pret, and pokecrystal's stance on "bloat" has tacitly changed with its support for building the Australian and debug ROMs. I think the VC patch build would now be worth adding: it's mostly just labels, and those can be conditionally defined so they won't clutter non-VC sym files.

The previous branch would need adapting for current pokecrystal: hJoyPressed is an address, not an EQU constant. It also has commented: "Of note is that the preprocessor is very stupid, and won't check if the offsets in the original ROM differ with those in the patched ROM. It also won't tell you if you didn't catch all of the differences between the ROMs." These flaws should be fixed. It should also add an assert to make sure the save file byte that gets written to unlock the Celebi event is at the right address.

If this is done, it should be done for pokegold, pokered, and pokeyellow as well. All the official patches are available here: https://gbatemp.net/threads/release-pokemon-gold-silver-and-crystal-virtual-console-wireless-linking-patches.439986/page-32#post-9097514

[Rangi42]

One way to conditionally define such labels:

vc_hook: MACRO
    if DEF(_CRYSTALVC)
        .VC_\1::
    endc
ENDM

AddHallOfFameEntry:
    ld a, BANK(sHallOfFame)
    call GetSRAMBank
    ld hl, sHallOfFame + HOF_LENGTH * (NUM_HOF_TEAMS - 1) - 1
    ld de, sHallOfFame + HOF_LENGTH * NUM_HOF_TEAMS - 1
    ld bc, HOF_LENGTH * (NUM_HOF_TEAMS - 1)
.loop
    ld a, [hld]
    ld [de], a
    dec de
    dec bc
    ld a, c
    or b
    jr nz, .loop
    ld hl, wHallOfFamePokemonList
    ld de, sHallOfFame
    ld bc, wHallOfFamePokemonListEnd - wHallOfFamePokemonList + 1
    call CopyBytes
    call CloseSRAM
; Causes the Virtual Console to set [sMobileEventIndex] and [sMobileEventIndexBackup] to MOBILE_EVENT_OBJECT_GS_BALL
    vc_hook BiographySave_ret
    ret

[Rangi42]

Filling in the template values for FISSURE and SELFDESTRUCT would be easier if rgbasm could write out constant values to a file the way it does with .sym/.map (see https://github.com/gbdev/rgbds/issues/606), instead of having to reimplement the const_def/const/const_skip logic (and rely on the .asm being consistently formatted since it doesn't do real parsing).

[mid-kid]

a workaround for now could be to make an object file that EXPORTs the required values (or similarly EXPORT them in the .sym file). This could be done near where the hooks are placed, for consistency.

[mid-kid]

Also I'm sure the emulator could be patched to change the offset of sMobileEventIndex, but the current setup would've only generated the .patch file...

[Rangi42]

Exported EQU constants are not included in the .sym or .map files. Edit: I see, you mean do SECTION "FISSURE", ROM0[FISSURE] / FISSURE:: etc. That could work.

If someone's patching the emulator, they'll also be able to remove a no-longer-valid assert.

[mid-kid]

I actually thought exported EQUs ended up in the sym file, but I guess that might either just be old behavior or I'm misremembering because ISAS does do it. Regardless, exported EQUs can be parsed off of the object files, I guess, or you can just output a text file with rgbasm by abusing strings and the small ROM option. Not exactly a clean solution but the only thing I could think of so fast.

[vulcandth]

This particular issue is very important to me and my romhack project i'm working on. A big premise of my romhack is to allow trading and battleing; and the preferred method for my family is friends would be using the 2DS/3DS consoles.

I would like to contribute to this effort in any way I can; Unfortunately I only started learning assembly two weeks ago, so I won't be much direct help there. I do have access to two nintendo 3ds that I can inject roms into the virtual console for testing. So once we get something testable, I have the time to do the testing and tweaking.

What I think I know: From what I understand, the goal is to generate a .patch file (based on the original VC crystal.patch file as a template) and changing the address values to point to specific locations in the code, referencing constants or something else outputting to a .sym file or something simlilar?

I know both of your time is limited, and you already spend so much time work on other issues. This entire disassembly project is amazing as it is. It is appreciated.

Perhaps you could set up a virtual-console branch in the meantime, to avoid interfering with the main branch while the support for building VC patches is being added. If you already have a vague idea of how you want things implemented; If you could give a few examples.. I could probably at least go through mid-kid's code as a reference and place stuff where they should probably go. I'd rather not touch the MakeFile myself.

Also random other question; I'm not sure how the addresses are supposed to relate from the VC patches. For example in the original Crystal USA patch file there is a section with the following info: [send_byt2] Mode = 2 Address = 0x100A14 Type = 31

Is the Address = 0x100A14 supposed to relate to an address in the format 0b:6a70 (random example) like in the .sym file? I feel like this is either super simple, super complex, or i'm just flat wrong in my assumption. If they do relate, how do they relate, like how do you transcribe one into another. (Don't feel obligated to teach me lol.)

[mid-kid]

From what I understand, the goal is to generate a .patch file (based on the original VC crystal.patch file as a template) and changing the address values to point to specific locations in the code, referencing constants or something else outputting to a .sym file or something simlilar?

Yes. Not just the sym file, but other constants like move IDs are used in the patch as well. Basically it should be a method for the patch to be automatically adjusted to the modifications people make to the disassembly.

Perhaps you could set up a virtual-console branch in the meantime, to avoid interfering with the main branch while the support for building VC patches is being added.

We generally use user branches/PRs for this, nothing on the mainline repo. This would be weird to do if none of us are actually working on it. My old branch is here: https://github.com/mid-kid/pokecrystal/tree/virtual-console

I do have access to two nintendo 3ds that I can inject roms into the virtual console for testing. [ ... ] I'd rather not touch the MakeFile myself.

I'd heavily recommend learning how to use the generic homebrew tools for extraction/creation of a .cia file, rather than relying on Super Ultimate Injector 3000:tm:. This not only gives you a better idea of how the .cia file is structured, but also allows you to automate it with a script, be it a makefile or not. It's also less likely to cause problems than whatever hackery automated tools apply.

I wrote a makefile snippet that downloads the necessary decryption key from the nintendo servers, decrypts and unpacks the cia (Assuming it's a 100% clean, encrypted CIA. Not sure if those are what's being shared in the wild), copies the patch and additional files (icon and banner, though no tool is used to generate them from png files yet) into position and rebuilds it, all automatically, which would make it easy to do builds of any romhack, once the patch generation is in place: https://github.com/mid-kid/pokecrystal-cia/blob/master/cia.mk

You're free to write your own scripts to do this in whatever language you wish, though the basics of makefile are very simple, and I heavily recommend getting to know it.

...that is, if you wish to work on this feature and get it upstream, otherwise feel free to ignore these preceding three paragraphs.

Is the Address = 0x100A14 supposed to relate to an address in the format 0b:6a70 (random example) like in the .sym file?

It's an absolute offset into the ROM binary. See https://github.com/mid-kid/pokecrystal/blob/virtual-console/tools/make_patch.c#L151 for the calculation based on a XX:XXXX entry in the .sym file. 0b:6a70 for example would translate to 0x0b * 0x4000 + (0x6a70 - 0x4000) which is 0x2ea70.

If you need any help regarding the creation of patches, injecting to the cia or testing, feel free to reach out through our Discord. I'd love to help out, even though I don't have a lot time to work on this myself.

[vulcandth]

Thanks @mid-kid ! I'm glad I was at least on the right track with a couple things. You gave me a lot of good stuff to read up on.

[vulcandth]

I forked the pret/pokecrystal repo and created a new virtual-console branch. I merged @mid-kid changes as best I could into the latest pokecrystal. I have yet to find time to actually attempt make a CIA and test on DS. Want to work out a few things first. I'm willing to give access to my forked repo for collaboration if anyone wants to help. However, the big plus is make works and builds the roms and .patch files... mostly. The roms seem to work, the patch file is questionable and yet untested.

As expected from earlier mention in this issue, hJoyPressed needs to be fixed. tools/make_patch .VC_ pokecrystalvc.sym pokecrystalvc.gbc pokecrystal11.gbc pokecrystalvc.patch.template > pokecrystalvc.patch Error: Could not find constant hJoyPressed in file hram.asm Error: Could not find constant hJoyPressed in file hram.asm Error: Could not find constant hJoyPressed in file hram.asm Warning: Not all differences in the ROM are defined in the patch

Also in order to get make to build... I had to comment out tools/stadium --base pokecrystalvc.gbc in the MakeFile and replace it with the older tools/sort_symfile.sh pokecrystal11.sym [Fixed this in latest commit; I was being sorta dumb.]

If anyone has time and would like to take a look at my mid-kid-virtual-console branch(I was trying to name it differently, but github decided I should honor @mid-kid instead); and let me know what I should change ect. The MakeFile should definitely be reviewed.

Tomorrow I'll look into the preprocessor logic, and see if I can make it more verbose about changes or lack there of. I'll also begin learning the whole CIA extract/build process.

The Branch: https://github.com/vulcandth/pokecrystal/tree/mid-kid-virtual-console

[vulcandth]

I built a CIA using @mid-kid's cia make files; and tested it with this updated pokecrystal build + a few debug stuff I added to the player's home radio. Brought it over to my 3DS's... and to my surprise... it worked. Mostly. I was able to run the game, trade, and battle between the two 3DS'.

Unfortunately, I was only able to get about 3 turns in during a battle before the link communication was interrupted. However, getting it to work at all was an awesome feeling.

@mid-kid Good job on your old code!

[mid-kid]

I'm glad my (4 year old - how time flies) work resulted useful to someone! :)

I didn't really test how modifications to the ROM would affect the patch/linking, mostly because I didn't have more than one 3DS console. It's possible that some relevant data in the patch is still hardcoded, or that the emulator itself hardcodes some things (I'm pretty sure that it hardcodes the location of wMobileEventIndex for the celebi event, unfortunately. Maybe we can somehow patch the code.bin somehow to correct this).

And yeah, the errors in the patch generation tool are due to the format of the constants files having changed. It probably generates the wrong values if it can detect anything at all. I'm still not sure what the best way to go about extracting these values would be. rgbds doesn't really output this and the binary format of the .o files is anything but stable. I don't like parsing the assembly files from C but it might be the only solution.

[vulcandth]

@aaaaaa123456789 said in PR #882:

I'd say this suffers from the same problem as a few years ago. Namely, we don't really understand the format.

The repo builds a fully-modifiable ROM; we understand everything in it and we can successfully build modified versions at our leisure. This means that changes are not restricted at all by the existing codebase; it's entirely possible to replace complete subsystems, as many of us have done, and there are no constraints on what the replacements can do. (: there are some leftovers that we don't fully understand, like mobile code and leftovers for the N64 integrations, but those parts are nonfunctional and they can be easily removed.)

On the other hand, the VC patch isn't fully understood. There are some good guesses at what some declarations do, but the format hasn't been fully reversed; it is not currently in a state where we can write our own patch files from scratch. Therefore, the presence of a patch file at all constrains the capabilities of the codebase, since complete subsystem replacements that require writing a new patch file become impossible as long as that kind of compatibility is desired.

Therefore, a patch file effectively holds back (in part) the ability to modify the codebase while keeping existing components functional: a modified project that intends to replace a component that interacts with the patch file will have to keep parts of said component intact (or at least similar) for the patch file to work, without being fully aware of how this interaction works.

Due to this difference in quality, and until the patch file format is fully reversed to the point where it is fully understood and it can be rewritten from scratch, I'd recommend keeping the VC components in a separate repository, perhaps linked from the README here, so that a partially-reversed component doesn't hold back modifications of a fully-reversed and almost-fully-documented codebase.

Although you are correct in your assumption that not all of how the Nintendo Virtual Console interacts with the patch file is fully understood. I believe we know enough. One important note that the patch file isn't just "patching" the game. It also marks addresses the Virtual Console should watch for execution, when executed the virtual console performs a function. This is typically a emulator function, such as setting up the pseudo link cable communication. Basically it just needs to know when to start certain emulator functions. There are some areas of the code that get patched by the emulator during runtime. You can write your own runtime patches into the template file, modify the existing ones, or manually implement these features yourself. If someone downstream is making a ROM hack, and the move around some code, the vc_hooks and vc_patches are flexible enough to be executed or patch in the new address locations. With the only exception of GS Ball Event; but this can simply be implemented manually through an existing tutorial. Building the patch is done by make crystalvc. If you don't want to use this downstream, then don't. The base assembly code for pokemon crystal is not going to change on master... sure some stuff might be formatted differently RGBASM wise, but this is super easy to fix for the patch generation. If a downstream user completely rewrites a component, they may indeed need to determine the correct placements for certain vc_hook, or modify or delete a vc_patch.

Downstream support can't be guaranteed if you make major changes to the game, but that goes with anything. I don't see there being an issue with normal stuff changing on the master repo.

@aaaaaa123456789 said in PR #882:

I'd also point out that adding about a thousand lines of code to the main codebase (i.e., not patch-specific code, but the main code of the game) just for VC compatibility isn't a great idea, and I'd much rather avoid it.

Out of the 1739 new lines in the Pull Request; 828 lines are from tools/make_patch.c; and 737 lines are the pokecrystalvc.patch.template file. This leaves only 177 new lines to the main codebase.

This is only my opinion; and I am quite new here. So take it with a grain of salt.

[aaaaaa123456789]

One important note that the patch file isn't just "patching" the game. It also marks addresses the Virtual Console should watch for execution, when executed the virtual console performs a function. This is typically a emulator function, such as setting up the pseudo link cable communication.

And how much do we understand about those functions to use them for our own code? As far as I can see, for arbitrary code we add to the games (i.e., entirely new features, not modifying existing ones), the answer is "don't do it".

Building the patch is done by make crystalvc. If you don't want to use this downstream, then don't.

I'm not too happy by the way the repos build a million ROMs right now, but the changes are mostly non-intrusive. This is not true for the VC patch. "Just don't use it" means removing a lot of code from a million places, that isn't even properly marked by a conditional compilation constant, because the VC patch muddies up many code files with extraneous macros.

The base assembly code for pokemon crystal is not going to change on master...

Incorrect, as shown by the introduction of many new macros embedded in mainline functions.

...sure some stuff might be formatted differently RGBASM wise, but this is super easy to fix for the patch generation.

This is the definition of code changing.

If a downstream user completely rewrites a component, they may indeed need to determine the correct placements for certain vc_hook, or modify or delete a vc_patch.

Which implies (1) learning about a subsystem they probably didn't expect to have to deal with to build for a target they probably don't care much about, (2) understanding all of these macros that have very little (if anything) to do with the actual behavior of the code being rewritten, (3) reversing enough of the VC patch (which isn't even fully understood right now, let alone documented) to understand the purpose of the replaced hooks, and (4) rewriting part of said patch to support the new code. This is not a trivial task, and I can't understand why you treat it as such.

Downstream support can't be guaranteed if you make major changes to the game, but that goes with anything. I don't see there being an issue with normal stuff changing on the master repo.

Downstream support is guaranteed, in that components you don't modify still work. The VC patch is an intrusive modification of game components that isn't even testable by downstream users (unless you know of a debugging VC emulator).

Out of the 1739 new lines in the Pull Request; 828 lines are from tools/make_patch.c; and 737 lines are the pokecrystalvc.patch.template file. This leaves only 177 new lines to the main codebase.

I might have miscounted, but 177 lines of code are still a lot because they are scattered all across the codebase. It's not a 177-line file, but rather about a hundred changes in as many places.

[vulcandth]

And how much do we understand about those functions to use them for our own code? As far as I can see, for arbitrary code we add to the games (i.e., entirely new features, not modifying existing ones), the answer is "don't do it".

Who said you can't implement entirely new features and modify your code? I believe the only thing you could do to really break compatibility with the patching process is probably to significantly change or delete engine/link/link.asm, engine/link/mystery_gift.asm, or home/serial.asm. Although if you did that, (maybe to save room) then your not concerned about building patches to enable link functionality on the 3DS virtual console anyways.

I'm not too happy by the way the repos build a million ROMs right now, but the changes are mostly non-intrusive. This is not true for the VC patch. "Just don't use it" means removing a lot of code from a million places, that isn't even properly marked by a conditional compilation constant, because the VC patch muddies up many code files with extraneous macros.

The changes ARE marked by a conditional compilation constant. All the code is marked with if DEF(_CRYSTALVC).... If you don't use make crystalvc.. it won't build a patch... A million places is still only around 177 lines of code across 21 files. With the majority of them being in link.asm, mystery_gift.asm, and serial.asm... To fully purge the feature.. (just so you don't have look look at it I guess?). Is a matter of just deleting all of the vc_hook and vc_patch lines... and the corresponding if DEF(_CRYSTALVC) conditional the follows every vc_patch line. Then cleaning up the MakeFile.

.gitattributes (3 adds) .gitignore (1 add) MakeFile (16 adds) engine/battle/battle_transitions.asm (6 adds) engine/battle/core.asm (1 add) engine/battle_anims/anim_commands.asm (9 adds) engine/events/print_unown.asm (5 adds) engine/gfx/color.asm (1 add) engine/link/link.asm (44 adds) engine/link/mystery_gift.asm (31 adds) engine/menus/menu.asm (2 adds) engine/menus/save.asm (4 adds) engine/overworld/scripting.asm (1 add) engine/pokedex/pokedex.asm (1 add) engine/pokemon/mail_2.asm (5 adds) home/serial.asm (12 adds) macros.asm (1 add) mobile/mobile_40.asm (14 adds) rom.sha1 (1 add) tools/.gitignore (1 add) tooks/Makefile (4 adds)

The base assembly code for pokemon crystal is not going to change on master...

Incorrect, as shown by the introduction of many new macros embedded in mainline functions.

The addition of new macros in the pret/pokecrystal repository with not change the underlying assembly code of the ROM. If it did, the hash would change, and it would no longer be Pokemon Crystal. If something changed this, it would be a problem for the whole repo.

...sure some stuff might be formatted differently RGBASM wise, but this is super easy to fix for the patch generation.

This is the definition of code changing.

If the pret/pokecrystal master decides to change some formatting around, there could be the off chance that we will need to adjust something with the patch generation. Yes. But since the underlying assembly code is not changing, this is easy enough.

If a downstream user completely rewrites a component, they may indeed need to determine the correct placements for certain vc_hook, or modify or delete a vc_patch.

Which implies (1) learning about a subsystem they probably didn't expect to have to deal with to build for a target they probably don't care much about, (2) understanding all of these macros that have very little (if anything) to do with the actual behavior of the code being rewritten, (3) reversing enough of the VC patch (which isn't even fully understood right now, let alone documented) to understand the purpose of the replaced hooks, and (4) rewriting part of said patch to support the new code. This is not a trivial task, and I can't understand why you treat it as such.

If they don't care about building a patch... Don't build a patch. Don't worry about it. If you need to delete one of the few Conditional Compilation IF's.. then do so. If you WANT this feature, then yes you may need to learn about it a little if you are going to do massive changes to some of the key areas. I think many people would like to be able to make homebrew roms that work for trading and battling when played on the 3DS. This feature gives him the option to incorporate it into their project.

Downstream support can't be guaranteed if you make major changes to the game, but that goes with anything. I don't see there being an issue with normal stuff changing on the master repo.

Downstream support is guaranteed, in that components you don't modify still work. The VC patch is an intrusive modification of game components that isn't even testable by downstream users (unless you know of a debugging VC emulator).

Sure: https://citra-emu.org/wiki/home/

Out of the 1739 new lines in the Pull Request; 828 lines are from tools/make_patch.c; and 737 lines are the pokecrystalvc.patch.template file. This leaves only 177 new lines to the main codebase.

I might have miscounted, but 177 lines of code are still a lot because they are scattered all across the codebase. It's not a 177-line file, but rather about a hundred changes in as many places.

as many places being 21.

[mid-kid]

Notto dissu shitto agen.

Incorrect, as shown by the introduction of many new macros embedded in mainline functions.

An impressive count of two (2)

I'm not too happy by the way the repos build a million ROMs right now, but the changes are mostly non-intrusive. This is not true for the VC patch.

This is especially true for the VC patch, what are you talking about.

Which implies (1) learning about a subsystem they probably didn't expect to have to deal with to build for a target they probably don't care much about,

Literally not at all. A quick glance at the macro's descriptions (of which there are few) will reveal what they are for. They will have no effect and cause no build errors unless you're specifically building the vc-patched rom.

[mid-kid]

(3) reversing enough of the VC patch (which isn't even fully understood right now, let alone documented) to understand the purpose of the replaced hooks

We know what all of them serve for, and this is really the best we can do. They have descriptive enough names. In cases where they don't, they should be documented like anything else in the codebase. It's simply stupid to imply you need to know everything about a rom/patch you're disassembling to publish a disassembly. For years this wasn't the case with pokecrystal, and it still isn't fully. Don't act like the documentation on the less explored areas like the battle tower is in any better shape than this patch would be.

The point has always been to make enough of it understandable to help people adjust the patch for their hacks, often just doing it automatically. Lack of documentation isn't the issue with the PR and never really was.

No we don't expect anyone to modify it beyond removing existing patches, but I don't see how that's a bad thing. The hooks are too closely tied to the emulator to write any new ones beyond simple code patches, or reducing some custom screen blinking effect for some move. We're not disassembling the entire emulator, we're documenting an official patch for this game. I consider this enough of a reason to add it.

We've dealt with hardcoded addresses before. We're trying to remove and make as many as possible adjustable, that's the entire point of the PR. Could it be that some are left because we don't fully understand the inner workings of the emulator? Sure! But:

• this has no bearing upon people not interested in building for the 3ds virtual console
• as always, mistakes happen and they will be resolved as we discover them with time

Please get off the high horse and actually read the PR+comments and point out the areas where the documentation is lacking. I know a few, and will make sure those are resoslved before the PR is merged. This was the real reason I dropped the old PR; I didn't feel like going through a rewrite of enough of the C program to unhardcode the addresses I found out about later on.

[Rangi42]

@mid-kid @vulcandth However make_patch eventually works, it should be capable of generating the patches for Gold, Red, and Yellow as linked in the top-level post here. So if any of those games need to use const values, or patch identical bytes, or whatever, that should be covered.

[vulcandth]

@mid-kid @vulcandth However make_patch eventually works, it should be capable of generating the patches for Gold, Red, and Yellow as linked in the top-level post here. So if any of those games need to use const values, or patch identical bytes, or whatever, that should be covered.

My full intent is to replicate the work done here to Gold, Red, and Yellow. In fact... a rom hack i'm working on is built on pokegold and I will require the ability to make virtual console patches for it. So I have extra motivation to do so. I'd like to get pokecrystal merged first before working on the others... However You are correct that we should look a little ahead at one might be different for those games. If they require a change on make_patch or anything else is done, we need to try to keep everythign standardized as much as possible between all the repos. I'll start looking into the other repos and making notes.

[mid-kid]

From a cursory look back when I was still looking at this, most of the patches are similar. The main one that stands out is how Yellow patches jinx' skin color. Someone even ported the gen1 patches to gen2 before the gen2 virtual console was available - I wonder to what extent this could be done for other games, though that's out of scope for this issue.

[vulcandth]

Yeah; some of this was addressed and explained a while back by TheStoneBanana. They produced a text document doing some basic explanations of some of the functions and how one might implement them in other games. I have attached this text document below in case anyone wants to look at it. Also the original thread about the port of the virtual console patches by TheStoneBanana is here: https://gbatemp.net/threads/release-pokemon-gold-silver-and-crystal-virtual-console-wireless-linking-patches.439986/

From what was mentioned, porting these hooks to other games is definitely doable; but as mentioned already, outside the scope of this issue. VC Wireless Link Patch Documentation (1).txt

There is also an .ips for patching code.bin to enable the Virtual Console (Debug Menu, Speed Up, Full Screen). The debug menu doesn't appear to contain useful information or insight. Also this is outside the scope of the issue, just including as reference. https://gbatemp.net/threads/pokemon-virtual-console-patches-debug-menu-speed-up-full-screen.498833/