From 8cfc61d3b6aca91fb7ba9c18ca9347aef04aece6 Mon Sep 17 00:00:00 2001 From: soloturn Date: Wed, 19 Aug 2026 17:43:21 +0200 Subject: [PATCH] fix(characters): stop extrapolating a dying entity's movement state into an NPE Fixes #4969. ServerCharacterPredictionSystem.onDestroy (BeforeDeactivateComponent, gated on CharacterMovementComponent/LocationComponent/AliveCharacterComponent) fires as soon as any one of those three is removed - e.g. an entity losing AliveCharacterComponent mid-death, before the rest of it is torn down. It only queues the entity into characterStatesToRemove rather than removing it from characterStates immediately; that queue is drained once per update() call. In between, restoreToPresent() and lagCompensate() both iterate characterStates unconditionally and can call setToTime -> setToExtrapolateState -> extrapolateCharacterMovementComponent on an entity whose CharacterMovementComponent is already gone, which unconditionally dereferenced the null getComponent() result. Two changes: - restoreToPresent()/lagCompensate() now skip entries already queued in characterStatesToRemove, matching what update()'s own replication loop already did for the same reason. - setToExtrapolateState() now checks for both components up front and returns if either is missing, mirroring setToState()'s existing guard in the same class - defense in depth in case some other path reaches it with a torn-down entity. Regression test added: CharacterMovementSystemUtilityTest. Co-Authored-By: Claude Sonnet 5 --- .../CharacterMovementSystemUtilityTest.java | 77 +++++++++++++++++++ .../CharacterMovementSystemUtility.java | 9 +++ .../ServerCharacterPredictionSystem.java | 18 +++++ 3 files changed, 104 insertions(+) create mode 100644 engine-tests/src/test/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtilityTest.java diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtilityTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtilityTest.java new file mode 100644 index 00000000000..6278ac94c75 --- /dev/null +++ b/engine-tests/src/test/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtilityTest.java @@ -0,0 +1,77 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 +package org.terasology.engine.logic.characters; + +import org.joml.Quaternionf; +import org.joml.Vector3f; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.terasology.engine.entitySystem.entity.EntityRef; +import org.terasology.engine.logic.location.LocationComponent; +import org.terasology.engine.physics.engine.CharacterCollider; +import org.terasology.engine.physics.engine.PhysicsEngine; + +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; + +/** + * Regression test for #4969: an entity mid-death can lose {@link CharacterMovementComponent} (or {@link + * LocationComponent}) between being queued for a state update and that update actually running - see + * {@link ServerCharacterPredictionSystem#onDestroy}. {@link CharacterMovementSystemUtility#setToExtrapolateState} + * used to assume both components were always present and NPE'd the moment they weren't. + */ +public class CharacterMovementSystemUtilityTest { + private PhysicsEngine physics; + private CharacterMovementSystemUtility utility; + private CharacterStateEvent state; + + @BeforeEach + public void setup() { + physics = Mockito.mock(PhysicsEngine.class); + utility = new CharacterMovementSystemUtility(physics); + state = new CharacterStateEvent(0, 1, new Vector3f(), new Quaternionf(), new Vector3f(), + 0, 0, MovementMode.WALKING, true); + } + + @Test + public void setToExtrapolateStateSkipsEntityMissingCharacterMovementComponent() { + EntityRef entity = Mockito.mock(EntityRef.class); + Mockito.when(entity.getComponent(LocationComponent.class)).thenReturn(new LocationComponent()); + Mockito.when(entity.getComponent(CharacterMovementComponent.class)).thenReturn(null); + + // Must not throw - this is the exact NPE from #4969's traceback. + utility.setToExtrapolateState(entity, state, 100); + + verify(entity, never()).saveComponent(Mockito.any()); + verifyNoInteractions(physics); + } + + @Test + public void setToExtrapolateStateSkipsEntityMissingLocationComponent() { + EntityRef entity = Mockito.mock(EntityRef.class); + Mockito.when(entity.getComponent(LocationComponent.class)).thenReturn(null); + Mockito.when(entity.getComponent(CharacterMovementComponent.class)).thenReturn(new CharacterMovementComponent()); + + utility.setToExtrapolateState(entity, state, 100); + + verify(entity, never()).saveComponent(Mockito.any()); + verifyNoInteractions(physics); + } + + @Test + public void setToExtrapolateStateUpdatesEntityWithBothComponents() { + EntityRef entity = Mockito.mock(EntityRef.class); + Mockito.when(entity.getComponent(LocationComponent.class)).thenReturn(new LocationComponent()); + Mockito.when(entity.getComponent(CharacterMovementComponent.class)).thenReturn(new CharacterMovementComponent()); + CharacterCollider collider = Mockito.mock(CharacterCollider.class); + Mockito.when(physics.getCharacterCollider(entity)).thenReturn(collider); + + utility.setToExtrapolateState(entity, state, 100); + + verify(entity).saveComponent(Mockito.isA(LocationComponent.class)); + verify(entity).saveComponent(Mockito.isA(CharacterMovementComponent.class)); + verify(collider).setLocation(Mockito.any()); + } +} diff --git a/engine/src/main/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtility.java b/engine/src/main/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtility.java index 8fa3cacdcb7..6fca7068f30 100644 --- a/engine/src/main/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtility.java +++ b/engine/src/main/java/org/terasology/engine/logic/characters/CharacterMovementSystemUtility.java @@ -97,6 +97,15 @@ public void setToInterpolateState(EntityRef entity, CharacterStateEvent a, Chara } public void setToExtrapolateState(EntityRef entity, CharacterStateEvent state, long time) { + // Mirrors setToState's own null-checks above: an entity mid-death can lose + // CharacterMovementComponent (or LocationComponent) between being queued for a state update + // and this actually running - see ServerCharacterPredictionSystem#onDestroy. Extrapolating a + // now-nonexistent state is meaningless, not an error; skip it like setToState does. + if (entity.getComponent(LocationComponent.class) == null + || entity.getComponent(CharacterMovementComponent.class) == null) { + return; + } + float t = (time - state.getTime()) * 0.0001f; Vector3f newPos = new Vector3f(state.getVelocity()); newPos.mul(t); diff --git a/engine/src/main/java/org/terasology/engine/logic/characters/ServerCharacterPredictionSystem.java b/engine/src/main/java/org/terasology/engine/logic/characters/ServerCharacterPredictionSystem.java index 6c8395ad3cb..d2d8403c403 100644 --- a/engine/src/main/java/org/terasology/engine/logic/characters/ServerCharacterPredictionSystem.java +++ b/engine/src/main/java/org/terasology/engine/logic/characters/ServerCharacterPredictionSystem.java @@ -103,6 +103,11 @@ public void onCreate(final OnActivatedComponent event, final EntityRef entity) { AliveCharacterComponent.class}) public void onDestroy(final BeforeDeactivateComponent event, final EntityRef entity) { physics.removeCharacterCollider(entity); + // Queued rather than removed immediately: this fires whenever any of the three required + // components is dropped (e.g. a dying entity losing AliveCharacterComponent mid-animation, + // before the rest of it is torn down), so characterStates can still hold this entity - with + // some of its components already gone - until update() batches the actual removal below. + // lagCompensate()/restoreToPresent() skip anything in this queue for the same reason. characterStatesToRemove.add(entity); lastInputEvent.remove(entity); } @@ -260,6 +265,14 @@ private void setToTime(long renderTime, EntityRef entity, CircularBuffer> entry : characterStates.entrySet()) { + // onDestroy queues the entity here rather than removing it immediately (see its own + // comment); until the next update() drains that queue, characterStates can still hold an + // entity whose CharacterMovementComponent (or LocationComponent) is already gone - e.g. + // a mob that died mid-tick but is still finishing its death animation. Skipping it here + // matches what update()'s own replication loop already does for the same reason. + if (characterStatesToRemove.contains(entry.getKey())) { + continue; + } if (networkSystem.getOwnerEntity(entry.getKey()).equals(client)) { characterMovementSystemUtility.setToState(entry.getKey(), entry.getValue().getLast()); } else { @@ -272,6 +285,11 @@ public void lagCompensate(EntityRef client, long timeMs) { public void restoreToPresent() { long renderTime = time.getGameTimeInMs() - RENDER_DELAY; for (Map.Entry> entry : characterStates.entrySet()) { + // See the matching comment in lagCompensate(): entries pending removal can already be + // missing the components setToTime -> setToExtrapolateState needs. + if (characterStatesToRemove.contains(entry.getKey())) { + continue; + } setToTime(renderTime, entry.getKey(), entry.getValue()); } }