pret / pokecrystal

Disassembly of Pokémon Crystal
https://pret.github.io/pokecrystal/
2.06k stars 770 forks source link

Bug: CheckObjectCoveredByTextbox doesn't account for the background scroll (SCX/SCY) #1093

Open xCrystal opened 9 months ago

xCrystal commented 9 months ago

(https://github.com/pret/pokecrystal/blob/38667169809b81eb39990b4341f9919332d27248/engine/overworld/map_objects.asm)

CheckObjectCoveredByTextbox uses OBJECT_SPRITE_X and OBJECT_SPRITE_Y to get the object's pixel coordinates. These variables include the contribution of the background scroll as well. Coord2Tile is later called to get the tile where the object is at from the tilemap, but the coordinates passed to it are offset by the scroll value in each axis. Furthermore, when the scroll is past a certain threshold (around 0xA0, depends on which of the objects of the sprite is currently being processed), the checks in line 2409 and 2416 will kick due to previous calculations and pass the object as not covered by textbox.

The thing is, I don't think there is any instance in the game where there is a textbox and the screen hasn't just been anchored, so in practice there is likely no preceivable bug. And if there is no textbox at all in the screen, no matter which random tile you erroneously look at you will never hit a textbox tile. In fact, the aforementioned checks in L2409/L2416 work as patches to avoid Coord2Tile looking beyond wTileMap with this bug in place.

Anyway, this is what I came up with:

ScrollAwareCoord2Tile::
; Return the address of wTilemap(c, b) in hl relative to the background scroll in hSCX and hSCY.
; c = c - [hSCX] / 8
    ld a, [hSCX]
    srl a
    srl a
    srl a
    ld l, a
    ld a, c
    sub l
    jr nc, .ok1
    add BG_MAP_WIDTH
.ok1
    ld c, a
; b = b - [hSCY] / 8
    ld a, [hSCY]
    srl a
    srl a
    srl a
    ld l, a
    ld a, b
    sub l
    jr nc, .ok2
    add BG_MAP_HEIGHT
.ok2
    ld b, a
    jr Coord2Tile

As well as removing CheckObjectCoveredByTextbox lines 2409, 2410, 2416, and 2417, and replacing the Coord2Tile call for ScrollAwareCoord2Tile (which could be in the same bank as CheckObjectCoveredByTextbox rather than in bank0).

If you delve, it's possible that you can actually rewrite and even simplify the function by extracting the scroll at the beginning and not accounting for it anywhere, but I'm not sure and I'm tired :)