Slight cleanup of fogging OnDestroy and some comments
ClosedPublic
Actions

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

wraitii created this revision.Jan 6 2019, 11:54 AM

wraitii mentioned this in rP15963: Fix mirages not flushed properly, introduced in r15956..

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/

Harbormaster completed remote builds in B6744: Diff 7291.Jan 6 2019, 11:57 AM

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

Modified after itms' comments.

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/

Harbormaster completed remote builds in B6756: Diff 7303.Jan 7 2019, 8:28 PM

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.

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.Apr 27 2019, 11:30 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.

@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. ?

Closed by commit rP22247: Slight cleanup of fogging OnDestroy and some comments (authored by wraitii). · Explain WhyMay 4 2019, 3:50 PM

This revision was automatically updated to reflect the committed changes.