pret / pokecrystal

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

wWalkingIntoNPC and other label issues #1019

Closed vulcandth closed 1 year ago

vulcandth commented 1 year ago

I wonder if wWalkingIntoNPC deserves renaming as a result of this, but that'd be the subject of a different PR, since there's more variables following the same scheme.

Opening this issue based on a comment from @mid-kid in https://github.com/pret/pokecrystal/pull/1018#issuecomment-1308714223. Just wanted a place to take some notes and talk about some of these labels that seem incorrect before suggesting PR's.

The first thing I found just now is wWalkingIntoNPC the following routine: https://github.com/pret/pokecrystal/blob/41d5ea0482f4ef577df600d2d8b5cad70f74a396/engine/overworld/player_object.asm#L517-L535

The only other place where this label is referenced is: https://github.com/pret/pokecrystal/blob/f7d26fbe9c878c67271db67257073f6ccc34daf3/engine/overworld/player_movement.asm#L320-L339

This would presume that the TrainerWalkToPlayer: checks if wWalkingIntoNPC has a value of 1, we add step_end to the buffer. However, we know from .CheckNPC already that a value of 1 instead means that there is No NPC in front of the player.. It doesn't make sense. Thus, I conclude that wWalkingIntoNPC in TrainerWalkToPlayer: should actually be wSeenTrainerDistance from the previous wram union. This would make more sense here, as the routine would now be checking if the player is already standing next to the trainer (ex. the player talked to the trainer) or if the trainer needs to compute the distance to the player.

Now this still leaves the question of what the purpose of wWalkingIntoNPC is in DoPlayerMovement.TrySurf. It may just be an unused leftover.