Clean up entity damage handling #2040
Conversation
|
Re: flat removal of unreachable Firework handling: Spigot has been assigning fireworks' shooter correctly since somewhere in 1.15-ish I believe. Firework testing
|
|
Would it be possible to block the ender's crystal damage inside the protection? |
I'm not looking to change features or fix bugs within this PR, I'm trying to make it less of a nightmare to work with. If you think GP is missing a case, please open an issue instead, I'll probably forget otherwise. In general end crystal explosions originate inside the claim and as a result should damage things also inside the claim. The only source of damage to end crystals we have an issue tracking is specifically for TNT minecarts in #540, are there other sources? |
|
If the player is inside the protection and someone breaks an ender crystal outside the protection but within the explosion range the player takes damage, I will test it further and open an issue. |
|
@RoboMWM if there's anything you'd like me to do differently with this (move helpers to separate pulls or something if possible, Idk) please let me know. I'm doing this the way that I am because I think the other options will be just as bad to review and even slower to process, but if there's something that works better for you I'm all ears. |
|
Re: 4fe0098 - I specifically set out to not fix bugs here, so I've moved comments and TODO to the appropriate locations to actually enable damage to villagers from the correct sources. I think this is approaching readiness. I'm still torn about creating a claim check helper, but for some cases it might aid with consistency, i.e. attacker last claim is inconsistently set to defender entity claim for non-pvp attacks. May be good to just always set it as expected. |
|
I was concerned about replacing the Running benchmarks in JMH yields weird results for the invocation and equality operator, i.e. As far as the rest goes, the loss is probably negligible due to call frequency (or lack thereof - not a super heavy load method) in the event of future weird NMS implementations that claim to be an EntityType but don't extend the corresponding interface. |
|
Okay, there are definitely more changes I'd like to include in the other listeners, but this already is truly disgustingly large (57 commits and +/-920ish lines is offensive) so I've marked it as ready. Note that I believe the line endings of the PlayerEventHandler are correct now, I think it still had Unix line endings when we specify Windows. IntelliJ does not allow mixed line endings by design, so any edit either must use existing line endings or convert all of them. I had thought it was my git installation messing up, but nope, "feature" of the IDE. Wonderful. @RoboMWM, if you'd prefer I can split this up into several PRs:
If you don't mind it as-is but don't want the whitespace changes I can convert the one file to Unix line endings, but we probably should renormalize again at some point - I know I hit the same issue in a couple previous PRs. |
|
tbh, since you're consistently active here, I don't mind it all being in one PR, since I know you'll be looking to fix things that arise. It's when I get big PRs from people who aren't so reliably active and also change behavior too (like the two I pulled for wilderness-tp) that I'm never ever gonna do again. |
|
One way or another this stuff does need to be cleaned up, v20 break or not. Idea would be to break this into modules or something to separate out regular block/non-player entity protection vs pvp protection handling and such for v20 at least. So I guess in your last comment's question, I presume that means going with 1? |
Ah, that was a 3 step process. Basically, 9 PRs in total that would ideally be more digestable: the initial split, then one per major code segment broken out into a helper method. If you're good with it as-is, all of the damage/combat-type handlers and helpers are in the EntityCombatHandler as of 17588a7
This is less modularization and more of a prep/cleanup that would allow readable edits to logic here. All of the PVP-specific state handling should (barring the one handler on Monitor that is exclusively for PVP) now be inside |
|
ah, re-read. In that case, option 3 is probably best. Or just put it in here. Significant refactor stuff would take anyone a long time to review - if anything, probably would be best to pair on such - but I trust your revisions since you have history of remaining active and fixing anything that's discovered along the way. |
Fireworks have become real projectiles and now actually have a shooter.
|
Okay, working on squashing into a more readable form. I will be including bugfixes too, but I will document them as I come across them, and they will receive their own commits so they can be more easily dropped if undesirable. First up is #538 - GP did its best job to prevent broken vanilla behavior, but vanilla has since fixed that broken behavior. GP's fix silently disables firework PVP damage when its own PVP system is not enabled. If vanilla's anti-PVP is capable of handling something (which it is - I verified that disabling PVP in server.properties does prevent firework damage on 1.19.4), GP does not need to handle it. I had accidentally included this in the previous cleanup attempt and only realized that it was a behavioral change when I went to squash it all. |
Switched to instanceof with pattern variables instead of educated guess casting. Previously we've run into issues with custom pet plugins creating NMS-based entities that return an "incorrect" EntityType for the Bukkit interfaces they implement. The instanceof calls are marginally slower, but are much safer for the subsequent checks.
Use quick returns, pattern variable, and new switch for readability
Pattern variables, remove redundant check
Remove redundant PVP check, add quick returns
Use quick returns, extract duplicate code into a helper method
Use quick returns, pattern variables, previously extracted helper method for duplicate code
Use quick returns Remove unreachable code Note potential issues/missed cases
Use quick returns Remove unreachable villager handling Make last claim caching more consistent
Use quick returns Previous patch to this area for display names of owners cleaned up most of the smells that were in the original cleanup PR
Use quick returns, pattern variables Listen to more specific event (no need to duplicate event system's handling) Don't fetch player data for self-inflicted damage
Use existing local instances instead of static access - GriefPrevention is still a god object, but this is a step towards testability Annotate handlers Clean up a few comments
Also don't load pet owner name from disk if not sending messages. Theoretically doesn't cause issues because player-induced ignition should not occur if original damage was cancelled, but is an error.
Use quick returns and pattern variables, remove unnecessary guards
This does not fully address potion splashing issues (namely, it still follows a completely different rule set from regular damage) but it is progress. I'm also taking this opportunity to correct effected -> affected, because as a grammar nerd that has bothered me for ages. Add some missing positive and negative effects - only slow fall is a vanilla-obtainable effect, but supporting other plugins is never a bad thing. Use pattern variables Reduce duplicate code with claim PVP handler
Jikoo
force-pushed
the
dev/damage-cleanup-v2
branch
from
ee8a68c to
8641abe
Compare
More cases were previously "allowed" in an unreachable segment of code, but even that was missing several cases.
Causing projectiles to ignore and pass through entities isn't easy to do "correctly" via the API without potentially becoming subtly incompatible with other plugins due to team manipulation.
|
Following the force push, the partial squashing is caught up with where I was in the previous iteration. Missing raider types doesn't have an issue attached to it, but projectiles spamming shooter is #850 |
This is (technically) a behavioral change. Pets are prevented from engaging in PVP attacks if the defender is PVP immune. Theoretically this state is unable to occur in vanilla, but plugins could add additional triggers for pets attacking. Pets are no longer checked with PVP rules if PVP rules are not enabled. The assumption is that either vanilla PVP protection is in place or another PVP plugin is in place. - In vanilla, pets will not engage in PVP unprompted. Disabling PVP disables other players causing pets to attack, so an attack must be retaliatory. - If another PVP plugin is in place, it should handle all of PVP.
|
The only feasible way to know if this works well is to test all the stuff you've touched at the very least, which would be quite an effort. Since there seems to be interest in beta releases, I might go ahead and just pull and release anyways. Do you think it's ready for release? |
This will ideally make the code easier to follow and easier to move to modularize later.
All messages are error messages, and the documentation exists.
|
Had a bit more stuff to push out locally to resolve some notes. I think this should be release-quality, yes. The vast majority of it is tool-modified existing code. |
|
Ok, imma pull this and #1218, and make a beta release. Will create a discussion for that release. |
The entity damage handling is a huge mess. It's a behemoth of logic containing a ton of outdated checks and redundancies.
Going to open this as a draft and update as I go. The majority of this will be reorganization, but certain things like the nonfunctional Firework handling (Firework extends Projectile, so it can't be hit) I'll need to test more thoroughly.
When the time does come to review, I recommend checking individual commits, potentially ignoring whitespace diffs. I expect that as this is a major chunk of housekeeping it will be very hard to follow otherwise.