From fc6a9087750a7a65dff9505e549509f2f097ddb4 Mon Sep 17 00:00:00 2001 From: josue Date: Fri, 12 Jun 2026 15:32:07 +0200 Subject: [PATCH] Don't send redundant PositionChanged messages UnitAI constantly calls FaceTowardsTarget while attacking or gathering, broadcasting an MT_PositionChanged message each time even when the entity already faces the target. With many units fighting, that floods the message bus (range manager, obstruction, minimap, AI proxy, ...) every turn. This was attempted in 080599442f by returning early from TurnTo, but MoveAndTurnTo relies on TurnTo to advertise the movement, so units walking in a straight line stopped advertising their position and LOS broke (#6844), which led to the revert in 3fb7319df7. Instead, deduplicate in AdvertisePositionChanges itself: remember the data of the last message sent and skip the message when it wouldn't change anything. Since the comparison covers the whole message data, a movement with an unchanged angle is still advertised. Add a regression test covering both #7654 and the #6844 scenario. Fixes: #7654 Co-Authored-By: Claude Fable 5 --- .../simulation2/components/CCmpPosition.cpp | 54 +++++++++--- .../components/tests/test_Position.h | 87 ++++++++++++++++++- 2 files changed, 129 insertions(+), 12 deletions(-) diff --git a/source/simulation2/components/CCmpPosition.cpp b/source/simulation2/components/CCmpPosition.cpp index 5d05b4a012..9731ffc85a 100644 --- a/source/simulation2/components/CCmpPosition.cpp +++ b/source/simulation2/components/CCmpPosition.cpp @@ -116,6 +116,14 @@ public: bool m_EnabledMessageInterpolate; + // The data of the last CMessagePositionChanged sent, to avoid sending redundant + // messages (e.g. when UnitAI turns an entity towards a target it already faces). + // Derived state: it always matches the current position when messages are up to + // date, so Deserialize recomputes it instead of serializing it. + bool m_LastAdvertisedInWorld; + entity_pos_t m_LastAdvertisedX, m_LastAdvertisedZ; + entity_angle_t m_LastAdvertisedRotY; + static std::string GetSchema() { return @@ -174,6 +182,11 @@ public: m_ActorFloating = false; m_EnabledMessageInterpolate = false; + + // Match the message that would be sent while out of the world. + m_LastAdvertisedInWorld = false; + m_LastAdvertisedX = m_LastAdvertisedZ = entity_pos_t::Zero(); + m_LastAdvertisedRotY = entity_angle_t::Zero(); } void Deinit() override @@ -278,6 +291,13 @@ public: UpdateXZRotation(); UpdateMessageSubscriptions(); + + // No message is sent during deserialization (subscribers restore their own + // state), so the last advertised data is the current position. + m_LastAdvertisedInWorld = m_InWorld; + m_LastAdvertisedX = m_InWorld ? m_X : entity_pos_t::Zero(); + m_LastAdvertisedZ = m_InWorld ? m_Z : entity_pos_t::Zero(); + m_LastAdvertisedRotY = m_InWorld ? m_RotY : entity_angle_t::Zero(); } void Deserialized() @@ -905,7 +925,7 @@ private: * - m_X, m_Z * - m_RotY */ - void AdvertisePositionChanges() const + void AdvertisePositionChanges() { for (std::set::const_iterator it = m_Turrets.begin(); it != m_Turrets.end(); ++it) { @@ -913,16 +933,28 @@ private: if (cmpPosition) cmpPosition->UpdateTurretPosition(); } - if (m_InWorld) - { - CMessagePositionChanged msg(GetEntityId(), true, m_X, m_Z, m_RotY); - GetSimContext().GetComponentManager().PostMessage(GetEntityId(), msg); - } - else - { - CMessagePositionChanged msg(GetEntityId(), false, entity_pos_t::Zero(), entity_pos_t::Zero(), entity_angle_t::Zero()); - GetSimContext().GetComponentManager().PostMessage(GetEntityId(), msg); - } + + const entity_pos_t x = m_InWorld ? m_X : entity_pos_t::Zero(); + const entity_pos_t z = m_InWorld ? m_Z : entity_pos_t::Zero(); + const entity_angle_t rotY = m_InWorld ? m_RotY : entity_angle_t::Zero(); + + // Don't send a message if the advertised position didn't actually change, + // e.g. when UnitAI keeps facing an entity towards a target it already faces + // (#7654). The check covers the whole message data, not just the angle, so + // MoveAndTurnTo with an unchanged angle still advertises the movement (#6844). + if (m_InWorld == m_LastAdvertisedInWorld && + x == m_LastAdvertisedX && + z == m_LastAdvertisedZ && + rotY == m_LastAdvertisedRotY) + return; + + m_LastAdvertisedInWorld = m_InWorld; + m_LastAdvertisedX = x; + m_LastAdvertisedZ = z; + m_LastAdvertisedRotY = rotY; + + CMessagePositionChanged msg(GetEntityId(), m_InWorld, x, z, rotY); + GetSimContext().GetComponentManager().PostMessage(GetEntityId(), msg); } /** diff --git a/source/simulation2/components/tests/test_Position.h b/source/simulation2/components/tests/test_Position.h index 3c5ba2b52a..0b0c65f2e6 100644 --- a/source/simulation2/components/tests/test_Position.h +++ b/source/simulation2/components/tests/test_Position.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2025 Wildfire Games. +/* Copyright (C) 2026 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -58,6 +58,39 @@ public: } }; +class MockPositionChangedListener : public IComponent +{ +public: + DEFAULT_MOCK_COMPONENT() + + JS::HandleValue GetJSInstance() const override + { + return JS::UndefinedHandleValue; + } + + bool NewJSObject(const ScriptInterface&, JS::MutableHandleObject) const override + { + return false; + } + + int m_Count = 0; + bool m_LastInWorld = false; + entity_pos_t m_LastX, m_LastZ; + entity_angle_t m_LastRotY; + + void HandleMessage(const CMessage& msg, bool /*global*/) override + { + if (msg.GetType() != MT_PositionChanged) + return; + const CMessagePositionChanged& msgData = static_cast(msg); + ++m_Count; + m_LastInWorld = msgData.inWorld; + m_LastX = msgData.x; + m_LastZ = msgData.z; + m_LastRotY = msgData.a; + } +}; + class TestCmpPosition : public CxxTest::TestSuite { public: @@ -213,6 +246,58 @@ public: TS_ASSERT_EQUALS(cmp->GetPosition(), fixedvec(100, 122, 200)); } + void test_advertise_position_changes() + { + CXeromycesEngine xeromycesEngine; + ComponentTestHelper test(*g_ScriptContext); + + MockTerrain terrain; + test.AddMock(SYSTEM_ENTITY, IID_Terrain, terrain); + + ICmpPosition* cmp = test.Add(CID_Position, "upright0false"); + + MockPositionChangedListener listener; + test.GetSimContext().GetComponentManager().DynamicSubscriptionNonsync(MT_PositionChanged, &listener, true); + + // Moving out of the world while already out of it doesn't send a message. + cmp->MoveOutOfWorld(); + TS_ASSERT_EQUALS(listener.m_Count, 0); + + // Entering the world does. + cmp->JumpTo(entity_pos_t::FromInt(100), entity_pos_t::FromInt(200)); + TS_ASSERT_EQUALS(listener.m_Count, 1); + TS_ASSERT(listener.m_LastInWorld); + TS_ASSERT_EQUALS(listener.m_LastX, entity_pos_t::FromInt(100)); + TS_ASSERT_EQUALS(listener.m_LastZ, entity_pos_t::FromInt(200)); + + // Turning to a new angle sends a message. + cmp->TurnTo(entity_angle_t::FromInt(1)); + TS_ASSERT_EQUALS(listener.m_Count, 2); + TS_ASSERT_EQUALS(listener.m_LastRotY, entity_angle_t::FromInt(1)); + + // Turning to the angle the entity already faces doesn't (#7654). + cmp->TurnTo(entity_angle_t::FromInt(1)); + TS_ASSERT_EQUALS(listener.m_Count, 2); + + // Moving without turning still advertises the movement (#6844). + cmp->MoveAndTurnTo(entity_pos_t::FromInt(110), entity_pos_t::FromInt(200), entity_angle_t::FromInt(1)); + TS_ASSERT_EQUALS(listener.m_Count, 3); + TS_ASSERT_EQUALS(listener.m_LastX, entity_pos_t::FromInt(110)); + TS_ASSERT_EQUALS(listener.m_LastZ, entity_pos_t::FromInt(200)); + TS_ASSERT_EQUALS(listener.m_LastRotY, entity_angle_t::FromInt(1)); + + // Moving to the position the entity already has doesn't send a message. + cmp->MoveTo(entity_pos_t::FromInt(110), entity_pos_t::FromInt(200)); + TS_ASSERT_EQUALS(listener.m_Count, 3); + + // Leaving the world sends an out-of-world message, but only once. + cmp->MoveOutOfWorld(); + TS_ASSERT_EQUALS(listener.m_Count, 4); + TS_ASSERT(!listener.m_LastInWorld); + cmp->MoveOutOfWorld(); + TS_ASSERT_EQUALS(listener.m_Count, 4); + } + void test_serialize() { CXeromycesEngine xeromycesEngine;