pokehacks/pokecrystal-board
0
0
Fork 0
mirror of https://gitlab.com/xCrystal/pokecrystal-board.git synced 2024-09-09 09:51:34 -07:00
Code Issues Packages Projects Releases Wiki Activity
271aa20b73
pokecrystal-board/docs/design_flaws.md

757 lines
14 KiB
Markdown
Raw Blame History

# Design Flaws
These are parts of the code that do not work *incorrectly*, like [bugs and glitches](/docs/bugs_and_glitches.md), but that clearly exist just to work around a problem. In other words, with a slightly different design, the code would not need to exist at all. Design flaws may be exceptions to a usual rule, such as "tables of pointers in different banks use `dba`" ([one exception](#pic-banks-are-offset-by-pics_fix), [and another](#pokédex-entry-banks-are-derived-from-their-species-ids)) or "graphics used as a unit are stored and loaded contiguously" ([a notable exception](#footprints-are-split-into-top-and-bottom-halves)).
## Contents
- [Pic banks are offset by `PICS_FIX`](#pic-banks-are-offset-by-pics_fix)
- [`PokemonPicPointers` and `UnownPicPointers` are assumed to start at the same address](#pokemonpicpointers-and-unownpicpointers-are-assumed-to-start-at-the-same-address)
- [Footprints are split into top and bottom halves](#footprints-are-split-into-top-and-bottom-halves)
- [`ITEM_C3` and `ITEM_DC` break up the continuous sequence of TM items](#item_c3-and-item_dc-break-up-the-continuous-sequence-of-tm-items)
- [Pokédex entry banks are derived from their species IDs](#pokédex-entry-banks-are-derived-from-their-species-ids)
- [Identical sine wave code and data is repeated five times](#identical-sine-wave-code-and-data-is-repeated-five-times)
- [`GetForestTreeFrame` works, but it's still bad](#getforesttreeframe-works-but-its-still-bad)
## Pic banks are offset by `PICS_FIX`
[data/pokemon/pic_pointers.asm](/data/pokemon/pic_pointers.asm), [data/pokemon/unown_pic_pointers.asm](/data/pokemon/unown_pic_pointers.asm), and [data/trainers/pic_pointers.asm](/data/trainers/pic_pointers.asm) all have to use `dba_pic` instead of `dba`. This is a macro in [macros/data.asm](/macros/data.asm) that offsets banks by `PICS_FIX`:
```asm
dba_pic: MACRO ; dbw bank, address
db BANK(\1) - PICS_FIX
dw \1
ENDM
```
The offset is translated into a correct bank by `FixPicBank` in [engine/load_pics.asm](/engine/load_pics.asm):
```asm
FixPicBank: ; 511c5
; This is a thing for some reason.
PICS_FIX EQU $36
GLOBAL PICS_FIX
push hl
push bc
sub BANK(Pics_1) - PICS_FIX
ld c, a
ld b, 0
ld hl, .PicsBanks
add hl, bc
ld a, [hl]
pop bc
pop hl
ret
.PicsBanks: ; 511d4
db BANK(Pics_1) + 0
db BANK(Pics_1) + 1
db BANK(Pics_1) + 2
db BANK(Pics_1) + 3
db BANK(Pics_1) + 4
db BANK(Pics_1) + 5
db BANK(Pics_1) + 6
db BANK(Pics_1) + 7
db BANK(Pics_1) + 8
db BANK(Pics_1) + 9
db BANK(Pics_1) + 10
db BANK(Pics_1) + 11
db BANK(Pics_1) + 12
db BANK(Pics_1) + 13
db BANK(Pics_1) + 14
db BANK(Pics_1) + 15
db BANK(Pics_1) + 16
db BANK(Pics_1) + 17
db BANK(Pics_1) + 18
db BANK(Pics_1) + 19
db BANK(Pics_1) + 20
db BANK(Pics_1) + 21
db BANK(Pics_1) + 22
db BANK(Pics_1) + 23
```
**Fix:** Use `dba` instead of `dba_pic`, and don't call `FixPicBank` to modify `a`.
## `PokemonPicPointers` and `UnownPicPointers` are assumed to start at the same address
In [gfx/pics.asm](/gfx/pics.asm):
```asm
; PokemonPicPointers and UnownPicPointers are assumed to start at the same
; address, but in different banks. This is enforced in pokecrystal.link.
SECTION "Pic Pointers", ROMX
INCLUDE "data/pokemon/pic_pointers.asm"
SECTION "Unown Pic Pointers", ROMX
INCLUDE "data/pokemon/unown_pic_pointers.asm"
```
In [pokecrystal.link](/pokecrystal.link):
```
ROMX $48
org $4000
"Pic Pointers"
"Pics 1"
ROMX $49
org $4000
"Unown Pic Pointers"
"Pics 2"
```
Two routines in [engine/load_pics.asm](/engine/load_pics.asm) make this assumption; `GetFrontpicPointer`:
```asm
ld a, [wCurPartySpecies]
cp UNOWN
jr z, .unown
ld a, [wCurPartySpecies]
ld d, BANK(PokemonPicPointers)
jr .ok
.unown
ld a, [wUnownLetter]
ld d, BANK(UnownPicPointers)
.ok
ld hl, PokemonPicPointers ; UnownPicPointers
dec a
ld bc, 6
call AddNTimes
```
And `GetMonBackpic`:
```asm
; These are assumed to be at the same address in their respective banks.
ld hl, PokemonPicPointers ; UnownPicPointers
ld a, b
ld d, BANK(PokemonPicPointers)
cp UNOWN
jr nz, .ok
ld a, c
ld d, BANK(UnownPicPointers)
.ok
dec a
ld bc, 6
call AddNTimes
```
**Fix:**
Don't enforce `org $4000` in pokecrystal.link.
Modify `GetFrontpicPointer`:
```asm
ld a, [wCurPartySpecies]
cp UNOWN
jr z, .unown
ld a, [wCurPartySpecies]
ld hl, PokemonPicPointers
ld d, BANK(PokemonPicPointers)
jr .ok
.unown
ld a, [wUnownLetter]
ld hl, UnownPicPointers
ld d, BANK(UnownPicPointers)
.ok
dec a
ld bc, 6
call AddNTimes
```
And `GetMonBackpic`:
```asm
ld a, b
ld hl, PokemonPicPointers
ld d, BANK(PokemonPicPointers)
cp UNOWN
jr nz, .ok
ld a, c
ld hl, UnownPicPointers
ld d, BANK(UnownPicPointers)
.ok
dec a
ld bc, 6
call AddNTimes
```
## Footprints are split into top and bottom halves
In [gfx/footprints.asm](/gfx/footprints.asm):
```asm
; Footprints are 2x2 tiles each, but are stored as a 16x64-tile image
; (32 rows of 8 footprints per row).
; That means there's a row of the top two tiles for eight footprints,
; then a row of the bottom two tiles for those eight footprints.
; These macros help extract the first and the last two tiles, respectively.
footprint_top EQUS "0, 2 * LEN_1BPP_TILE"
footprint_bottom EQUS "2 * LEN_1BPP_TILE, 2 * LEN_1BPP_TILE"
; Entries correspond to Pokémon species, two apiece, 8 tops then 8 bottoms
; 001-008 top halves
INCBIN "gfx/footprints/bulbasaur.1bpp", footprint_top
INCBIN "gfx/footprints/ivysaur.1bpp", footprint_top
INCBIN "gfx/footprints/venusaur.1bpp", footprint_top
INCBIN "gfx/footprints/charmander.1bpp", footprint_top
INCBIN "gfx/footprints/charmeleon.1bpp", footprint_top
INCBIN "gfx/footprints/charizard.1bpp", footprint_top
INCBIN "gfx/footprints/squirtle.1bpp", footprint_top
INCBIN "gfx/footprints/wartortle.1bpp", footprint_top
; 001-008 bottom halves
INCBIN "gfx/footprints/bulbasaur.1bpp", footprint_bottom
INCBIN "gfx/footprints/ivysaur.1bpp", footprint_bottom
INCBIN "gfx/footprints/venusaur.1bpp", footprint_bottom
INCBIN "gfx/footprints/charmander.1bpp", footprint_bottom
INCBIN "gfx/footprints/charmeleon.1bpp", footprint_bottom
INCBIN "gfx/footprints/charizard.1bpp", footprint_bottom
INCBIN "gfx/footprints/squirtle.1bpp", footprint_bottom
INCBIN "gfx/footprints/wartortle.1bpp", footprint_bottom
...
```
`Pokedex_LoadAnyFootprint` in [engine/pokedex/pokedex.asm](/engine/pokedex/pokedex.asm):
```asm
push hl
ld e, l
ld d, h
ld hl, vTiles2 tile $62
lb bc, BANK(Footprints), 2
call Request1bpp
pop hl
; Whoever was editing footprints forgot to fix their
; tile editor. Now each bottom half is 8 tiles off.
ld de, 8 tiles
add hl, de
ld e, l
ld d, h
ld hl, vTiles2 tile $64
lb bc, BANK(Footprints), 2
call Request1bpp
```
**Fix:**
Store footprints contiguously:
```asm
INCBIN "gfx/footprints/bulbasaur.1bpp"
INCBIN "gfx/footprints/ivysaur.1bpp"
INCBIN "gfx/footprints/venusaur.1bpp"
INCBIN "gfx/footprints/charmander.1bpp"
INCBIN "gfx/footprints/charmeleon.1bpp"
INCBIN "gfx/footprints/charizard.1bpp"
INCBIN "gfx/footprints/squirtle.1bpp"
INCBIN "gfx/footprints/wartortle.1bpp"
...
```
Modify `Pokedex_LoadAnyFootprint`:
```asm
ld e, l
ld d, h
ld hl, vTiles2 tile $62
lb bc, BANK(Footprints), 4
call Request1bpp
```
## `ITEM_C3` and `ITEM_DC` break up the continuous sequence of TM items
[constants/item_constants.asm](/constants/item_constants.asm) defined the 50 TMs in order with `add_tm`, but `ITEM_C3` and `ITEM_DC` break up that sequence.
```asm
add_tm DYNAMICPUNCH ; bf
...
add_tm ROLLOUT ; c2
const ITEM_C3 ; c3
add_tm ROAR ; c4
...
add_tm DIG ; db
const ITEM_DC ; dc
add_tm PSYCHIC_M ; dd
...
add_tm NIGHTMARE ; f2
NUM_TMS = const_value - TM01 - 2 ; discount ITEM_C3 and ITEM_DC
```
`GetTMHMNumber` and `GetNumberedTMHM` in [engine/items.asm](/engine/items.asm) have to compensate for this:
```asm
GetTMHMNumber:: ; d407
; Return the number of a TM/HM by item id c.
ld a, c
; Skip any dummy items.
cp ITEM_C3 ; TM04-05
jr c, .done
cp ITEM_DC ; TM28-29
jr c, .skip
dec a
.skip
dec a
.done
sub TM01
inc a
ld c, a
ret
GetNumberedTMHM: ; d417
; Return the item id of a TM/HM by number c.
ld a, c
; Skip any gaps.
cp ITEM_C3 - (TM01 - 1)
jr c, .done
cp ITEM_DC - (TM01 - 1) - 1
jr c, .skip_one
.skip_two
inc a
.skip_one
inc a
.done
add TM01
dec a
ld c, a
ret
```
**Fix:**
Move `ITEM_C3` and `ITEM_DC` above all the TMs in every table of item data.
Modify engine/items.asm:
```asm
GetTMHMNumber:: ; d407
; Return the number of a TM/HM by item id c.
ld a, c
sub TM01
inc a
ld c, a
ret
GetNumberedTMHM: ; d417
; Return the item id of a TM/HM by number c.
ld a, c
add TM01
dec a
ld c, a
ret
```
## Pokédex entry banks are derived from their species IDs
`PokedexDataPointerTable` in [data/pokemon/dex_entry_pointers.asm](/data/pokemon/dex_entry_pointers.asm) is a table of `dw`, not `dba`, yet there are four banks used for Pokédex entries. The correct bank is derived from the species ID at the beginning of each Pokémon's base stats. (This is the only use the base stat species ID has.)
Three separate routines do the same derivation; `GetDexEntryPointer` in [engine/pokedex/pokedex_2.asm](/engine/pokedex/pokedex_2.asm):
```asm
GetDexEntryPointer: ; 44333
; return dex entry pointer b:de
push hl
ld hl, PokedexDataPointerTable
ld a, b
dec a
ld d, 0
ld e, a
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
push de
rlca
rlca
maskbits NUM_DEX_ENTRY_BANKS
ld hl, .PokedexEntryBanks
ld d, 0
ld e, a
add hl, de
ld b, [hl]
pop de
pop hl
ret
.PokedexEntryBanks:
db BANK(PokedexEntries1)
db BANK(PokedexEntries2)
db BANK(PokedexEntries3)
db BANK(PokedexEntries4)
```
`GetPokedexEntryBank` in [engine/item_effects.asm](/engine/item_effects.asm):
```asm
GetPokedexEntryBank:
push hl
push de
ld a, [wEnemyMonSpecies]
rlca
rlca
maskbits NUM_DEX_ENTRY_BANKS
ld hl, .PokedexEntryBanks
ld d, 0
ld e, a
add hl, de
ld a, [hl]
pop de
pop hl
ret
.PokedexEntryBanks:
db BANK(PokedexEntries1)
db BANK(PokedexEntries2)
db BANK(PokedexEntries3)
db BANK(PokedexEntries4)
```
And `PokedexShow_GetDexEntryBank` in [engine/radio.asm](/engine/radio.asm):
```asm
PokedexShow_GetDexEntryBank:
push hl
push de
ld a, [wCurPartySpecies]
dec a
rlca
rlca
maskbits NUM_DEX_ENTRY_BANKS
ld hl, .PokedexEntryBanks
ld d, 0
ld e, a
add hl, de
ld a, [hl]
pop de
pop hl
ret
.PokedexEntryBanks:
db BANK(PokedexEntries1)
db BANK(PokedexEntries2)
db BANK(PokedexEntries3)
db BANK(PokedexEntries4)
```
**Fix:** Use `dba` instead of `dw` in `PokedexDataPointerTable`, and modify the code that accesses it to match.
## Identical sine wave code and data is repeated five times
`_Sine` in [engine/routines/sine.asm](/engine/routines/sine.asm):
```asm
_Sine:: ; 84d9
; A simple sine function.
; Return d * sin(e) in hl.
; e is a signed 6-bit value.
ld a, e
and %111111
cp %100000
jr nc, .negative
call .ApplySineWave
ld a, h
ret
.negative
and %011111
call .ApplySineWave
ld a, h
xor $ff
inc a
ret
.ApplySineWave: ; 84ef
ld e, a
ld a, d
ld d, 0
ld hl, .sinewave
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
ld hl, 0
; Factor amplitude
.multiply
srl a
jr nc, .even
add hl, de
.even
sla e
rl d
and a
jr nz, .multiply
ret
.sinewave ; 850b
; A 32-word table representing a sine wave.
; sin(90 degrees) is index $10 with an amplitude of $100.
sine_wave 32
```
`Sprites_Cosine` and `Sprites_Sine` in [engine/sprites.asm](/engine/sprites.asm):
```asm
Sprites_Cosine: ; 8e72a
; a = d * cos(a * pi/32)
add %010000
Sprites_Sine: ; 8e72c
; a = d * sin(a * pi/32)
and %111111
cp %100000
jr nc, .negative
call .ApplySineWave
ld a, h
ret
.negative
and %011111
call .ApplySineWave
ld a, h
xor $ff
inc a
ret
; 8e741
.ApplySineWave: ; 8e741
ld e, a
ld a, d
ld d, 0
ld hl, .sinewave
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
ld hl, 0
.multiply
srl a
jr nc, .even
add hl, de
.even
sla e
rl d
and a
jr nz, .multiply
ret
; 8e75d
.sinewave ; 8e75d
sine_wave 32
```
`BattleAnim_Cosine` and `BattleAnim_Sine` in [engine/battle_anims/functions.asm](/engine/battle_anims/functions.asm):
```asm
BattleAnim_Cosine: ; ce732 (33:6732)
; a = d * cos(a * pi/32)
add %010000
BattleAnim_Sine: ; ce734 (33:6734)
; a = d * sin(a * pi/32)
and %111111
cp %100000
jr nc, .negative
call .ApplySineWave
ld a, h
ret
.negative
and %011111
call .ApplySineWave
ld a, h
xor $ff
inc a
ret
.ApplySineWave:
ld e, a
ld a, d
ld d, 0
ld hl, BattleAnimSineWave
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
ld hl, 0
.multiply
srl a
jr nc, .even
add hl, de
.even
sla e
rl d
and a
jr nz, .multiply
ret
...
BattleAnimSineWave: ; ce77f
sine_wave 32
; ce7bf
```
`StartTrainerBattle_DrawSineWave` in [engine/battle/battle_transition.asm](/engine/battle/battle_transition.asm):
```asm
StartTrainerBattle_DrawSineWave: ; 8c6f7 (23:46f7)
; a = d * sin(a * pi/32)
and %111111
cp %100000
jr nc, .negative
call .ApplySineWave
ld a, h
ret
.negative
and %011111
call .ApplySineWave
ld a, h
xor $ff
inc a
ret
.ApplySineWave: ; 8c70c (23:470c)
ld e, a
ld a, d
ld d, 0
ld hl, .sinewave
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
ld hl, 0
.multiply
srl a
jr nc, .even
add hl, de
.even
sla e
rl d
and a
jr nz, .multiply
ret
; 8c728 (23:4728)
.sinewave ; 8c728
sine_wave 32
; 8c768
```
And `CelebiEvent_Cosine` in [engine/events/celebi.asm](/engine/events/celebi.asm):
```asm
CelebiEvent_Cosine: ; 49b3b (12:5b3b)
; a = d * cos(a * pi/32)
add %010000
and %111111
cp %100000
jr nc, .negative
call .ApplySineWave
ld a, h
ret
.negative
and %011111
call .ApplySineWave
ld a, h
xor $ff
inc a
ret
.ApplySineWave: ; 49b52 (12:5b52)
ld e, a
ld a, d
ld d, 0
ld hl, .sinewave
add hl, de
add hl, de
ld e, [hl]
inc hl
ld d, [hl]
ld hl, 0
.multiply
srl a
jr nc, .even
add hl, de
.even
sla e
rl d
and a
jr nz, .multiply
ret
; 49b6e (12:5b6e)
.sinewave ; 49b6e
sine_wave 32
; 49bae
```
**Fix:** Call a single shared copy of the (co)sine code in bank 0.
## `GetForestTreeFrame` works, but it's still bad
In [engine/tileset_anims.asm](/engine/tileset_anims.asm):
```asm
GetForestTreeFrame: ; fc54c
; Return 0 if a is even, or 2 if odd.
and a
jr z, .even
cp 1
jr z, .odd
cp 2
jr z, .even
cp 3
jr z, .odd
cp 4
jr z, .even
cp 5
jr z, .odd
cp 6
jr z, .even
.odd
ld a, 2
scf
ret
.even
xor a
ret
; fc56d
```
**Fix:**
```asm
GetForestTreeFrame: ; fc54c
; Return 0 if a is even, or 2 if odd.
and 1
add a
ret
; fc56d
```
Reference in New Issue View Git Blame Copy Permalink