search

TinyModularThings / UniqueEnchantments

Have some Unique Enchantments for yourself
https://www.curseforge.com/minecraft/mc-mods/unique-enchantments
Other
18 stars 16 forks source link

Avoid potential NullPointerException in LootingLevelEvent subscriber #30

Closed vemerion closed 3 years ago

vemerion commented 3 years ago

Due to global loot modifiers, there is no guarantee that that a damage source exists, and thus getDamageSource() has a chance to return null. This commit adds a check to prevent a NullPointerException in such cases. You can read more about the details in this Forge github issue.

Speiger commented 3 years ago

Thx for the fix @vemerion.

Yeah this is a forge bug. Also to give one note: You should not use the "killer" as reference if the event should be fired but if the "DamageSource" is not null because the damagesource is used not the "killer". You could have a case where the killer != null but damagesource is still null.

vemerion commented 3 years ago

Yes, you are right, even with my pull request to Forge, there is still a risk of the DamageSource being null. I would have preferred to do it another way, to guarantee that the DamageSource is never null, but as diesieben07 explained in the Forge issue, there might be situations where you want LootingLevelEvent to fire, even when a DamageSource does not exists. However, you would never want LootingLevelEvent to fire when the killer is null, so that is the compromise I made.

I am glad to have been able to be of help, even if it was just for a tiny fix 🙂

Speiger commented 3 years ago

Eh i know more situations where the Killer is null but the DamageSource isnt. Void Damage, FallDamage, FireDamage and a couple others. Thats why I suggested it that Killer should not be a reference because all these "Callbacks" that could be converted into features would be broken by this fix you are creating in forge. @vemerion

Anyways no problem from my side.

vemerion commented 3 years ago

Hm, you might be right. I guess it all depends on how you would want looting to work. Does it make sense to increase the looting level when an entity died without a killer? If so, then I agree that my fix is wrong. Does it make sense to increase the looting level when an entity died (with a killer) but without a DamageSource? If so, my fix is reasonable. I think we are going to have to wait and see what the reviewers say about my pull request, and work from there. Hopefully we can reach a solution that avoids all the null pointer shenanigans.