PaperMC
Fix option fixClimbingBypassCramming
This pull request fixes config option fixClimbingBypassingCrammingRule not working on Paper 1.21.7, the config value is never getting passed.
I directly edited the Level#getPushableEntities as the only usage is the one in LivingEntity#pushEntities.
It's passed here to LivingEntity#isPushable which as far as I can tell should be enough. Are you saying that how cramming itself works changed so that this isPushable check isn't responsible for the cramming damage anymore?
It's passed here to
LivingEntity#isPushablewhich as far as I can tell should be enough. Are you saying that how cramming itself works changed so that thisisPushablecheck isn't responsible for the cramming damage anymore?
The getPushableEntities is using EntitySelector.pushableBy, which is always passing a false to isCollidable
The isPushable check at head passed but getting entities ignored this, so the result entity list won't include climbing entities even fixClimbingBypassingCrammingRule is true
Right, looks like there is an unnecessary change there in the pushableBy method:
There should be no reason to even have a pushableBy(Entity, boolean) method with the ignoreClimbing parameter (as it is never set to anything but false) and it should work when simply using the same code as before the patch (at least for the push-check) which used isPushable (which then uses the config option) instead of the custom isCollidable which takes the parameter.
I personally think that's the cleaner approach as it actually reduces the diff instead of increasing it as your solution does.
Right, looks like there is an unnecessary change there in the
pushableBymethod:There should be no reason to even have a
pushableBy(Entity, boolean)method with theignoreClimbingparameter (as it is never set to anything butfalse) and it should work when simply using the same code as before the patch (at least for the push-check) which usedisPushable(which then uses the config option) instead of the customisCollidablewhich takes the parameter.I personally think that's the cleaner approach as it actually reduces the diff instead of increasing it as your solution does.
Yeah but, vanilla uses pushableBy entity selector, the pushable added by Paper is a redirection, which prevents diffs from flying everywhere. And some entities don't need this check like boats and minecarts. (see AbstractBoat and Old/NewMinecartBehavior)
I'm unsure what you mean by "prevents diffs from flying everywhere" seeing as you are increasing the diff which would be my understanding of "flying everyhwere".
Also Boats and Minecarts are not a LivingEntity hence why the climbing setting wouldn't even apply to them (it's defined in LivingEntity#isPushable after all)
Just to be a bit more clear, this is what I suggest (I haven't tested that though)
Yea that would make sense, i assume that should be included in another PR? Seems kinda off-topic for this one though( ´▽` )
Yea that would make sense, i assume that should be included in another PR? Seems kinda off-topic for this one though( ´▽` )
What do you mean? It's literally the solution for the issue this PR solves but simpler.
Yea that would make sense, i assume that should be included in another PR? Seems kinda off-topic for this one though( ´▽` )
What do you mean? It's literally the solution for the issue this PR solves but simpler.
Ah I see, what I thought before is to minimize diffs made by this PR, sry for my misunderstanding
That's how I meant it, yea. Just that there are now some left-over comments regarding the climbing option which shouldn't really be necessary as that feature does not touch this method anymore.
Done.