mojira.dev

Z0rty

Assigned

No issues.

Comments

Sorry Rachel, looks like my fault. I wasn’t reading carefully (mojira shows oldest comments and hides newer ones) and thought you were implying the necessary changes had to be made in ChunkMap::tick instead of the ServerLevel class. I’ll make sure to be more careful next time :upside_down_smile:

Yes, MC-297196 definitely explains some of the observed server behaviour and I have left a comment on there as well.
Also, instead of the tickOffset of the PositionStep being set as the number of ticks since the last update was sent, it should rather be a prediction of when the next packet is going to be sent (with possibly 1 extra tick added). So e.g. if updateInterval = 3 and we’re currently on tick 1, we’d be sending a tickOffset of 2 + 1 = 3.

Rachel Arevalo seems to have already pointed out the main issue with a slight inaccuracy (maybe due to outdated/obfuscated code). The root of the problem is that the server sends updated positions to players before actually moving them, causing a tick of unnecessary delay. This is due to this.getChunkSource().tick(…) being called before this.tickNonPassenger(…) in ServerLevel::tick.
If the server receives an attack packet, it adds some velocity to the attacked entity and sets the entities needsSync flag to true. The server then distributes the old position on the next tick and won’t trigger again for possibly up to 3 ticks.

can confirm, experiencing this as well in one particular world.

[media]

To further clarify on what I mean when I say that entities can be stuck interpolating forever. Imagine a scenario where remainingTicks = 0 and currentStepTicks = 2.99995 < 3 or similar due to floating point precision. If remainingSteps contains a single step of tickOffset 3, then this will cause the entity to try interpolating on every tick when standing completely still. I noticed something like that happening when testing a few things but cannot reproduce it consistently right now.

On the server side the undesired behaviour is due to code found in ServerEntity::sendChanges and how it interacts with SteppedInterpolationTracker. It calls SteppedInterpolationTracker::getPositionPath to get the PositionPath which is then sent to the player, including the tickOffset. The problem is due to how the steps are added in SteppedInterpolationTracker::addStep;

private void addStep(final Vec3 pos) {
      if (this.ticksSinceLastStep > 0) {
         this.trackedSteps.add(new PositionStep(pos, this.ticksSinceLastStep));
         this.ticksSinceLastStep = 0;
      }
   }

Occasionally this produces very small steps of tickOffset 1-2, causing very visibile stutters in the entities interpolated movement

There are several problems with the current version of the stepped interpolation and using updateInterval() + 1 instead of updateInterval() is not going to fix the most notable stutters. Please make sure to first quickly read my report MC-311976 on this (mostly same) issue.

The first issue is that the vanilla server seemingly already sends move entity packets using the new PositionPath.Stepped packet protocol. This is especially noticeable when hitting an entity currently in rest, as the server will sometimes sent stepped move entity packets with a tickOffset of only 1 or 2, making any changes on the client useless in this regard as they overwrite the interpolationSteps value.

Another issue i mention is that entities can get stuck interpolating on every client tick even though they are not being moved at all.

For a complete fix one must change the interpolationSteps value to updateInterval() + 1 as proposed, must stop the server from sending the move entity packets with PositionSteps of small tickOffset and adjust the following methods in a way similar like I did (I do not guarantee that these are completely free of bugs, but at least entities move smoothly °_°),

In SteppedInterpolationHandler:

protected void doInterpolate() {
		float tick = this.entity.level().getRelativeTickSpeed();
		this.interpolationData.advance(tick, this.interpolationSteps);
		PositionAndRotation target = this.interpolationData.getNewPositionAndRotation();
		this.entity.setPos(target.position());
		this.entity.setRot(target.yRot(), target.xRot());
	}


and in SteppedInterpolationHandler.InterpolationData:

private void setStartingPoint(Vec3 position, float yRot, float xRot) {
        this.lastStepPosRot.set(position, yRot, xRot);
        this.currentStepTicks = 0.0F;
    }
private void advance(float ticks, final int interpolationSteps) {
        float targetSpeed = Math.max(this.remainingTicks / interpolationSteps, 1.0F);
        this.interpolationSpeed = Mth.lerp(1.0F / interpolationSteps, this.interpolationSpeed, targetSpeed);
        ticks *= this.interpolationSpeed;

        this.currentStepTicks += ticks;
        this.remainingTicks -= ticks;

        while (!this.remainingSteps.isEmpty()) {
            SteppedInterpolationHandler.Step step = this.remainingSteps.getFirst();
            int offset = step.tickOffset;
            if (this.currentStepTicks < offset) {
                return;
            }
            this.currentStepTicks -= offset;
            this.lastStepPosRot.set(step.position(), step.yRot(), step.xRot());
            this.remainingSteps.removeFirst();
        }
        this.remainingTicks = 0.0F;
    }
private PositionAndRotation getNewPositionAndRotation() {
    if (this.remainingSteps.isEmpty()) {
        return this;
    }
    SteppedInterpolationHandler.Step step = this.remainingSteps.getFirst();
    float a = this.currentStepTicks / step.tickOffset;
    return PositionAndRotation.of(
            this.lastStepPosRot.position().lerp(step.position(), a),
            Mth.rotLerp(a, this.lastStepPosRot.yRot(), step.yRot()),
            Mth.lerp(a, this.lastStepPosRot.xRot(), step.xRot())
    );
}


I think it makes sense structurally to have all of the core logic in the advance method only and avoid the problem with checking for having cleared the full path twice in this way. We set the starting value of currentStepTicks to 0 instead of 1 and instead run the advance() method before getting the new position and rotation.

The “fix” of this seems to have just been reverted in 26.3-rc-3. I think it would be a much better approach to make the attack_damage attribute syncable. That should be a very simple change and it only changes client behaviour slightly, importantly it makes baseDamage be equal to 0 in Player::attack if the attack_damage attribute value is 0, skipping most of the attacking logic.

I accidentally created a duplicate of this with a lot more information and reproduction steps in MC-311022. Not sure how incomplete reports like this are usually handled here, but look there for more information on this.

For all blocks and entities in Minecraft with the exception of the lectern, the outline when hovering over it matches exactly with the actual interaction hitbox of that object. MC-154004 only makes a suggestion about how the top shouldn’t be slanted, so I don’t think this should be a considered a duplicate and possibly be reopened. I have attached a video to show that this still occurs in 26.3 Snapshot 5.

[media: lectern.mp4]

From what I can tell, this doesn’t really seem like a proper fix but just delays interacting with newly held items by a tick (as @Longieee and @Primitive Enderdragon already mentioned). I believe the expected behaviour is that the Item in the slot the player is hovering the instance the client tick occurs is treated as the Item the player is holding. The previous client behaviour made perfect sense in that it always correctly sent the correct SetCarriedItem packet right before the corresponding Interaction and Swing packets, and I think the newly introduced client behaviour is very suboptimal. The problem lies entirely within how the Server handles these incoming packets. When the server receives a SetCarriedItem packet it essentially runs Inventory.setSelectedSlot(…), but doesn’t update the corresponding attributes until the next server tick. I think it would probably be more logical to call player.collectEquipmentChanges(…) or similar from within the SetCarriedItem packet listener to instantly apply the attributes on the server.

@[MCQA] Baslod

My attached video shows a clear setup that is very easy to reproduce and the problem should be extremely obvious. I already went through a lot of effort to point out the exact issue and I have absolutely no clue what further you want from me.

@gnembon No, the new slime/bed bounce physics completely forget to take drag into consideration. Why are you asking questions and then resolving the report instantly?

Can confirm in 26.1.1.

[media: Video Project 28.mp4]

@bl4ckscor3 I’m sorry for the late reply. Both of these mechanics worked correctly in 1.15-1.20. The bug I mentioned in MC-275671 which likely causes the first of those already existed pre-1.14 and was recently readded in 1.21. The second one is likely due to the complete stepping overhaul in 1.21

[media]
[media]
[media]
[media]
[media]
[media]

still happening in 1.21.5 😢

Still happening in 25w04a. 😞

[media]

I uploaded another video showcasing an easy setup for replicating this glitch. Just walk up to the trapdoor, press space and w at the same time while sprinting and hold space.

@Jiingy You're jumping too late. It's a tick perfect space input, player hitbox doesn't move either.