Page MenuHomeWildfire Games

Fix issue with Healer range visualization described in #4632
ClosedPublic

Authored by Sandarac on Jul 1 2017, 1:50 AM.

Details

Summary

When loading a saved game, the heal range visualization of already-existing healers will not correctly take into account tech modifications. Due to the order in which components are deserialized; when RangeVisualization.Deserialize() is called (which calls Init()), the ApplyValueModificationsToEntity in cmpHeal.GetRange() doesn't account for techs. And since UpdateVisualHealRanges will not be called again after Init() unless a relevant ValueModification message is sent, the healer has an incorrect range.

Two options:

  • Call UpdateVisualHealRanges when the Deserialized message is sent (sent when all components finished being deserialized).
  • Serialize the range value, and then use that on deserialization (not preferable).
Test Plan

Follow the steps in the trac ticket to see that old healers still take into account tech modifications when loading a saved game - with the patch.

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

Sandarac created this revision.Jul 1 2017, 1:50 AM
elexis accepted this revision.Jul 1 2017, 3:03 AM
elexis added a subscriber: elexis.

Confirmed that first the RangeVisualization component is deserialized, then the TechnologyManager (it's the alphabet afaik. Might or rather might not want to add a complicated dependency system some day.)

Index: binaries/data/mods/public/simulation/components/RangeVisualization.js
===================================================================
--- binaries/data/mods/public/simulation/components/RangeVisualization.js	(revision 19860)
+++ binaries/data/mods/public/simulation/components/RangeVisualization.js	(working copy)
@@ -18,10 +18,11 @@ RangeVisualization.prototype.Init = func
 // The GUI enables visualizations
 RangeVisualization.prototype.Serialize = null;
 
 RangeVisualization.prototype.Deserialize = function(data)
 {
+	warn("RangeVisualization");
 	this.Init();
 };
 
 RangeVisualization.prototype.UpdateVisualAuraRanges = function()
 {
Index: binaries/data/mods/public/simulation/components/TechnologyManager.js
===================================================================
--- binaries/data/mods/public/simulation/components/TechnologyManager.js	(revision 19860)
+++ binaries/data/mods/public/simulation/components/TechnologyManager.js	(working copy)
@@ -16,10 +16,17 @@ TechnologyManager.prototype.Serialize =
 	}
 	ret.modificationCache = {};
 	return ret;
 };
 
+TechnologyManager.prototype.Deserialize = function(data)
+{
+	warn("DESERIALIZE TECHMANAGER");
+	for (let x in data)
+		this[x] = data[x];
+};
+
 TechnologyManager.prototype.Init = function()
 {
 	this.researchedTechs = {}; // technologies which have been researched
 	this.researchQueued = {};  // technologies which are queued for research
 	this.researchStarted = {}; // technologies which are being researched currently (non-queued)

The patch works (easy to test as mentioned in the ticket).
It's somewhat unconventional to use the OnDeserialized message subscription (at least it's the first time we do it for a component, not counting that Trigger hack).
Also the trick won't scale indefinitely (what if another component needs to execute code after this new code?).
But it is more than sufficient for now, we won't run into any of these hypothetical issues soon.
The patch also leaves a very clear and readable impression.
Agree we should avoid serializing values where possible.

This revision is now accepted and ready to land.Jul 1 2017, 3:03 AM
elexis added a comment.Jul 1 2017, 3:06 AM

and thanks for the patch Sandarac!

This revision was automatically updated to reflect the committed changes.
Vulcan added a subscriber: Vulcan.Jul 1 2017, 3:43 AM

Build is green

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

http://jw:8080/job/phabricator/1674/ for more details.