harbingerofme / DebugToolkit

Debugging commands for Risk of Rain 2. Previously known as RoR2Cheats.
https://thunderstore.io/package/IHarbHD/DebugToolkit/
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

More robust error logging #133

Closed SChinchi closed 1 year ago

SChinchi commented 1 year ago

Also gathered all the command help texts in Lang.cs for convenience.

SChinchi commented 1 year ago

On a side note, I'm thinking that when you die your body becomes null and this causes the NPE. I need to do some debugging because this had me stumped.

xiaoxiao921 commented 1 year ago

Yeah I can't verify right now but maybe that's an additional check we could do

SChinchi commented 1 year ago

It does turn out to be the body being null while dead.

This has far-reaching consequences for some commands, such as, god. If we just have a check to skip dead players, then in multiplayer it can cause god mode to be out of sync for everyone. So either we apply some hook to flip the god mode on dead players when they respawn, or we make sure the command only has an effect when everyone on the team is alive. This is a behavioral change and this PR was only intended to check for and log errors. So I would rather leave this bug in and sort it out in another PR.

But for commands like give_item, where accessing the inventory of a dead body will lead to an NPE, it would be in the spirit of this PR to just log a "body dead" message and end prematurely.

xiaoxiao921 commented 1 year ago

Depending on where the nre happens, this can be gracefully handled by just checking if args.sender is dead or not, no?

SChinchi commented 1 year ago

In some cases, yes. But a few commands allow to specific another target, so a dead player/DS can target another target who may be alive or not. But this can also be easily checked for if the target argument is provided after it has been parsed.

SChinchi commented 1 year ago

I had a look at MessageNetworked and it seems to check for the sender being a local player before calling Message internally, so I didn't revert those modifications. Could you please verify whether my understanding is correct?

I fixed the dead body bug on most commands. However, I didn't touch god, midgame, and lategame, which will require some modifications if the effect is supposed to apply to everyone at the same time or not at all. If this is something we aren't going to address in this PR and there aren't any other outstanding issues, consider this complete from my side.

SChinchi commented 1 year ago

I don't know what autocomplete voodoo I unleashed with the second to last commit, but I seemed to have changed all args[i].ToUpper() == Lang.PINGED to not convert to uppercase, so I reverted that. In the meantime, I had a look at the source code and I realised using the CharacterMaster instead of CharacterBody addresses any previous issues as the former is not null on character death. I added that in with the current commit because it was a minimal change.