Virtual Console for Modern Pokecrystal

[vulcandth]

This pull request is in response to issue #813. It is based off of mid-kids' virtual-console branch https://github.com/mid-kid/pokecrystal/tree/virtual-console and has been updated to modern pokecrystal with a few changes as noted below.

Changes from mid-kid's original Pull Request #484:

• Ported code to modern pokecrystal
• Converted to MACRO vc_hooks throughout code
• Created macros/vc.asm for storing VC macros
• Added some useful troubleshooting output to make_patch.c verify_completeness function
• Configured make_patch.c verify_completeness to ignore stadium checksums
• Updated .gitattributes to ensure pokecrystalvc.patch.template is CRLF [Virtual Console will ignore patch file if it is not CRLF]
• Added vc/ directory to .gitignore as this is currently used to clone mid-kid's build cia repo into.
• Added BiographySave_ret assert to notify user of address change. [This hook is hardcoded in the virtual console]
• Added hJoyPressed MACRO assert in macros/vc.asm and hram.asm to notify user if hJoyPressed address has changed. [It shouldn't...]

I'm looking for feedback on what needs to still be worked on, in order to merge this. Again, as mid-kid originally stated when he attempted to merge this in 2018... It is far from perfect; but I believe it is pretty close to mergeable. If any issues arise, it will be on the virtual-console build; and users attempting to build normal pokecrystal should be unaffected. If we can get merged, then it will provide a base to continue improving.

[mid-kid]

Quite a bit to untangle here, from a quick look:

• We need a separate vc_hook and vc_patch macro, ideally with a vc_patch_end to mark the end of a virtual console patch block, instead of having make_patch.c guess the end of it. Maybe even a vc_patch_else macro to get rid of if DEF(_CRYSTALVC) at all, embedding it within the macro. The difference between vc_hook and vc_patch is important at least semantically, this is why there's ; hook and ; patch comments, it's useful for the programmer to know whether the emulator will kick in and execute its spiel or not.
• the hJoyPressed macro is just plain bad. Unlike what you may expect, people are very keen on reordering the layout of hram.asm, as it's a very useful piece of memory (faster than others). There should be a preprocessor command in make_patch.c to get an arbitrary address from the .sym file. The addresses of things in the .sym file can be retrieved with find_symbol. I think it might be useful to redo the format of the preprocessor commands as a whole, but if you don't wish to do that, you may leave that be.
• Don't add vc/ to .gitignore, if people clone the pokecrystal-cia repository to add cia generation functions to the makefile, it's good for it to be checked into the repository as a submodule, as this will tell everyone what version of that repository was used. If you want to ignore it only within your local copy of the repository, add it to the .git/info/exclude file.
• Instead of hardcoding the offset of the stadium checksum in verify_completeness, hardcode it in the same location as the other checksums, the start of process_template. Its offset may have to be calculated the same way as stadium.c does, though I've been considering hardcoding the address in there too, instead of expanding it depending on the ROM size like is being done currently.

There's more, but this is just the few things I can think of so quickly. I'll look more in depth tonight.

[vulcandth]

I appreciate the feedback a lot; I figured this still had a lot of work.. and you made a lot of good points to make me stop being lazy. I'll get to working on fixing the issues you addressed. As far as your first two bullets; I don't know why I didn't differentiate between the hooks and patches in the macros... Easy enough. As far as hJoyPressed; i've been lazy and trying to avoid having to rewrite a lot of your make_patch.c. I made a few half hearted attempts to go through it and find easy changes.. but I ended up opting for the easy way out with the assert. Obviously isn't' the way to go.

I'll admit my programming skills are still somewhat beginner; and I only started learning Assembly, RGBASM, and pokemon rom hacking like two-ish weeks ago lol. However, I like projects like this as it forces me to learn. I'll have some fixed commits soonish for further review.

[mid-kid]

Don't sweat it, I'm happy someone's already gone through making sure it's compatible with modern pokecrystal and has already done part of the work necessary to get it upstream!

Unfortunately, it takes a bit more than you'd initially expect to get a feature like this merged, in a way that's ready for others to use and improve, without hitting stone walls/annoyances when editing the ROM like normal. This can take a bit of back and forth until we can settle on something agreeable at times, though I don't think this feature will need a lot of that.

That said, at any point you're free to just leave the PR as is, and leave it to someone else to finish. We'll keep it open as long as the issue with the feature request exists, so don't see it as an obligation.

[kanzure]

Is this an actual console? Can I see some screenshots?

[vulcandth]

Is this an actual console? Can I see some screenshots?

This is to enable support for building a patch file to be used when importing roms into Nintendo's Virtual Console to be used on Nintendo 3DS/2DS' systems. The Nintendo Virtual Console uses these patch files to know when to initiate functions for Link Trade/Battle by watching for code execution in the rom; as well as what locations it should patch. (There is a lot more to it.. and poorly explained... but should give you an idea.)

[aaaaaa123456789]

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.

[aaaaaa123456789]

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.

[vulcandth]

@mid-kid I Implemented a vc_patch macro in 89b82f0 and 612858d however I don't believe it is possible to split a conditional IF statement between multiple macros.. So I don't believe the vc_patch_else and vc_path_end will work.

@aaaaaa123456789 I appreciate your input (No sarcasm), i'm only responding to an issue that has been open #813. In order not to clutter up the Pull Request with a discussion, may I suggest that it might be better to have this discussion there.. and in the meantime i'll continue working on the PR here.

[mid-kid]

So I don't believe the vc_patch_else and vc_path_end will work.

vc_patch: MACRO
VC_PATCH_CUR = "\1"
.VC_{VC_PATCH_CUR}::
if DEF(_CRYSTALVC)
ENDM

vc_patch_else: MACRO
else
ENDM

vc_patch_end: MACRO
endc
.VC_{VC_PATCH_CUR}_end::
ENDM

Or something to this effect. I forget how string interpolation works in recent versions of rgbds.

Though I'd like to ask @Rangi42 for input before doing this. I'm a bit iffy on expressing normal conditionals with macros as well, though we do need start and end markers for a patch.

[Rangi42]

Those vc_patch_if/else/end macros won't work. RGBASM looks for the literal "else" and "endc" identifiers when skipping the if or the else block depending on the condition. Given how macros and EQUs can expand to anything, it could not be otherwise.

I'm fine with patches being surrounded by standard if DEF(_CRYSTALVC)/else/endc syntax. It's obvious what's happening, a comment can explain why the patch is there (e.g. "Thunder flashes more slowly to avoid triggering epilepsy"), and it's no worse than the bowdlerized _CRYSTAL_AU strings for usability.

[Rangi42]

How about this:

    vc_patch Foo
if DEF(_CRYSTAL_VC)
    ld a, 42
else
    ld a, 100
endc
    vc_patch_end Foo

Then vc_patch_end Foo can create a .VC_End_Foo label, and make_patch can use .VC_End_Foo - .VC_Foo for the patch size, without needing the unpatched ROM for comparison.

[mid-kid]

@Rangi42 what about my idea to use the value passed to vc_patch within vc_patch_end? That would reduce repetition and potential problems with mismatched names.

[Rangi42]

@Rangi42 what about my idea to use the value passed to vc_patch within vc_patch_end? That would reduce repetition and potential problems with mismatched names.

Yeah, that sounds good:

vc_patch: MACRO
	assert !DEF(CURRENT_VC_PATCH), "Already started a vc_patch"
CURRENT_VC_PATCH EQUS "\1"
.VC_{CURRENT_VC_PATCH}::
ENDM

vc_patch_end: MACRO
	assert DEF(CURRENT_VC_PATCH), "No vc_patch started"
.VC_End_{CURRENT_VC_PATCH}::
	PURGE CURRENT_VC_PATCH
ENDM

    vc_patch Foo
if DEF(_CRYSTAL_VC)
    ld a, 2
else
    ld a, 1
endc
    vc_patch_end

[vulcandth]

Give it another two weeks and @Rangi42 will have it down under 250 lines of code.

[Rangi42]

Give it another two weeks and Rangi42 will have it down under 250 lines of code.

Not quite, but @mid-kid I did take your suggestion to remove the single-line braces, bringing it to 399 lines. (Without any comments or blank lines it would still be 330.)

[Rangi42]

To do:

• Document vc_hook and vc_patch/vc_patch_end macros
• Only discuss the casing and underscore/space command variations once
• Only discuss "value series" once, mentioning "two-digit hexadecimal bytes"
• Describe db as for a byte and dws as for little-endian words
• Try to identify the remaining values in the printer-related ConditionValueCs

[vulcandth]

I don't believe that the remaining ConditionValueC's should be a blocker from merge. I will continue to research better ways to abstract the remaining items; Fortunately the ONLY thing these Print Forbids actually do is block the player from pressing any button other than D_DOWN, D_UP, B_BUTTON, or simply providing NO_INPUT when the player is attempting to do a print function. They did a really lazy implementation at this, and its easy to circumvent. If these sections are removed it doesn't cause the game to crash.. the game just tells the user that the GameBoy Printer is unavailable. They are rather pointless. However, for the sake of perfectly replicating the original patch files, they are here. The Wiki should recommend that homebrew users should REMOVE these sections and perhaps provide manual in-game code changes if they want to.

I fully support continuing to find ways to better abstract the values, and making them more dynamic... but this will take some time.

What I know of the remaining values:

• wWindowStackPointer:: dw also begins at the same address as wMenuMetadata::; It appears to correspond to specific values representing a particular menu sequence? All I know is whenever you open the menu in the PokeDex for printer, Opening the Bill's PC Change Box for printing, or when you are presented with the Yes/No menu at the Cianwood Photo Studio. I believe this will be hard to extrapolate due to the stack's push, pop stuff. I'd need to learn more how it actually works.
• wMenuCursorY is literally a value that corresponds to menu entry that the cursor is currently on. I'm not sure what more we'd want from this.
• wMenuSelection I believe this corresponds to wMenuCursorY... althought i'm not sure why the VC developers decided to include this. Perhaps it was to ensure id there is a duplicate wWindowStackPointer in the game it checks the length of the options, like in [print forbid 2]?? not sure right now.
• wXCoord Literrally a check for the x coord of where the player is standing.
• wYCoord Literrally a check for the y coord of where the player is standing.
• wDexArrowCursorPosIndex Like wMenuCursorY but for use in the pokdex. In the case it represents when the user is hovering over the PRINT option.

[print forbid 1]: Prevents printing unknown
[print forbid 2]: Prevents printing from Bill's PC CHANGE BOX. (Player's box pokemon list)
[print forbid 3]: Prevents printing at the Cianwood Photo Studio if the player is standing diretly below the photographer.
[print forbid 4]: Prevents printing mail.
[print forbid 5]: Prevents printing pokedex.

[mid-kid]

wMenuMetadata is the name of a structure (running from wMenuMetadata to wMenuMetadataEnd) of which wWindowStackPointer is the first element. This stack pointer points to a location between wWindowStack and wWindowStackBottom where the tiles that are obscured by the currently open menu are saved (because menus can stack on top of eachother, this is implemented as a stack). Of course this means it depends on how big the menus are that are loaded before this menu, the hard part about this is that it's in no way an identification of the menu, the developers just chose to rely on this.

wMenuSelection stands separate from wMenuCursorY in the sense that it's not an Y coordinate relative to the top left corner of the menu box. wMenuSelection will usually be equal to (wMenuCursorY - 1) / 2, except in horizontal or 2d menus. It seems the devs chose to rely more on the latter to identify the menu selection. Maybe they could not figure out what wMenuSelection meant, and how wDexArrowCursorPosIndex and similar replace it in "non-standard" menus, but either way it's a pain.

Ideally all of this would somehow be programatically calculated, by for example having constants for the menu entries in the menus that need them (maybe even a constant to determine the NPC's position), but I agree doing so is a pain, would involve a bunch of hacks, and I'd rather push the PR through asap, so I'm fine with just documenting this, too. Maybe with {# comment}s, maybe with comments in the disassembly. Someone looking to make their hack work on VC should stumble upon these comments when they notice a random menu's 2nd option isn't being able to be pressed because their window stack pointer unfortunately landed on print_forbid_2's 0xd3dd. Someone editing the menu options in the pokedex should be aware that the 4th option is shadowed by this patch when editing DexEntryScreen_ArrowCursorData (or surrounding code), etc.

@Rangi42 Thoughts on where to put these comments?

[Rangi42]

Ideally all of this would somehow be programatically calculated, by for example having constants for the menu entries in the menus that need them (maybe even a constant to determine the NPC's position)

I think constants for the NPC position would be overkill; it's obvious from the wMapNumber and wMapGroup checks, plus the names of the wXCoord and wYCoord variables being checked, that the combined check is for a position in a .blk map.

If constants are needed specifically for the VC, we might be able to put them in custom vc_const statements.

If I didn't know why my VC menu was failing, the menu code is the first place I'd look, so IMO that's where a comment should be.

[mid-kid]

If I didn't know why my VC menu was failing, the menu code is the first place I'd look, so IMO that's where a comment should be.

Even if you know that it works on a regular emulator?

[vulcandth]

IMO; we should just packaged this information into the Wiki. Perhaps have make_patch print out a message after successful execution to refer to the Wiki page for troubleshooting or use in romhacks.

Or you could have MakeFile run a SHA check on the original rom to check for modifications.. If modified. If the user is building a patch using make crystal11vc or whatever.. it can just print out a message the inform the user to check the wiki. Maybe even include a way to only print the first time its run?

I'm just trying to think outside the box.

Should you decide to just go with the comment method; i'd be fine with that too.

[mid-kid]

You might be right - documenting this in the "hard-coded logic" page might make more sense. This can be fixed over time so I'm fine with just not doing anything right now, I guess.

[vulcandth]

This PR has gotten rather... LONG.... my recommendation. Lets finish the doc modfications @mid-kid wanted and merge this PR... and the create new github issues for the rest of the tasks (ex: Identify remaining CondValueC, Customizable labels, ect.). I can then open up new PR's in regards to those issues when we have enough info on how we want to address them.

What you think @mid-kid @Rangi42.

[mid-kid]

Sounds good.