Stripping Down EntityDamageEvent - DamageModifier

Discussion in 'API Discussion' started by md_5, Nov 19, 2016.

?

Remove DamageModifier API?

  1. Yes

  2. No

Results are only viewable after voting.
  1. md_5

    Administrator Developer

    In the final months of Bukkit 1.7, there was an API added to EntityDamageEvent that allowed plugins to get the individual components of damage from this event. For example a plugin using this event could see how much damage was blocked by armor, how much was blocked by potion, how much was blocked by blocking etc etc.

    Unfortunately in order to implement this API a large amount of the damage code within the server had to be changed. This has time and time again caused difficulties for us when updating to new Minecraft versions, and also still causes inconsistencies between how CraftBukkit handles damage and how Vanilla does. In particular shields and events with 0 damage such as snowballs and golden apples continue to cause issues. Furthermore this code is so complicated that fixing one bug almost inevitably just causes two more.

    We are not aware of how much this API is used (and I can't think of much of a use case), and given that it was only added fairly recently - I'm proposing that we remove it from 1.12 to finally put an end to all these damage inconsistencies.

    Examples of stuff which gets fixed and then broken in an endless cycle include: snowballs, shields,
     
    #1 md_5, Nov 19, 2016
    Last edited: Nov 19, 2016
    • Agree Agree x 6
    • Creative Creative x 1
  2. One use would be making a magic-resist armor (check if damage == magic, then reduce it). But remove it if its broken and impossible to fix :p
     
    • Like Like x 1
  3. Choco

    Moderator

    I assume this is in regards to the EntityDamageEvent#DamageModifier API? I can see why this would be useful, but I've never found any use for it. I really see no purpose for this other than what @lookcook mentioned. Is there really any need to be specified about what amount of damage was caused by what? If you want to know what the increase of damage was total, using #getFinalDamage() - #getDamage() would suffice. Again, I can see why this was added in the first place, but this seems like a horrible implementation for it. Perhaps there's a way to make this better.

    I think it should, yes, be removed, but immediately replaced with something else that could be more flexible
     
    #3 Choco, Nov 19, 2016
    Last edited: Nov 19, 2016
    • Agree Agree x 8
  4. Then you could check if the damage cause is magic (I believe there is something like that) and then reduce the damage ;3

    I believe I once used it for a plugin request, but I can't quite member what it was. I think the feature should still be removed though, as it would reduce your workload and evades more bugs and inconsistencies
     
  5. I've messed around with this API before, didn't find any good use case for it.
    It seems like an over-complicated solution to a problem noone is having.

    I support removing it.
     
  6. I use it for custom mobs. When a player is attacked I look at the mob name, then look in an enum for the damage that mob does and change the damage to that value. (I'm open to any other ways of doing it, even if it involves NBT tags)
     
  7. md_5

    Administrator Developer

    I'm failing to see what your description has to do with what I stated.
     
  8. I thought you were talking about anything related to EntityDamageEvent#getDamage(), setDamage(), Player#getLastDamageCause(), Player#setLastDamageCause(), Player#getLastDamage() and Player#setLastDamage(). If those methods stay I'm happy!
     
    • Agree Agree x 2
  9. It is a great API, take my word for that, but there is just about nobody that actually uses it for production.
    If anything, we'll just mess about with NMS to set the damage permanently.

    Supporting to remove.
     
  10. Anything that requires "just messing with NMS" should be implemented in the API/an external library...

    Anyways as long as you can simply override the total damage value I don't see a problem.
     
  11. md_5

    Administrator Developer

    ITT no one actually bothering to look at what I'm talking about.
     
    • Like Like x 1
    • Agree Agree x 1
  12. It's as simple as: how does it even get used?
    Like, nobody uses it. Nobody knows what to use it for. Nobody knows how to use it.
    I don't see the point in having it at all, personally. If it just gets broken, remove it.
    We'll find alternatives ourselves.
     
    • Agree Agree x 1
  13. I Agree. I have never used this in any of my work.
     
    • Agree Agree x 1
  14. I think most of us know exactly what you're talking about. It makes sense for the data to be in the event. The data should be in the event! But because not a lot of people are actually using it, the time spent maintaining this thing can be focused on something more important.

    I don't think accessing the data will be easy without the EntityDamageEvent.DamageModifier, but I'm sure people can manage without it.

    It's up to the development team to weigh the cost of maintaining this part, not us.
     
  15. I used it in the past to remove all damage reduction effects from armor. The armor is than used for the visual appearance of the player only.

    And in one minigame plugin I let the server admin configure what damage modifiers are applied. I have no data though how much this is used.

    So for those uses cases it would be sufficient to be able to change the values for the different DamageModifiers to 0.0D. Not sure if that would make the implementation any simpler.
     
    #15 blablubbabc, Nov 21, 2016
    Last edited: Nov 21, 2016
  16. If it is removed what are we going to use instead? If we need to do something when something is damage.
     
    • Optimistic Optimistic x 1
  17. Choco

    Moderator

    The event is not getting removed. The "DamageCause" feature of the event is potentially being removed from the event
     
    • Informative Informative x 1
  18. Thx
     
  19. But speaking of NMS is there not a way spigot can add a more simple API for it? @Dablakbandit did this with his huge API but it would be even cooler if spigot did this, it would save so much time and it would be neat to make NMS custom entity's with pathfinding and more with an actual API.
     
  20. md_5

    Administrator Developer

    Your question is basically "Why isn't all of Minecraft in Spigot API".
    1) Because we have to maintain it, 2) No one has added it, 3) When people try to add API they often do a terrible job.
    You're welcome to check README.md for information about how to add API to Spigot.
     
    • Informative Informative x 1