search

project-topaz / topaz

Server emulator for FFXI
Other
157 stars 222 forks source link

ROE: Fix crash on heal #1533

Closed zach2good closed 3 years ago

zach2good commented 3 years ago

I affirm:

Check that the entity is char before char-specific things are called on it, tripping debug breaks and other horrors.

Fixes: https://github.com/project-topaz/topaz/issues/1531

Kreidos commented 3 years ago

I'm afraid this won't correct whatever the root cause of the crash is. https://github.com/project-topaz/topaz/blob/36fc2f1b64c80b0b1d159fd142e72aa2dfd1eb55/src/map/lua/lua_baseentity.cpp#L6992 getEminenceProgress always returned nil for non-PC entities, as that's handled in Core.

Edit: I realize that since it wasn't a consistent crash on Canaria, it's reasonable to assume that since it didn't crash before or after this fix, it may have been a possible culprit.

zach2good commented 3 years ago

I only had a little time to check, but if I remember correctly the crash was on the debug break when a wyvern gets a healing tick.

If we have logic to handle it gracefully, we don't need the debug break in the first place. Pick one style or the other, I don't mind, but mixing both leads to different logic in release and debug builds: which is bad.

zach2good commented 3 years ago

I wouldn't call resolving a 100% reproducible crash a "non-fix" either. 👀

TeoTwawki commented 3 years ago

Are/will there be records that need to fire off on non-player entities such as pets, or does anything rely on receiving nil back from the function?

if no: debug break makes sense, since debug breaks exist to tell you during testing (in debug mode..for..debugging..) that you messed up and to check whats calling the function.

If yes, then removing the debug is preferable to inducing a forced crash telling you to check your stuff and hit that pushnil instead.

For now, this is exactly logical to fix the issue. a debug break said we called this on a non pc, we go to where the call is and check that its a pc or not 1st. no more debug break hit. We have tons of bindings that could exit rather then induce a break that do not. the old team thought this was good to force people to look what they called stuff on. I'd personally prefer more things simply not run rather than explode with exceptions to that based on how critical a functions execution is, but that is a can of worms for another day.

Kreidos commented 3 years ago

Perhaps I misunderstood that the crash was in-fact the debug-if being tripped.

There's no case where a non-player would have any eminence progress to get. This fix works fine in this case but ideally I envisioned getEminenceProgress being safe to call on any entity and you'd simply get nil which is easy to check.

In my mind I mistakenly assumed users would be running release, and in hind-sight I might have left the debug-break for PC out, since calling on a non-PC is technically safe.

TeoTwawki commented 3 years ago

users generally should run release. testing generally should happen in debug. :+1: