creature: initialise room number for target enemy #144
Conversation
|
Thank you for working on this PR @lahm86. I've not experienced any crashes while testing this build. I did notice an issue with enemies being able to target and shoot Lara/monks through walls. Saves: |
Thank you. Back to the drawing board 😄 |
lahm86
changed the title
lahm86
force-pushed
the
issue-137-target-enemy-room
branch
2 times, most recently
from
cf8eee8 to
897e7dc
Compare
|
The issue with enemies shooting through walls is now resolved (thank you @rr-), this was a problem only in the release build which I had sent. Would you mind checking over it again @aredfan? So this is the same fix as before for the original crashing problem, plus the shooting issue resolved as well. |
lahm86
changed the title
| @@ -913,6 +913,7 @@ int32_t __cdecl Creature_CanTargetEnemy( | |||
| target.pos.x = enemy->pos.x; | |||
| target.pos.y = enemy->pos.y - STEP_L * 3; | |||
| target.pos.z = enemy->pos.z; | |||
| target.room_num = enemy->room_num; | |||
There was a problem hiding this comment.
I'm a bit torn here; can we use the previous fix where we always set the target room number, and on top of it, have every caller set the room number as well?
This way we make both the callers and the LOS_Check API behave nicely – the latter will make sure the room number is always set to a valid value, and the former will always provide a nice input to the function. Ideally we could use this same approach in TR1X.
(I'm open to discussion about this.)
There was a problem hiding this comment.
Another idea is to alter the LOS internals to use XYZ_32 for target, and have LOS_Check set the target->room_num to Room_FindByPos result.
There was a problem hiding this comment.
Yes, I'm happy to put the check back into LOS_Check, then have each caller explicitly set it. It feels right for sure.
I think we may need to have Gun_FindTargetPoint decompiled as well first though, as @walkawayy mentioned:
#137 (comment)
There was a problem hiding this comment.
Thanks! Looks like Gun_FindTargetPoint does set target->room_num = item->room_num;, so that situation should be good.
There was a problem hiding this comment.
Pushed a change to have all calls set the room number first, and LOS_Check also does a check for identical x/z. I can squash when the time comes as the title/commits have changed a bit already for this one.
The other main references to the function are in camera.c but everything here checks out already as far as I can tell.
Of course, no problem. I did 30 minutes of tests with the newer build and there's been no crashes, or any other odd issues. I'll be happy to do more tests tomorrow if the code changes. 👍 |
This avoids cases with enemies fighting one another where x and z positions are identical and so LOS_Check does not initialise the target room number and hence later passes an invalid room number to Room_GetSector.
lahm86
force-pushed
the
issue-137-target-enemy-room
branch
from
897e7dc to
da6ad4a
Compare
There was a problem hiding this comment.
LGTM. I didn't see any regressions or crashes. I killed a monk and kept a locked on target on his dead body across the level to make sure shooting through rooms / target lock remained. Did we figure out if this and the issue should be tagged OG bug or TR2X bug?
Resolves #137.
This avoids cases with enemies fighting one another where x and z positions are identical and so
LOS_Checkdoes not initialise the target room number, and hence later passes an invalid room number toRoom_GetSector.I got the trace to show it was stemming from the call in
Creature_CanTargetEnemy. I realise the room number isn't being set elsewhere for other LOS check targets, but felt perhaps this should be kept in its own commit for now while we continue testing the rest.I tested the Barkhang scenario for an hour (i.e. loading a save, waiting for monk/merc fights to play out, then reloading) and it was crash-free. @aredfan, perhaps you could also check it out if you have time?
TR2X.zip