bad coding in roms

[Euclid]

So after staring at the following assembly for 20minutes just now, i finally figured out what it does.

Code: [Select]

$0D/FBDA AF 6C F3 7E LDA $7EF36C[$7E:F36C]
$0D/FBDE CF 6D F3 7E CMP $7EF36D[$7E:F36D]
$0D/FBE2 F0 09       BEQ $09    [$FBED]
$0D/FBE4 38          SEC
$0D/FBE5 E9 04       SBC #$04
$0D/FBE7 CF 6D F3 7E CMP $7EF36D[$7E:F36D]
$0D/FBEB B0 00       BCS $00    [$FBED]
$0D/FBED AF 6D F3 7E LDA $7EF36D[$7E:F36D]
$0D/FBF1 18          CLC                 
$0D/FBF2 69 03       ADC #$03             
$0D/FBF4 C2 30       REP #$30

Answer:

Spoiler:

first 7 lines are useless - great for us to insert custom code

Wondering if other people doing assembly level hacks/translations have run into funny coding similar to this?

Normally roms (especially snes/nes ones i have played around with) seems to be fairly well coded and packed without much spare bits and bytes - sure there might be improvements which can be made to save some cpu cycles, but seeing this today... looks like bad coding existed ever since people wrote code 

[justin3009]

Code: [Select]

9.  $C2/801A 9D 7E 31    STA $317E,x[$7E:3544]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/801D 9D 80 31    STA $3180,x[$7E:3546]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8020 9D 82 31    STA $3182,x[$7E:3548]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8023 9D 84 31    STA $3184,x[$7E:354A]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8026 9D 86 31    STA $3186,x[$7E:354C]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8029 9D 88 31    STA $3188,x[$7E:354E]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/802C 9D 8A 31    STA $318A,x[$7E:3550]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/802F 9D 8C 31    STA $318C,x[$7E:3552]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8032 9D 8E 31    STA $318E,x[$7E:3554]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8035 9D 90 31    STA $3190,x[$7E:3556]   A:0140 X:03C6 Y:2000 P:envmxdizc
    $C2/8038 9D 92 31    STA $3192,x[$7E:3558]   A:0140 X:03C6 Y:2000 P:envmxdizc 
    $C2/803B 9D 94 31    STA $3194,x[$7E:355A]   A:0140 X:03C6 Y:2000 P:envmxdizc
Tales of Phantasia.  Take a guess at what this does.

Spoiler:

It's used to erase item names when you scroll down the list.. seriously.. it was this badly coded.

Yep, it was REALLY that bad.  Had to write a mini loop for this sucker just to make it easier.  It legitimately was one of the worst coding pieces I have ever seen ~_~.

[KingMike]

We're talking about most hilariously bad commercially-written code?

Maka-Maka graphics decompression.
It uses the DMA WRAM registers ($2180-2183) to write a single a byte to WRAM. Not only that but it reloads the address registers each time (including doing LDA $2183: ORA $01: STA $2183. As I have heard, only the lowest bit has any significance and the register should be write-only anyways, so it's essentially ensuring the lowest bit of some garbage value is 1, and writing that.)
Oh, and to clear WRAM, it uses a loop routine to clear 64KB one at a time, instead of the DMA registers. It's like the programmers had a completely backwards understanding of what that WRAM DMA register is for. That right there is part of the reason for the game's damn awful load times.
Not to mention how they released an RPG where on the field menus, if you try to use a magic spell, it will ask you which character should cast a spell AND THEN IGNORE YOUR INPUT AND ALWAYS CHOOSE THE PROTAGONIST ANYWAYS.
Well, we won't know who's at fault because the credit scroll is also broken.

And Jerry Boy 2's graphics decompression routine contains a bug that incorrectly detects bank boundaries (using HiROM addressing at $C00000+). If data crosses ROM address xx7FFF, the decompressor will jump 32KB forward in the ROM and continue decompression. Obvious the original devs' graphics compressor had to account for that. How the game managed to stay playable is amazing. (only serious flaw I know of is that the music freezes in the good ending credits, though I don't know if that happens on real hardware or just in emulation)

[LostTemplar]

Yep, it was REALLY that bad.  Had to write a mini loop for this sucker just to make it easier.  It legitimately was one of the worst coding pieces I have ever seen ~_~.

Why is that bad coding? It's pretty common to unroll loops, it saves quite some cycles.

I've seen a lot of horrendous coding in commercial ROMs myself. Even in otherwise good games you sometimes encounter useless code like "REP #$20 : SEP #$20" or "BRA $00". I bet stuff like that stems from the developers using macros.

What's also not necessarily bad coding, but confusing as hell is manipulating the stack like this (Far East of Eden Zero):

Code: [Select]

$C0/075F 48          PHA

$C0/0760 A3 05       LDA $05,s
$C0/0762 18          CLC
$C0/0763 69 01       ADC #$01
$C0/0765 85 5E       STA $5E

$C0/0767 A3 06       LDA $06,s
$C0/0769 69 00       ADC #$00
$C0/076B 85 5F       STA $5F

$C0/076D A3 07       LDA $07,s
$C0/076F 69 00       ADC #$00
$C0/0771 85 60       STA $60

$C0/0773 68          PLA

$C0/0774 3A          DEC A
$C0/0775 18          CLC
$C0/0776 65 5E       ADC $5E
$C0/0778 83 04       STA $04,s
$C0/077A A5 5F       LDA $5F
$C0/077C 69 00       ADC #$00
$C0/077E 83 05       STA $05,s
$C0/0780 A5 60       LDA $60
$C0/0782 69 00       ADC #$00
$C0/0784 83 06       STA $06,s

$C0/0786 A7 5E       LDA [$5E]
$C0/0788 6B          RTL


Spoiler:

First, it gets the return address of the caller and stores it in a pointer. At that return address is stored data (that's very nice when you disassemble the code and suddenly there's data instead of code although there's nothing there to indicate it). It then goes on to adjust the return address to skip the data. It doesn't make things better that the data is stored TWO levels above the current routine (return address of the caller). Finally, it loads the first byte of data into the accumulator and returns that.

I've also seen a bug in a game recently that you won't notice unless you debug it. It has a long list of items of different types. For each menu screen, it adjusts the base index corresponding to the item's type and then only passes the relative index within this type (say items, or magic) when printing the name. But when you change to a different menu screen, it adjusts the base index, but still tries to print the previous menu's strings and thus indicies, which often leads to out-of-bounds indices (base index + relative index). You usually won't notice that because it's displayed only one frame long and affects only the tile map. However, I changed the font to a VWF, and this bug lead to my new code writing garbage all over the screen when changing to some menus. I fixed it by checking for valid indices.

[justin3009]

The thing is with Tales of Phantasia's junk, the game already lags to heck when you scroll anywhere with items as it has to reload the VWF, the item Image, stats, etc..  There's ALWAYS a mini lag that appears.  Adding that little loop didn't really cause anymore delay in the slightest, not too mention it saves room and it allows for easier expansion incase item names are longer than what someone would hope.

Hell, the game has such bad loading times at point that there's literally text in the item menu stating, 'Loading...'

[LostTemplar]

It's a very small loop, so yes, it shouldn't really add that much of a overhead unless it's executed very, very often. For example, I once did an 8x16 VWF and unrolling the loop that shifts each line of the current character halfed the cycles required for the whole routine, which was critical.

Well, what I wanted to say is mainly that it's pretty common to have unrolled loops like that and that it's still easily understood what the code does. But of course using a loop nets you better maintainability and extensibility.

[KingMike]

I've seen a lot of horrendous coding in commercial ROMs myself. Even in otherwise good games you sometimes encounter useless code like "REP #$20 : SEP #$20" or "BRA $00". I bet stuff like that stems from the developers using macros.

Intentional delays, I guess.
(one example I know of is for using the multiplication/division registers on the SNES. Code doing multiplication has to delay, apparently, at least 8 cycles before reading the result.)

[LostTemplar]

That's of course true, but I encountered the examples I mentioned in contexts where no delay was necessary.

For instance, sometimes there's code like this, where the last BRA is completely unnecessary:

Code: [Select]

    ...
    BRA end
    ...
    BRA end
    ...
    BRA end ( = BRA $00)
end:
    ...

I've also seen BRA $FE (an infinite loop) being used for error states and other crazy stuff.

[neige]

The game Kingyo Chuuihou! Tobidase Game Gakuen is full of wasteful coding, like putting a 32Kb+ font in RAM, using pointer tables to access sequential fixed length data, etc.

I think that the gargantuan monster that was used to display answer choices was the worst offender (0x88B03 in the ROM if you want to check). The function used 1319 bytes of ROM and 668 bytes of RAM spanning 589 lines of codes in addition to a table of 560 16-bits pointers to look up the font characters. It started by copying the string to a temporary buffer then copied each character's graphic from the font to another buffer one by one (ie. without using a loop). Since the characters where 12 pixels wide, they shifted every odd character by 4 pixel, again without using a loop. Finally they copied and combined the character graphics to form the final output image.

Another weird thing I saw in another game was a strange looping construct, I don't remember the game but it was something like this:

Code: [Select]

; Code to set-up the A register
loop:
BIT #$68
PHA
; Code that changes the A register
B?? loop+1


Spoiler:

0x68 is the opcode of the PLA instruction. The BIT instruction was used as a no-op to avoid pulling A from the stack when first entering the loop, subsequent runs through the loop pulled A before pushing it again.

[LostTemplar]

Now that's weird and inscrutable. Using

Code: [Select]

    PHA
loop:
    PLA
    PHA
   ...
    B?? loop

was probably too mainstream ;)

[neige]

Exactly, and they can't pretend it's a speed optimization since both the BIT and PLA/PHA take 2 cycles to execute (assuming the m flag is set).

Maybe it was used for obfuscation purposes because when I first tried to disassemble this, I was stumped .

[KC]

Partly the result of wrong data types, and partly the result of probably compiling with debug settings:

Code: [Select]

    08033B8A (T)  ldr     r0,=3003A64h                            ;12 4404
    08033B8C (T)  ldrh    r1,[r0]                                 ;9  4413
    08033B8E (T)  mov     r2,1h                                   ;3  4416
    08033B90 (T)  mov     r0,r1                                   ;3  4419
    08033B92 (T)  and     r0,r2                                   ;3  4422
    08033B94 (T)  mov     r2,r0                                   ;3  4425
    08033B96 (T)  lsl     r1,r2,10h                               ;3  4428
    08033B98 (T)  lsr     r0,r1,10h                               ;3  4431
    08033B9A (T)  cmp     r0,0h                                   ;3  4434Almost all of the Devil Children (GBA) code looks like this.

[MathOnNapkins]

Things that piss me off the most are coders choosing to intentionally use an endianness that is non native on the system in question. Not only does it impact performance somewhat, it also makes code harder to read when you go against the grain. I can only guess that these kinds of people just couldn't wrap their head around little endian.

Zero length branches and jumps used to irritate me more, but then I realized that they were probably written around code that didn't make it to the final binary, either due to comments or assemble time mechanisms (like #ifdef / #ifndef in CPP).

Some of the dumber things I've seen I've chalked up to macros, such as code that performs arithmetic operations to build up a constant. Example:

Code: [Select]

lda #$0030
add #$0100
ora #$8000

instead of:

Code: [Select]

lda #$8130

The truly unforgivable sins are those that impact performance in a strong way without incurring some kind of other benefit. In particular, I despise code that is heavy on comparisons and branches where a lookup table and / or jump table would have been more appropriate (and smaller!).

[henke37]

Some of those are required on fixed length opcode systems. You really can't store a full 32 bit pointer + the instruction type + the destination register in 32 bits.

[snarfblam]

Now that's weird and inscrutable.

I know the BIT trick is pretty common in 6502, where programmers often need to save every byte and cycle possible, so I would expect it to show up in 65816 from time to time, if only out of habit. I've seen better examples of the BIT trick, though.

[LostTemplar]

That example makes more sense, yes.

[Zoinkity]

Most of the crummier code in N64 games is tied to partially-removed debug features.  For instance, something that will read, format, and print debug info to a temporary buffer and then return without doing anything with it, or cases where you loop several times NOPping away. 

Worst offender of all was Datel's GameShark, although to be fair a part of that was due to the hardware itself.  Instead of using the PI to load ROM in large chunks, they copy short values using two different hardware addresses, one mirroring the upper halfword and the other grabbing the lower.  These are pushed and meshed into words, and this is done for each word in ROM.  Mnd you, although that is necessary for the GS's ROM, why they do it for cart ROMs is questionable.
This is especially annoying when doing a CRC calculation.  To switch CRCs (keycodes) it requires loading the entire ROM one short at a time, splicing them together, calcing the checksum, then compensating for the new base value.  This also happens when you upload a new BIOS, in which case it takes the BIOS you uploaded in rdram and reads it not as words but as shorts, calcs the new sum and sets it, needlessly reads its own BIOS and calculates that sum, then copies the new BIOS to hardware one short at a time and recalculates the sum again by reading it all back in.  On a failure, it writes back the original BIOS and recalcs again--which is really funny since a falure to write back properly will result in it writing not the original ROM but the replacement.  Naturally, they just return to the beginning of the process and read the current ROM (failed BIOS), treating the original as the replacement.
Oh, and the 'alternate hook' method will cause a permanent loop in virtually any commercial game.

As far as new code, you should see the mess that passes for reading controller input these days.

[RedComet]

My favorite is the in Zyuranger for the Famicom. The routine that writes data to the PPU has a block of repeating STA $2007 rather than a loop. There was no performance gain from unrolling the loop that I could see.

[danke]

My favorite is the in Zyuranger for the Famicom. The routine that writes data to the PPU has a block of repeating STA $2007 rather than a loop. There was no performance gain from unrolling the loop that I could see.

I was waiting for you to post about that here. Kinda bummed you didn't post the source code for all to see and laugh at.

[Pennywise]

I'm currently working on a game that probably 75% of the text pointers are hardcoded into the ROM. Granted, it's only a 16kb bank, but that's just bad no matter how you cut it. The worst offender is the battle text, which follows some sort of pattern and happens to compromise the bulk of the text, but yet the pointers are all hardcoded.