Page MenuHomeWildfire Games

Slight cleanup of fogging OnDestroy and some comments
ClosedPublic

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

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22247: Slight cleanup of fogging OnDestroy and some comments
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Vulcan added a subscriber: Vulcan.Jan 6 2019, 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.Jan 6 2019, 5:30 PM
Itms requested changes to this revision.Jan 6 2019, 9:05 PM
Itms added a subscriber: Itms.
Itms added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
201 ↗(On Diff #7291)

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.

202 ↗(On Diff #7291)

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.

202 ↗(On Diff #7291)

Good idea!

binaries/data/mods/public/simulation/components/Mirage.js
211 ↗(On Diff #7291)

👍

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

Modified after itms' comments.

Vulcan added a comment.Jan 7 2019, 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/

bb added a subscriber: bb.Apr 16 2019, 2:06 PM
bb added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
185–186 ↗(On Diff #7303)

What is the benefit of merging those two function? appears to me we just add more checks, instead of calling the right function

wraitii added inline comments.Apr 16 2019, 5:02 PM
binaries/data/mods/public/simulation/components/Fogging.js
185–186 ↗(On Diff #7303)

I think (as in believe) that it's our general practice to use ownershipChanged instead of Destroy.
Three reasons I can think of:

  • OwnershipChanged gets called as soon as the unit actually "dies" in the simulation iirc, which isn't the case of Destroy I think (which gets called on flushing).
  • For several components, changing ownership and dying get handled by the same code.
  • It makes this component listen to one fewer message which is always good for perf.

I could be convinced of not merging them but I don't think it's too bad.

Itms accepted this revision.Sat, Apr 27, 11:30 PM

Thanks for performing the changes and sorry for not going back on this immediately. I agree with the merging of the functions, because we indeed often use OwnershipChanged for gameplay stuff whereas Destroy is seen as a rather low-level message. Additionally it's always better to associate miraging code with ownership concepts.

I'm accepting but I would be happy if you'd use the opportunity to add the semicolon detected by the linter when you commit.

This revision is now accepted and ready to land.Sat, Apr 27, 11:30 PM
bb added a comment.Sat, Apr 27, 11:37 PM

comment I had laying around:

binaries/data/mods/public/simulation/components/Fogging.js
185–186 ↗(On Diff #7303)

I think (as in believe) that it's our general practice to use ownershipChanged instead of Destroy.

In that case the onDestroy message shouldn't exist anywhere
Components with ownershipchange and destroy
gate
Garrisonholder (a global onwershipchange even)
unitAI
Fogging
BuildingAI
ProductionQueue

Components with ondestroy but no ownershipchange
pack
foundation (foundations are not capturable in the public mod, I guess this would bug...)
triggerpoint

It makes this component listen to one fewer message which is always good for perf.

Sounds even better if one removes the messages everywhere

OwnershipChanged gets called as soon as the unit actually "dies" in the simulation iirc, which isn't the case of Destroy I think (which gets called on flushing).

False, see messageTypes.h, L226 and further.

For several components, changing ownership and dying get handled by the same code.

There surely are examples where onDestroy could be used instead of ownershipChanged, but there are also examples where it is really the same code, so some duplication argument holds there.

Itms added a comment.Sat, Apr 27, 11:41 PM

@bb Could you create a ticket for that? It sounds like we should indeed take a look at this and make the use of OwnershipChanged over Destroy consistent, and put explicit comments where one is needed over the other. 👍

This revision was automatically updated to reflect the committed changes.