mojira.dev

Rachel Arevalo

Assigned

No issues.

Reported

MC-303181 Spears have an attribute modifier for attack_damage for both hands, even though the charge attack does not use it Confirmed MCPE-229475 Spears have maximum reach of 4 blocks. Unconfirmed MC-302984 Holding a spear in the off hand and attacking an entity adds the spear's attack damage to the attack Confirmed MC-302165 Bows/tridents/crossbows are able to be charged earlier than should be possible on a singleplayer world Community Consensus

Comments

Neither of these reports are attribute swapping, the Spear just provides its Attack Damage modifier to the off-hand…

Despite claimed fix, still happens in 25w42a.

To be more specific, the spear does check if a block is in the way of its attack, however it is between min reach and max reach. If the block is in front of the player but at a distance less than the min reach, it cannot be picked and thus fails. This should be easy to fix, make it pick blocks from the entity’s eye position to its max reach instead and cancel it if the block hit result is less than the min reach.

I should note, easier fix: just move the call for ChunkMap.tick() to the end of ServerLevel.tick(BooleanSupplier booleanSupplier). This is what I did in my PR for Paper, and it works flawlessly.

If I had to speculate, the performance impact was more substantial back when the game was older, so these checks were an optimisation made to account for that. my preferred fix would be bumping the base tick speed for the game up to either run entirely on delta time or to run at a higher tick rate since this also minimises tick jitter and makes the update intervals less important.

But I do agree, though it might have minor performance impacts this would in general be an improvement, but not necessarily a bug since it’s how the server code was intentionally designed and has always worked, 1.3 just made it impact singleplayer.

Oh, okay, I would like to note that the way entities are updated actually predates even 1.3 from the server side, 1.2.5 singleplayer is indeed much more responsive however that is because of no integrated server. This is from 1.2.5 server code using MCP mappings:

// EntityTrackerEntry.java
        if (this.updateCounter++ % this.field_9234_e == 0 || this.trackedEntity.isAirBorne)
        {
            int var2 = MathHelper.floor_double(this.trackedEntity.posX * 32.0D);
            int var3 = MathHelper.floor_double(this.trackedEntity.posY * 32.0D);
            int var4 = MathHelper.floor_double(this.trackedEntity.posZ * 32.0D);
            int var5 = MathHelper.floor_float(this.trackedEntity.rotationYaw * 256.0F / 360.0F);
            int var6 = MathHelper.floor_float(this.trackedEntity.rotationPitch * 256.0F / 360.0F);
            int var7 = var2 - this.encodedPosX;
            int var8 = var3 - this.encodedPosY;
            int var9 = var4 - this.encodedPosZ;
            Object var10 = null;
            boolean var11 = Math.abs(var7) >= 4 || Math.abs(var8) >= 4 || Math.abs(var9) >= 4;
            boolean var12 = Math.abs(var5 - this.encodedRotationYaw) >= 4 || Math.abs(var6 - this.encodedRotationPitch) >= 4;

            if (var7 >= -128 && var7 < 128 && var8 >= -128 && var8 < 128 && var9 >= -128 && var9 < 128 && this.field_28165_t <= 400)
            {
                if (var11 && var12)
                {
                    var10 = new Packet33RelEntityMoveLook(this.trackedEntity.entityId, (byte)var7, (byte)var8, (byte)var9, (byte)var5, (byte)var6);
                }
                else if (var11)
                {
                    var10 = new Packet31RelEntityMove(this.trackedEntity.entityId, (byte)var7, (byte)var8, (byte)var9);
                }
                else if (var12)
                {
                    var10 = new Packet32EntityLook(this.trackedEntity.entityId, (byte)var5, (byte)var6);
                }
            }
            else
            {
                this.field_28165_t = 0;
                this.trackedEntity.posX = (double)var2 / 32.0D;
                this.trackedEntity.posY = (double)var3 / 32.0D;
                this.trackedEntity.posZ = (double)var4 / 32.0D;
                var10 = new Packet34EntityTeleport(this.trackedEntity.entityId, var2, var3, var4, (byte)var5, (byte)var6);
            }

there is a slight difference in that the update counter is updated before the check rather than after however this should not result in a substantial gameplay difference.
This was not changed in 1.3, as can be seen here:

            if (this.ticks++ % this.updateFrequency == 0 || this.myEntity.isAirBorne)
            {
                int var2 = this.myEntity.myEntitySize.multiplyBy32AndRound(this.myEntity.posX);
                int var3 = MathHelper.floor_double(this.myEntity.posY * 32.0D);
                int var4 = this.myEntity.myEntitySize.multiplyBy32AndRound(this.myEntity.posZ);
                int var5 = MathHelper.floor_float(this.myEntity.rotationYaw * 256.0F / 360.0F);
                int var6 = MathHelper.floor_float(this.myEntity.rotationPitch * 256.0F / 360.0F);
                int var7 = var2 - this.lastScaledXPosition;
                int var8 = var3 - this.lastScaledYPosition;
                int var9 = var4 - this.lastScaledZPosition;
                Object var10 = null;
                boolean var11 = Math.abs(var7) >= 4 || Math.abs(var8) >= 4 || Math.abs(var9) >= 4;
                boolean var12 = Math.abs(var5 - this.lastYaw) >= 4 || Math.abs(var6 - this.lastPitch) >= 4;

                if (var7 >= -128 && var7 < 128 && var8 >= -128 && var8 < 128 && var9 >= -128 && var9 < 128 && this.ticksSinceLastForcedTeleport <= 400)
                {
                    if (var11 && var12)
                    {
                        var10 = new Packet33RelEntityMoveLook(this.myEntity.entityId, (byte)var7, (byte)var8, (byte)var9, (byte)var5, (byte)var6);
                    }
                    else if (var11)
                    {
                        var10 = new Packet31RelEntityMove(this.myEntity.entityId, (byte)var7, (byte)var8, (byte)var9);
                    }
                    else if (var12)
                    {
                        var10 = new Packet32EntityLook(this.myEntity.entityId, (byte)var5, (byte)var6);
                    }
                }
                else
                {
                    this.ticksSinceLastForcedTeleport = 0;
                    var10 = new Packet34EntityTeleport(this.myEntity.entityId, var2, var3, var4, (byte)var5, (byte)var6);
                }

Though by 1.8 they did add more conditions, oddly enough:

        if (this.updateCounter % this.updateFrequency == 0 || this.trackedEntity.isAirBorne || this.trackedEntity.getDataWatcher().hasObjectChanged())
        {
            if (this.trackedEntity.ridingEntity == null)
            {
                ++this.ticksSinceLastForcedTeleport;
                int k = MathHelper.floor_double(this.trackedEntity.posX * 32.0D);
                int j1 = MathHelper.floor_double(this.trackedEntity.posY * 32.0D);
                int k1 = MathHelper.floor_double(this.trackedEntity.posZ * 32.0D);
                int l1 = MathHelper.floor_float(this.trackedEntity.rotationYaw * 256.0F / 360.0F);
                int i2 = MathHelper.floor_float(this.trackedEntity.rotationPitch * 256.0F / 360.0F);
                int j2 = k - this.encodedPosX;
                int k2 = j1 - this.encodedPosY;
                int i = k1 - this.encodedPosZ;
                Packet packet1 = null;
                boolean flag = Math.abs(j2) >= 4 || Math.abs(k2) >= 4 || Math.abs(i) >= 4 || this.updateCounter % 60 == 0;
                boolean flag1 = Math.abs(l1 - this.encodedRotationYaw) >= 4 || Math.abs(i2 - this.encodedRotationPitch) >= 4;

                if (this.updateCounter > 0 || this.trackedEntity instanceof EntityArrow)
                {
                    if (j2 >= -128 && j2 < 128 && k2 >= -128 && k2 < 128 && i >= -128 && i < 128 && this.ticksSinceLastForcedTeleport <= 400 && !this.ridingEntity && this.onGround == this.trackedEntity.onGround)
                    {
                        if ((!flag || !flag1) && !(this.trackedEntity instanceof EntityArrow))
                        {
                            if (flag)
                            {
                                packet1 = new S14PacketEntity.S15PacketEntityRelMove(this.trackedEntity.getEntityId(), (byte)j2, (byte)k2, (byte)i, this.trackedEntity.onGround);
                            }
                            else if (flag1)
                            {
                                packet1 = new S14PacketEntity.S16PacketEntityLook(this.trackedEntity.getEntityId(), (byte)l1, (byte)i2, this.trackedEntity.onGround);
                            }
                        }
                        else
                        {
                            packet1 = new S14PacketEntity.S17PacketEntityLookMove(this.trackedEntity.getEntityId(), (byte)j2, (byte)k2, (byte)i, (byte)l1, (byte)i2, this.trackedEntity.onGround);
                        }
                    }
                    else
                    {
                        this.onGround = this.trackedEntity.onGround;
                        this.ticksSinceLastForcedTeleport = 0;
                        packet1 = new S18PacketEntityTeleport(this.trackedEntity.getEntityId(), k, j1, k1, (byte)l1, (byte)i2, this.trackedEntity.onGround);
                    }
                }

The differences between versions show that with only the fix to tracker update timings you should have the same core functionality if not more fluid in modern versions. Thus, this is more a suggestion than a bug.

So, few complaints: This is a duplicate of MC-297196 and also is incorrect.

Whilst your code analysis is fundamentally true, all behaviour mentioned in the code analysis already existed before 1.14 and was merely updated across versions. The actual changes in 1.14 responsible for this are to do with the entity trackers being moved to the ChunkMap class and thus happening before the entity ticks. The fix you have described treats the symptoms, but not the actual bug.

The order is in fact how it was done before 1.14, when this bug was introduced. Therefore, any other logical changes are how they were intended and byproducts of them moving it as well.

Update: When tick freeze is used, it appears the glitch is no longer noticeable, I believe due to how frozen ticks work.

With this fix, if you were to do tick freeze, hit a mob, then tick step, it would, as expected, immediately be seen on the client. In addition, it is sometimes possible, in this bug, to have 1-3 ticks of delay, as, for most mobs, they update every 3 ticks unless hasImpulse is set to true, so if you were a tick before an update, it would be delayed by one tick, but if an update would have happened on that tick regardless of hasImpulse, it becomes a whole 3 ticks of delay.

I confirm this bug. The issue is caused in ChunkMap.tick() where ServerEntity.sendChanges() is called for all tracked entities in the chunk before the entity ticks, this is an issue because in the case of a player attack, it, with other packets, is handled at the end of a tick, thus gets processed right before on the next tick it is sent to clients, before the entity is moved on the server. I have also confirmed the possibility of fixing this, reducing the delay in movement to 0 ticks, by moving the logic from ChunkMap.tick() to after ServerLevel.guardEntityTick(Consumer<Entity> consumer, Entity entity) is called in ServerLevel.tick(BooleanSupplier booleanSupplier) (Using the entity that just got ticked, rather than doing all tracked entities at once).

For some reason, I can’t attach the videos.