mchlnix / SMB3-Foundry

SMB3 Level Editor in Python
GNU General Public License v3.0
91 stars 12 forks source link

Inconsistency between ROM's search methods and read/write methods #149

Closed fortenbt closed 1 year ago

fortenbt commented 1 year ago

In the new ROM.search and ROM.search_bank methods, rom_data is searched directly based on a start offset or given bank.

In order to be consistent, the offsets used must first have prg_normalize called on them. However, something has never quite sat right with me regarding prg_normalize. If we ever want to access something outside the vanilla ROM bounds (especially as we begin to more fully support expanded ROMs), we may find or calculate that something is in a bank above bank 31. This happens currently with the new heuristic search for Level_BG_Pages. I set up the search bank to be "the second-to-last bank", which is based on the calculation of total number of banks in the ROM. Because of this, the code searches the correct offset in the ROM and finds the location of the array, but then that offset is used to do a bulk_read. Inside of bulk_read, the offset is prg_normalized, causing the offset to become outside the bounds of the rom_data.

The immediate (and technically still correct) fix for this is to not search the "second-to-last" bank, but rather "bank 30". All of our code currently deals only with vanilla ROM things, so expanded ROMs should still be supported by only accessing vanilla ROM banks. However, we may want to create another issue for how we may begin addressing this if we end up fully supporting expanded ROMs by finding things in banks above 31 (like the smb3parse/util/parser might be doing by using the emulator).

fortenbt commented 1 year ago

Strike that. It still won't work to prg_normalize the search. The search works, but then the found offset (in bank 62) is passed along to bulk_read, which prg_normalizes that. So we have to "unnormalize" the found offset.

I'm going to think about a better way to handle this expanded ROM thing than the seemingly half-assed prg_normalize thing we do.

mchlnix commented 1 year ago

Why not handle this with types? Have a RawAddress and a NormalizedAddress. That way we can remember if the Address has already been normalized, which we cannot with simple ints.

Makes it a bit more cumbersome to work with, but at least gets rid of the ambiguity.

I already let mypy run over the smb3parse package and it returns without errors, so we could type check the whole affair (within the package at least).

Unfortunately PySide6 type stubs are really bad about Signals, so type checking foundry and scribe is more problematic.

fortenbt commented 1 year ago

I think this is fine. All the constant values we currently use from smb3parse/constants.py are vanilla ROM constants, so we could treat all integers and RawAddresses as "raw" and anything that is calculated will be a true, NormalizedAddress.

mchlnix commented 1 year ago

closed with #151