Don't apply cramming damage to players #5903
Conversation
| fixInvulnerableEndCrystalExploit = getBoolean("unsupported-settings.fix-invulnerable-end-crystal-exploit", fixInvulnerableEndCrystalExploit); | ||
| } | ||
| + | ||
| + public boolean allowPlayerCrammingDamage = false; |
There was a problem hiding this comment.
| + public boolean allowPlayerCrammingDamage = false; | |
| + public boolean allowPlayerCrammingDamage = true; |
There was a problem hiding this comment.
Allowing cramming damage by default would defeat one of the main reasons for this fix: Saving unsuspecting server owners from killing their players and crashing the server...
And I have never seen cramming damage for players play a role in normal gameplay so this wouldn't really impact that.
There was a problem hiding this comment.
mmm, I'm not quite sure about this. I think it probably should default to the, well, default behavior. The setting is there if they want to change it.
There was a problem hiding this comment.
I'm unsure why you think that crashing people's server should be the default behaviour.
There was a problem hiding this comment.
There are already ways to crash the server. Repeating command blocks spawning thousands of items a second. That is also default behavior right?
EDIT: This isn't something that crashes servers that can be abused, a positive action taken by someone theoretically in control of the server in some capacity. If this was something that any player could abuse and cause a crash with, for sure, the default behavior should be changed to fix that.
There was a problem hiding this comment.
A change that can easily prevent an admin error and has near zero impact on Vanilla gameplay should be done imo. if it saves people from making this error.
Without this enabled by default this PR is essentially worthless because as pointed out by Billy: A plugin could've already fixed this issue before and I guarantee you that 99.99% of servers do not run such a thing nor are aware that players can take cramming damage at all until they have crashed their servers with dozens if not hundreds of players online. (I know that because I have experienced that, not with my server though, I already ran the fix at that point)
And imo. it's the software's job to help prevent such situation and use sensible defaults. Optional performance improvements with minimal gameplay impact are enabled by default too after all (e.g. EAR, hopper improvements, feature/structure search improvements), this isn't much different.
EDIT: Besides that: It's entirely possible for a player to achieve this: All they need is a very populated server and get tons of players to teleport to them in a 1x1 hole in a very short time. (E.g. by promising rare item drops)
There was a problem hiding this comment.
Just randomly glanced at this issue, and if I may pitch my two cents, this kind of fix seems to perfectly align with Paper's goals (as I understand them). The about section even states it "aims to fix gameplay and mechanics inconsistencies". I would strongly suggest that this is an inconsistency, and defaulting to false here is the expected behavior for most people. This likely impacts very few servers anyway, and if someone does not want this default for whatever reason, they can easily toggle it (again, probably little / no servers in practice).
There was a problem hiding this comment.
This has been debated before in other PRs - you can see Aikars comments on a similar topic of defaulting to change gameplay.
You're welcome to change it. Most people will prefer these on, and we would rather inconvenience an extreme minority rather than telling the majority to flip the configs.
I share the same opinion.
See #3222
|
Please also open a PR to document the new config option in https://github.com/PaperMC/PaperDocs |
|
I'm glad someone agrees with me about this ^_^ We've been doing this in Purpur for a long time now. I would have PR'ed it years ago, but I figured it would have been turned down for "plugins can handle that" ;) |
| @Override | ||
| public boolean isInvulnerableTo(DamageSource damageSource) { | ||
| - return super.isInvulnerableTo(damageSource) || this.isChangingDimension() || this.getAbilities().invulnerable && damageSource == DamageSource.WITHER; | ||
| + return super.isInvulnerableTo(damageSource) || this.isChangingDimension() || (!level.paperConfig.allowPlayerCrammingDamage && damageSource == DamageSource.CRAMMING) || this.getAbilities().invulnerable && damageSource == DamageSource.WITHER; |
There was a problem hiding this comment.
Missing //Paper? and the change we add should probably be on the end of there just for the sake of easy cut & paste if we hit a conflict here
There was a problem hiding this comment.
Yeah, I forgot about the comment thing, and moving it to the back makes sense I guess, I think I thought the getAbilities call was more expensive but that actually only returns one object.
92474e7 to
3889100
Compare
Reasonable arguments against changing the default given
|
I don't understand, this prevents cramming damage only if
If both, can you add an setting to allow cramming damage to player from mobs only? |
This prevents players from taking cramming damage of any kind (unless allow-player-cramming-damage is set to true of course) |
|
If you want more fine-grained control over this use a plugin, the PR was meant to prevent a pretty unknown behaviour in the server which is potentially exploitable. |
It does not make a lot of sense to damage players if they get crammed, especially as the usecase of teleporting lots of players to the same location isn't too uncommon and killing all those players isn't really what one would expect to happen. (And I have seen this happen before, it could even result in a server crash when the items of 100 players get dropped...)
For those who really want it a config option is provided.