mirrors/0ad
1
0
Fork 2
mirror of https://gitea.wildfiregames.com/0ad/0ad.git synced 2026-09-21 20:06:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix modifiersManager cache incorrectly storing values with player-wide modifier

Browse Source 
The cache would take the value post-player-modification, which is incorrect. This would usually just make caching fail, but could trigger worse problems if the post-player value is also used in other situations.

One such situation leads to an OOS for britons.

Thanks to elexis for the thorough investigation.

Fixes #7634
This commit is contained in:
Lancelot de Ferrière
2025-02-17 19:53:21 +01:00
committed by wraitii
parent e65879bdbf
commit 0a4bfefb1e
2 changed files with 35 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
binaries/data/mods/public/simulation/components/ModifiersManager.js
+4 -8
View File
@@ -110,10 +110,7 @@ ModifiersManager.prototype.FetchModifiedProperty = function(classesList, propert
return GetTechModifiedProperty(modifications.flat(), classesList, originalValue);
};
/**
* @returns originalValue after modifiers
*/
ModifiersManager.prototype.Cache = function(classesList, propertyName, originalValue, entity)
ModifiersManager.prototype.Cache = function(classesList, propertyName, originalValue, newValue, entity)
{
let cache = this.cachedValues.get(propertyName);
if (!cache)
@@ -123,9 +120,7 @@ ModifiersManager.prototype.Cache = function(classesList, propertyName, originalV
if (!cache2)
cache2 = cache.set(entity, new Map()).get(entity);
let value = this.FetchModifiedProperty(classesList, propertyName, originalValue, entity);
cache2.set(originalValue, value);
return value;
cache2.set(originalValue, newValue);
};
/**
@@ -174,7 +169,8 @@ ModifiersManager.prototype.ApplyModifiers = function(propertyName, originalValue
pc.add(entity);
newValue = this.FetchModifiedProperty(classesList, propertyName, newValue, ownerEntity);
}
newValue = this.Cache(classesList, propertyName, newValue, entity);
newValue = this.FetchModifiedProperty(classesList, propertyName, newValue, entity);
this.Cache(classesList, propertyName, originalValue, newValue, entity);
return newValue;
};
binaries/data/mods/public/simulation/components/tests/test_ModifiersManager.js
+31 -21
View File
@@ -9,6 +9,7 @@ cmpModifiersManager.Init();
// These should be different as that is the general case.
const PLAYER_ID_FOR_TEST = 2;
const PLAYER_ENTITY_ID = 3;
const STRUCTURE_ENTITY_ID = 5;
AddMock(SYSTEM_ENTITY, IID_RangeManager, {
"GetEntitiesByPlayer": () => [],
@@ -22,7 +23,7 @@ AddMock(PLAYER_ENTITY_ID, IID_Player, {
"GetPlayerID": () => PLAYER_ID_FOR_TEST
});
let entitiesToTest = [5, 6, 7, 8];
let entitiesToTest = [STRUCTURE_ENTITY_ID, 6, 7, 8];
for (let ent of entitiesToTest)
AddMock(ent, IID_Ownership, {
"GetOwner": () => PLAYER_ID_FOR_TEST
@@ -31,7 +32,7 @@ for (let ent of entitiesToTest)
AddMock(PLAYER_ENTITY_ID, IID_Identity, {
"GetClassesList": () => "Player",
});
AddMock(5, IID_Identity, {
AddMock(STRUCTURE_ENTITY_ID, IID_Identity, {
"GetClassesList": () => "Structure",
});
AddMock(6, IID_Identity, {
@@ -87,38 +88,47 @@ cmpModifiersManager.AddModifiers("Test_A_0", {
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_A", 5, 5), 15);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_B", 5, 8), 13);
// Add two local modifications, only the first should stick.
cmpModifiersManager.AddModifier("Test_C", "Test_C_0", [{ "affects": ["Structure"], "add": 10 }], 5);
cmpModifiersManager.AddModifier("Test_C", "Test_C_1", [{ "affects": ["Unit"], "add": 5 }], 5);
cmpModifiersManager.AddModifier("Test_C", "Test_C_0", [{ "affects": ["Structure"], "add": 10 }], STRUCTURE_ENTITY_ID);
cmpModifiersManager.AddModifier("Test_C", "Test_C_invalid", [{ "affects": ["Unit"], "add": 5 }], STRUCTURE_ENTITY_ID);
SerializationCycle();
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, 5), 15);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, STRUCTURE_ENTITY_ID), 15);
// test that local modifications are indeed applied after global managers
cmpModifiersManager.AddModifier("Test_C", "Test_C_2", [{ "affects": ["Structure"], "replace": 0 }], 5);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, 5), 0);
cmpModifiersManager.AddModifier("Test_C", "Test_C_player", [{ "affects": ["Structure"], "replace": 2 }], PLAYER_ENTITY_ID);
TS_ASSERT(!cmpModifiersManager.HasAnyModifier("Test_C_3", PLAYER_ENTITY_ID));
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, STRUCTURE_ENTITY_ID), 12);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 2, STRUCTURE_ENTITY_ID), 12);
SerializationCycle();
// check that things still work properly if we change global modifications
cmpModifiersManager.AddModifier("Test_C", "Test_C_3", [{ "affects": ["Structure"], "add": 10 }], PLAYER_ENTITY_ID);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, 5), 0);
TS_ASSERT(cmpModifiersManager.HasAnyModifier("Test_C_3", PLAYER_ENTITY_ID));
TS_ASSERT(cmpModifiersManager.HasModifier("Test_C", "Test_C_3", PLAYER_ENTITY_ID));
TS_ASSERT(cmpModifiersManager.HasModifier("Test_C", "Test_C_2", 5));
// test removal
cmpModifiersManager.RemoveModifier("Test_C", "Test_C_2", 5);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, 5), 25);
cmpModifiersManager.RemoveModifier("Test_C", "Test_C_player", PLAYER_ENTITY_ID);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, STRUCTURE_ENTITY_ID), 15);
// check that things still work properly if we change global modifications
cmpModifiersManager.AddModifier("Test_C", "Test_C_player", [{ "affects": ["Structure"], "add": 12 }], PLAYER_ENTITY_ID);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_C", 5, STRUCTURE_ENTITY_ID), 27);
TS_ASSERT(cmpModifiersManager.HasAnyModifier("Test_C_player", PLAYER_ENTITY_ID));
TS_ASSERT(cmpModifiersManager.HasModifier("Test_C", "Test_C_player", PLAYER_ENTITY_ID));
SerializationCycle();
TS_ASSERT(cmpModifiersManager.HasModifier("Test_C", "Test_C_3", PLAYER_ENTITY_ID));
TS_ASSERT(!cmpModifiersManager.HasModifier("Test_C", "Test_C_2", 5));
TS_ASSERT(cmpModifiersManager.HasModifier("Test_C", "Test_C_player", PLAYER_ENTITY_ID));
TS_ASSERT(!cmpModifiersManager.HasModifier("Test_C", "Test_C_player", STRUCTURE_ENTITY_ID));
// Regression test for a caching issue
cmpModifiersManager.AddModifier("Test_E", "Test_E_player", [{ "affects": ["Structure"], "add": 1 }], PLAYER_ENTITY_ID);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_E", 3, STRUCTURE_ENTITY_ID), 4);
cmpModifiersManager.AddModifier("Test_E", "Test_E_1", [{ "affects": ["Structure"], "add": 1 }], STRUCTURE_ENTITY_ID);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_E", 4, STRUCTURE_ENTITY_ID), 6);
TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Test_E", 5, STRUCTURE_ENTITY_ID), 7);
// Test that entities keep local modifications but not global ones when changing owner.
AddMock(SYSTEM_ENTITY, IID_PlayerManager, {
@@ -134,7 +144,7 @@ cmpModifiersManager.Init();
cmpModifiersManager.AddModifier("Test_D", "Test_D_0", [{ "affects": ["Structure"], "add": 10 }], PLAYER_ENTITY_ID);
cmpModifiersManager.AddModifier("Test_D", "Test_D_1", [{ "affects": ["Structure"], "add": 1 }], PLAYER_ENTITY_ID + 1);
cmpModifiersManager.AddModifier("Test_D", "Test_D_2", [{ "affects": ["Structure"], "add": 5 }], 5);
cmpModifiersManager.AddModifier("Test_D", "Test_D_2", [{ "affects": ["Structure"], "add": 5 }], STRUCTURE_ENTITY_ID);
cmpModifiersManager.OnGlobalPlayerEntityChanged({ "player": PLAYER_ID_FOR_TEST, "from": -1, "to": PLAYER_ENTITY_ID });
cmpModifiersManager.OnGlobalPlayerEntityChanged({ "player": PLAYER_ID_FOR_TEST + 1, "from": -1, "to": PLAYER_ENTITY_ID + 1 });