Page MenuHomeWildfire Games

Slight cleanup of fogging OnDestroy and some comments
Needs ReviewPublic

Authored by wraitii on Sun, Jan 6, 11:54 AM.

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Following rP22029 I dwelled in the fogging code. Some confusion could have been avoided by some comments.

Also early-exit the loop to avoid doing un-necessary js->c++ transitions.

Test Plan

Code review.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
D1737_mirage_cleanup
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6756
Build 11115: Vulcan BuildJenkins
Build 11114: arc lint + arc unit

Event Timeline

Vulcan added a subscriber: Vulcan.Sun, Jan 6, 11:57 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|  73|  73| 	this.classesList = clone(cmpIdentity.GetClassesList());
|  74|  74| };
|  75|  75| 
|  76|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  76|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  77|  77| 
|  78|  78| // Foundation data
|  79|  79| 

binaries/data/mods/public/simulation/components/Mirage.js
|  76| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/933/

wraitii added a reviewer: Restricted Owners Package.Sun, Jan 6, 5:30 PM
Itms requested changes to this revision.Sun, Jan 6, 9:05 PM
Itms added a subscriber: Itms.
Itms added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
202

Good idea!

206

What about the following?

When this.entity is in the line of sight of the player, its mirage is hidden, rather than destroyed, to save on performance.
All hidden mirages can be destroyed now (they won't be needed again), and other mirages will destroy themselves when they get out of the fog.

It's easy to confuse the reader by mixing up the "fogged entity" and the visibility status of either entity.

207

I'd like to keep this == hidden. Mirages are visible for one turn while they are forwarded to the actual entity (else they "blink"). In this case, it doesn't matter, because the parent entity is destroyed, but for consistency with the rest of the code, I recommend against changing it.

binaries/data/mods/public/simulation/components/Mirage.js
211

👍

This revision now requires changes to proceed.Sun, Jan 6, 9:05 PM
wraitii updated this revision to Diff 7303.Mon, Jan 7, 8:17 PM
wraitii marked 4 inline comments as done.

Modified after itms' comments.

Vulcan added a comment.Mon, Jan 7, 8:28 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Mirage.js
|  73|  73| 	this.classesList = clone(cmpIdentity.GetClassesList());
|  74|  74| };
|  75|  75| 
|  76|    |-Mirage.prototype.GetClassesList = function() { return this.classesList };
|    |  76|+Mirage.prototype.GetClassesList = function() { return this.classesList; };
|  77|  77| 
|  78|  78| // Foundation data
|  79|  79| 

binaries/data/mods/public/simulation/components/Mirage.js
|  76| Mirage.prototype.GetClassesList·=·function()·{·return·this.classesList·};
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/940/