Ensure shulker bounding box is updated

[lynxplay]

A previous paper patch introduced bounding box reuse for entities on
position updates if the x,y and z coordinate did not change.
While this reusing in itself works fine, shulkers do update their
bounding box without actually changing their x,y and z coordinates which
is not respected due to the old bounding box being reused.

This commit fixes this issue by manually updating the shulker bounding
box after reapplying the position.

Another alternative solution would have been an exception specifically
made for shulkers to ignore the reuse logic, tho this would introduce a
new instanceOf check into the Entity#setPosRaw method and would be
evaulated for every type of entity.

See also: https://github.com/PaperMC/Paper/blob/master/patches/server/0467-Ensure-Entity-AABB-s-are-never-invalid.patch
Resolves: #5915

[lynxplay]

Actually does not fix the portal issue, only the pressure plate interaction.

It does, I am just too stupid to properly test.

[Machine-Maker]

So, the reason I'm not sure about this fix, is that the bounding box has its own method on Entity that's overriden only by Shulker but could be overriden in the future by other entities, or by custom ones. So the way the AABB is being created in this patch isn't correct, which is why the Shulker needs this special fix.

[lynxplay]

Yes the call against the entity dimension is also wrong, yet never called for a shulkee as the shulkers setRawPos calls during the peek will not supply different X,y and z values anyway hence the incorrect call to the entity dimension for the bounding box is never made for the shulker.

Do you think it makes sense to also switch the call in setRawPos to the entity method for building the bounding box?

[Machine-Maker]

Yeah, perhaps. But that’s not as simple as it sounds, because the makeBoundingBox method uses the position field which hasn’t been updated yet. I need to understand more about what this patch is trying to prevent, or how it would get messed without this patch.

[Prof-Bloodstone]

@Machine-Maker Not sure if helpful, but 2 related issues to that patch:
This one was created reporting a bug this was fixing: #3427
This one with some Aikar explanation: #3401

[lynxplay]

Looking further through to patch, it seems like gist of it lies in the bounding box update prior to calling respective movement callbacks in the level.
As you already stated, makeBoundingBox does use the current position rather than respecting potential custom position arguments.

One "valid" solution might be moving the aikar logic after the update of the position but prior to any further level callbacks.
I personally do not see a rather, why this was implemented prior to the position modification anyway, as it also entails a repetition of the position difference check.

By moving the

if (!(this instanceof net.minecraft.world.entity.decoration.HangingEntity) && (this.position.x != x || this.position.y != y || this.position.z != z)) {
    this.setBoundingBox(this.dimensions.makeBoundingBox(x, y, z));
}

inside the if block following, we could safely use the Entity#makeBoundingBox method after both position and blockPosition were updated.
This would also remove the current doubling of the oldX != x.... check.

---

This does however not solve the issue of shulkers not updating their bounding box, as their location does not change and hence the prior bounding box would still be reused.

[Machine-Maker]

Ok, so the patch that caused the issue reported here doesn't seem to even be in the current version anyways? So I'm not 100% sure the "Ensure AABBs aren't invalid" patch, which is causing the issue, is even needed anymore.

[lynxplay]

That would imply we could drop the patches related to this fix and move the bounding box update outside of the setPosRaw method in the setPos method. Tho, with the exception of the Schulker, reusing the bounding box for entities that haven't moved but still reapply their position seems like a neat optimization.

[lynxplay]

Well looking deeper into this, we are now at a pretty straight forward decision.

1. Either we revert to the prior setup before both the desync BB patch and the (already removed?) sane BB patch, which would put the this.setBoundingBox(this.makeBoundingBox()); right back under the Entity#setPosRaw call inside Entity#setPos. This would fix the shulker inaccuracy, yet we would loose the caching of bounding box instances for entities that have not moved in their move tick.

2. We stick to the current state of this PR, keeping the BB cache for entities that haven't moved while hence requiring the manual update for entities that change their bounding box without moving (which afaik seems to be unique to the shulker). This solution would obviously break the vanilla setup and potentially require futher manual BB updates for entities in the future.

I don't really have a preference here, so input would be much appreciated 😃

[Machine-Maker]

The problem with this, is that it's using the old position in the makeBoundingBox method, because the position hasn't been updated yet in setPosRaw

[lynxplay]

The current implementation does not ? The call to setBoundingBox is executed after both position and blockPosition are updated. (Idk if you were talking about this tho 😅)

[Machine-Maker]

Ok, yes I see that now. But that effectively just undoes what the patch that moved it did in the first place. Part of the first patch was moving the bounding box set to before the position set, but this goes back to, do we even need the initial change that causes all this in the first place.

[lynxplay]

Yeah, as you stated I don't think we really need that patch anymore (as you pointed out, the bounding box modification patch is no longer in place). This way, we would simply keep the "reuse" of non-changing bounding boxes for entities as stated in my prior comment. If we want to use it (and force ourselves to manually update bounding boxes that change without an entities location changing) is on the maintainers really.

[Machine-Maker]

Unless someone thinks the patch that re-orders the bounding box update and the position update is still needed, I say we drop that change all together.

[Machine-Maker]

Ok, so talked some internally, patch should be kept because setPosRaw is called much more frequently; the bounding box should be updated in that method to ensure no desync, but the bounding box can be created after the position is set. So I’d just have it call makeBoundingBox after the position field is set, and always call that if it’s a shulker regardless of position being changed. So no need to manually call it in the Shulker class.

[lynxplay]

Okay, yeah I guess that is definitely a valid way of implementing it, tho it does put the instanceOf check onto every single entities setPosRaw. Should be fast enough tho to not really make a difference. I will update the PR accordingly 👍

//EDIT.0
After implementing it, I am not sure the instanceOf check on the entity level is the best implementation for this especially if we want to ensure that the bounding box is updated before the callbacks to the level callback onMove function (which is introduced by the aikar patch but does not seem to be required in general as vanilla used to call the bounding box update after the entire setPosRaw method finished).
Another disadvantage of the instanceOf implementation is the constant update of the bounding box of each shulker even if they are not peeking.