fix: don't update kill statistics if player died of own account

[ujjman]

This PR is to fix issue #234

[DarkWeird]

Looks right.
event.investigator is NullRef when damage happens from falling.

[jdrueckert]

@ujjman I would argue that if the player doesn't have the PlayerStatisticsComponent yet and we want to update statistics, that instead of returning without doing anything, we should create the component 🤔 or am I overlooking anything here?

[ujjman]

I think we can update the deaths counter of the player but I think it would be unfair to do so because the player died by itself and not being killed by the other player. So what do you think?

[jdrueckert]

That the player wasn't killed by a person but on their own account is a good point, but it's still a death, so is it really unfair? 🤔 I would be interested in what @skaldarnar has to say about this as we talked about not punishing players, but only rewarding them...

[jdrueckert]

@ujjman Had a quick check offline with @skaldarnar and he agrees, that we should count it in the deaths statistic.

[jdrueckert]

Question 1: When is the player ID equal to 0 and why do we want to simply return in that case?
Question 2: What happens if a player a different ID dies on their own account but doesn't have a PlayerStatisticsComponent yet?

[ujjman]

let me give a brief explanation of what I have done. So, when the player dies by itself, beforeDestroy() calls updateStatistics().

The first time updateStatistics() is called with parameter event.instigator (which will be NullRef in this case as player dies by itself, returning with EntityRef of ID=0) and "kills". So, in this case we cannot update statistics.

The Second time when updateStatistics() is called with parameter player and "deaths", we get EntityRef of the player (initially having PlayerStatisticsComponent). So, in this case we update the player's deaths.

Answer 1: Player ID=0 when event.instigator is called and player dies by itself. We return in that case because we cannot update the statistics of NullRef (also no point of updating any stats here)

Answer 2: The player gets PlayerStatisticsComponent as soon as the player gets spawned on the fool's platform

[jdrueckert]

Ohh... so basically you're saying that we run into this NPE because in some cases updateStatistics is called with a NullRef instead of a player entity and obviously NullRef won't have the component we're trying to update?
Interesting find!

I'm wondering, whether we want to fix this here though 🤔 Ideally updateStatistics should always be called with a player entity and it also doesn't really know anything about the possible causes for being called with a NullRef (e.g. player dying without any other entity causing it). So I'd say we'd rather want to fix this in beforeDestroy and only call updateStatistics for "kills" if the event instigator of the BeforeDestroyEvent is not NullRef.

And while we're talking about this: For the spear I think this means that when the spear is thrown, we forgot who threw it by the time it hits and kills a player - hence the event instigator is NullRef and we run into the NPE 💡
With this PR fixing the NPE, I think we will see that killing a player by throwing a spear will increase their death count but not our kill count, so this is a separate bug we need to document (as a GitHub issue) and fix. Can you please create a ticket for this and (in a separate PR) investigate why we forget who threw it and how we can memorize that information?

[ujjman]

ok, will investigate on this issue 👍