search

nwesterhausen / valheim-discordconnector

A plugin to connect a Valheim server to a discord webhook.
https://discord-connector.valheim.games.nwest.one/
MIT License
27 stars 13 forks source link

Duplicate Death messages being sent #80

Open thedefside opened 3 years ago

thedefside commented 3 years ago

After fixing the Death Position toggle, I realized now that 2 message are being sent. Also, if you have the player firsts messages enabled, you will get 3 messages.

These are all from the same death: image

I see the issue is in the RPC_CharacterId.PostFix() method, where the if conditions need to be reworked. I can work on that, unless you would like to. Just let me know.

nwesterhausen commented 3 years ago

I always saw the first death message as a separate additional message, since it's supposed to happen only once anyway. So having it in addition to the normal death message was OK to me. Were you wanting it to somewhat combine them and say the first death message instead of the standard one in the single death message?

It might be easy to add a toggle saying "squash first-time messages into announcement messages" or something.

Definitely a bug that it sent the message for both regular and POS death.

thedefside commented 3 years ago

Yes, I assumed that it was not intentional being sent as a separate message. The way I saw it, if you had 

Message 1: Player has died.
Message 2: Player has died for the first time.

All of the information contained in message 1 is also contained in message 2, and so I saw that as redundant and changed it so that only the second message is sent.

In my opinion, that method already has too many nested ifs, and adding another toggle would further complicate things. But, it is your project :)

Let me know if you would like me to move the first messages back to their own separate if conditions, and I will update my PR.

nwesterhausen commented 3 years ago

I agree with your logic.

This should also be applied to all the other messages too then, right?

Ah just looked at the pull request again and scrolled down lol. You did do the logic changes for all the events.

thedefside commented 3 years ago

Well, I figured if I was going to do it, I better do it all 😁

nwesterhausen commented 3 years ago

I want to re-work this so that they can have FirstDeathMessage on but regular Deathmessage off.

I think it also makes sense to break out some of the inner logic sections to a static method to lessen code dupilication.