There was a problem with Spanish Emerald triggering a crash in mystery_gift_injector.cpp
The problem was that the section30 and script BPS patch FileContainers specify @chunkSize=4096 in their .containerdef files.
However, mystery_gift_injector.cpp didn't specify a chunkSize in the constructor of the FileContainerReader instances in reconstructPregeneratedPayloads()
This resulted in it falling back to DEFAULT_CHUNK_SIZE, which is 2048.
Fix: Specify chunkSize 4096 as a constructor argument!
Do some more work for multilang gba payloads. Revise the patch structure and make it use the right RSEFRLG table.
But it's broken. For some reason the english text doesn't show up when running the payload. Don't ask me why yet.
first_time entries would be longer, therefore the reference pointers inside the payload would shift.
To avoid that, we now reserve space for the longest entry, knowing that the texts will get stripped after
the payloads have been generated.
Doing so ensures that the text offsets remain constant, no matter if we're dealing with the first_time or othertimes payload.
This also means we need to take that reservation into account on the PTGB rom side of things of course.
To do so, I created a pair array indicating the first time entries and there non-first_time counterparts.
We'll determine which entry is the longest and ensure we use that length for the offset of the next entry.
This commit makes mystery_gift_injector re-insert the text entries into the gba payload at runtime.
You may or may not remember that I made gba-payload-generator strip the text entries again after generating the payloads.
I did this to improve compression: it removes the variability of the text lengths. It also ensures that texts are not really part of the BPS patch files
that are generated to transform the base payload into a specific language and game variant.
Therefore, we don't run at risk of storing certain text entries multiple times (or partially multiple times) due to them being stored at FileContainer boundaries.
It should make patch files smaller and more predictable.
Stripping the text entries also made it easy to reinsert: almost every rom uses the same offset for the first entry.
We now only need to store each language variant of the RSEFRLG (variants aren't being generated yet) once. That should be a big boon.
By first generating the payloads and THEN stripping the texts, we also ensure that the pointers/references within the payload remain intact and will become valid
again once the texts get reinserted.
Anyway, this specific commit does the re-insertion of those text entries into the payload (script + section30) using the RSEFRLG table.
Because the text stripping takes out these variable length texts, the text insertion offsets always end up the same, no matter the language:
Section30: 0x97E
Script:
- Ruby/Sapphire: 0x268
- Fire Red/Leaf Green: 0x278
- Emerald: 0x298
Problem: several variables were used in mystery_gift_builder before they were initialized.
Fix: initialize them.
I also made sure to memset the entire buffer before generating a payload to avoid garbage data to seep in.
This commit integrates the pregenerated payload into mystery_gift_injector.
It currently hardcodes the English Ruby v1.2 payload for testing purposes though.
The next test I'll have to do is to see if the transfer process works with this specific rom.
If it works, I can start integrating Pi's work in regards to delta patching using BPS.
There was a crash happening with ptgb::vector when you'd press A on the CONFIRM button of the box screen. It only occurred on actual gba hardware and
was a real heisenbug: as soon as you'd add code to display logs on screen, the problem would disappear. So it was very difficult to figure this one
out. We're not even entirely sure why, but it looks like the malloc/realloc/free use in ptgb::vector would cause issues.
Maybe it was alignment, but after messing with the code we also saw a warning appear in the terminal telling us that realloc wouldn't properly
deal with non-POD types. It complained about this very thing while referring to the add_track() function, which stores ptgb::vectors inside another
ptgb::vector. We also didn't have a custom copy constructor yet to actually copy the buffer instead of its pointer.
All of these could potentially have led to the crash. But debugging during the link cable flow was difficult, so we were never able to confirm it in
a debugger, log or dump.
Because I suspected the high IWRAM consumption (especially now with ZX0 decompression) for a while, I also did an optimization in mystery_gift_builder
to pass global_memory_buffer as its section_30_data buffer instead. This reduces IWRAM consumption by 4 KB.
There was another problem I discovered during my crash hunt: the out_array (now payload_buffer) was allocated as a 672 byte array, but the payloads
were actually 707 bytes. Therefore writing this to the buffer caused a buffer overflow, thereby corrupting the global variables appearing after it in
IWRAM. It turned out eventually that none of these variables were really critical, but it could explain some minor bugs GearsProgress has seen.
I also did a few performance optimizations:
- At various stages in the code, for loops were used to copy data from one buffer into another byte-by-byte. This was far from optimal because the gba
cpu can load/copy 4 bytes at a time if you ask it to. So I replaced those with memcpy(), which is a hand-optimized assembly function to copy data
using this principle.
- generate_payload was being called twice: once at start_link and once at continue_link, giving the exact same result, even though it was already
being stored in a global buffer allocated in IWRAM. This was also a fairly heavy function. So I optimized the code to only initialize it once in
the script chain and then just retrieve the buffer.
- generate_payload was constructing the eventual payload twice even within the same call. That's because it first merged z80_rng_seed, z80_payload
and z80_patchlist into a full_data ptgb::vector, after which it then copied the data again to out_array (now called payload_buffer). I eliminated the
full_data vector now.
Add a binary table format and convert the text entries into this format in text_helper/main.py. It then gets compressed with zx0.
The new text_data_table and streamed_data_table classes exist to read the various entries from this binary table. streamed_data_table specifically
exists to use a decompression buffer that is smaller than the actual binary table. But it requires a decompression buffer that is
still larger than ZX0_DEFAULT_WINDOW_SIZE (default: 2048 bytes) and will only be able to decompress in
chunks of (<decompression_buffer_size> - <ZX0_DEFAULT_WINDOW_SIZE>) bytes
Try to keep the binary text tables sufficiently small though, because since zx0 doesn't actually support random access,
getting to the last entry is significantly more expensive than reading the first one. And unless you use streamed_data_table,
it also requires <uncompressed_size> bytes of stack space, therefore IWRAM to decompress them.
I also had to rework script_array because it can no longer reference the strings directly. Instead we now reference the DIA_* "enum" values.
We also no longer store an array of script_obj instances, because these were getting stored in IWRAM since they're non-const global variables
originally. Instead we now have const arrays of script_obj_params structs, which should end up in .rodata -> therefore EWRAM.
Right now, script_obj only supports the PTGB text table (originally the dialogue array). But if the need arises to support other tables as well,
I'd consider adding a separate enum to script_obj_params to indicate the specific table.
The compilation process will also output .su files in the build folder from now on. These files indicate the stack frame size for every function in
every compilation unit, so be sure to check them from time to time. Note that they will only show the stack consumption for that specific function.
So to get the worst case stack consumption, you need to manually add all the functions in a certain stack flow.
