Page MenuHomeWildfire Games

AuraManager shouldn't serialize the affected classes array directly
ClosedPublic

Authored by elexis on Jan 6 2018, 3:23 AM.

Details

Summary

The AuraManager is a cache that keeps the add and multiply bonuses of auras stored in order to improve performance.
It stores the classes array of the auras, for instance ["Structure", "Ship"].

In rP20737 / D1108 the DataTemplateManager component was removed and with it the serialization of aura templates in the Auras component.
So the affected classes in the AuraManager are serialized while the affected classes in the Auras component isn't serialized.
So when AuraManager.RemoveTemplateBonus is called after deserialization, that function fails to identify the old classes array, since it has a new array after deserialization.

There are multiple ways to address the issue:

(1) One would be using the Auraname as an identifier and then looking up the affected classes.
But that would be a big rewrite and we should now find the smallest bugfix (so that in case the big rewrite fails, we don't have to revert two rewrites).
(Also the performance would have to be judged)

(2)
The approach here is to not store the affected classes array, but to store the effects per item of that array.
That means the cache gets bigger, but the performance shouldn't be much worse.
The only bad part is the [class] array construction.

(3) We could change the aura template structure or MatchClasses. But that doesn't sound minimal either.

Test Plan

#4924 contains a way to reproduce the reported failure.
For instance build a theatron, save, load, resign and it will show an error because it fails to identify the classes array.

See D1198 for a brief explanation what the component does.

You can use the attached patch to display the contents of the AuraManager cache variables.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jan 6 2018, 3:23 AM
elexis edited the test plan for this revision. (Show Details)Jan 6 2018, 3:25 AM
Vulcan added a subscriber: Vulcan.Jan 6 2018, 3:25 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis edited the test plan for this revision. (Show Details)Jan 6 2018, 3:26 AM
Vulcan added a comment.Jan 6 2018, 3:26 AM
Executing section Default...
Executing section Source...
Executing section JS...
elexis edited the test plan for this revision. (Show Details)Jan 6 2018, 3:27 AM
gameboy added a subscriber: gameboy.Jan 6 2018, 3:47 AM

Can you provide a complete patch repair ticket #4924 and [[ https://trac.wildfiregames.com/ticket/4926 | #4926 ]]mentioned in the question, can you upload the patch to SVN?

OK,this pitch, Can this patch and this patch be used at the same time? Perhaps only this patch is used: Superhackylolnopeuneval.patch???

bb added a comment.Jan 6 2018, 9:51 PM

This will work as long as the arrays are not nestled, however that is allowed..., but if we have nestled arrays, the inner things are "and", so we can easily make that a string by .join("+") and that might work.

binaries/data/mods/public/simulation/components/AuraManager.js
241 ↗(On Diff #5099)

probably when doing the above the array scope here can be removed (but requires testing)

elexis updated this revision to Diff 5153.Jan 7 2018, 12:39 AM
elexis edited the test plan for this revision. (Show Details)

Add the join("+") for the third format of MatchesClassList as suggested by bb.

Vulcan added a comment.Jan 7 2018, 2:22 AM
Executing section Default...
Executing section Source...
Executing section JS...
Vulcan added a comment.Jan 7 2018, 6:02 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

This patch has fixed this mistake. Thank you very much, my friend, elexis.

bb accepted this revision.Jan 7 2018, 10:15 PM

Couldn't reproduce the error and the oos with the patch applied, while I could without.

For now this is a fix, but hoping a better solution can be found

binaries/data/mods/public/simulation/components/AuraManager.js
111 ↗(On Diff #5153)

This works for the reason that an aura can only apply once and different classes can still have the same key thus with:

aura affects [a, b]
template with classes [a,b]

aura will apply only once

However it is entirely not rubust imo (not that I see a better solution)

114 ↗(On Diff #5153)

using + to merge the classes is correct following the rules from MatchesClassList, since the inner scope in the array means "and"

249 ↗(On Diff #5153)

the scopes here are unneeded (but don't hurt either)

This revision is now accepted and ready to land.Jan 7 2018, 10:15 PM
mimo added a comment.Jan 20 2018, 4:14 PM

Any reason not to commit this?

In D1201#50157, @mimo wrote:

Any reason not to commit this?

I wanted to find out if there was a better way to fix this - by removing serialization entirely and rebuilding the cache upon deserialization. (Any knowledge about this being a good or stupid idea would be appreciated)
Also I'm afraid to make things worse as it is my first sighting of this file.
I'll try to close this X file now.

gameboy added a comment.EditedJan 21 2018, 10:05 AM

My friend MIMO, do you have a better solution to solve this problem?
@mimo

It ought to be possible to rebuild the AuraManager without serialization, but my attempts are always failing so far. I'm not satisfied yet.

I can't get it to work and I expect the breakage to increase if we remove the serialization in a non-straightforward way (note that we already have an edge case in Auras on init, the autogarrisoning). Especially given that we don't have much time left for the release, committing it as is.

Note auras do with affects being strings don't throw errors and do apply aura effects, just not as intended.
For instance changing female inspiration aura affects from ["Citizen Soldier"] to "Citizen Soldier" results in the aura being applied to all Citizen and all Soldier units, i.e. women too.
So the code in this patch is correct (as it assumes affects to be an array), but the code in MatchesClassesList could become more strict and aura templates ought to be hardened against malformed properties.

Thanks for the review bb!

@elexis My friend, have you decided to formally launch this patch? Or do you have a better solution?

And the modification cache of the TechnologyManager doesn't have that problem.

This revision was automatically updated to reflect the committed changes.