Page MenuHomeWildfire Games
Differential D2286

Notify renamed entities about value modifications when created
ClosedPublic
Actions

Authored by Silier on Sep 13 2019, 2:03 PM.

Details

Reviewers
Freagarach
wraitii
Commits
rP22912: Make health correctly recalculate its cached values when the entity changes…
Trac Tickets
#5588
Summary

Issue reported by Freagarach:
In SVN 22890, when promoting a unit with the cheat from the dev. menu the max health is not changed. This could be not limited to only health but also to other components that cache values and only use "OnValueModification?" to refresh them.

Solution:
Notifiy to update cache values when entity changes owner (because new entity is created when promoted)

Introduced in rP22767

Test Plan

Check bonus health is added correctly.

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

Silier created this revision.Sep 13 2019, 2:03 PM
Vulcan added a comment.Sep 13 2019, 2:04 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/150/display/redirect

Vulcan added a comment.Sep 13 2019, 2:07 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 433| 433| 		this.CheckRegenTimer();
| 434| 434| };
| 435| 435| 
| 436|    |-Health.prototype.OnGlobalEntityRenamed  = function(msg)
|    | 436|+Health.prototype.OnGlobalEntityRenamed = function(msg)
| 437| 437| {
| 438| 438| 	if (msg.newentity != this.entity)
| 439| 439| 		return;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 437| 437| {
| 438| 438| 	if (msg.newentity != this.entity)
| 439| 439| 		return;
| 440|    |-	this.OnValueModification({"component": "Health"});
|    | 440|+	this.OnValueModification({ "component": "Health"});
| 441| 441| }
| 442| 442| 
| 443| 443| Health.prototype.RegisterHealthChanged = function(from)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 437| 437| {
| 438| 438| 	if (msg.newentity != this.entity)
| 439| 439| 		return;
| 440|    |-	this.OnValueModification({"component": "Health"});
|    | 440|+	this.OnValueModification({"component": "Health" });
| 441| 441| }
| 442| 442| 
| 443| 443| Health.prototype.RegisterHealthChanged = function(from)
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 438| 438| 	if (msg.newentity != this.entity)
| 439| 439| 		return;
| 440| 440| 	this.OnValueModification({"component": "Health"});
| 441|    |-}
|    | 441|+};
| 442| 442| 
| 443| 443| Health.prototype.RegisterHealthChanged = function(from)
| 444| 444| {

binaries/data/mods/public/simulation/components/Health.js
| 441| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/660/display/redirect

Harbormaster completed remote builds in B9343: Diff 9736.Sep 13 2019, 2:07 PM
Silier edited the test plan for this revision. (Show Details)Sep 13 2019, 2:09 PM
wraitii added a subscriber: wraitii.Sep 13 2019, 2:15 PM

Seems like more of an issue in OnOwnershipChanged... What happens if player techs change the max HP?

Silier updated this revision to Diff 9737.EditedSep 13 2019, 2:27 PM

address comment by wraitii so this fixes also scenario where unit changes ownership and technologies change Health values based on player

Vulcan added a comment.Sep 13 2019, 2:28 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/151/display/redirect

Vulcan added a comment.Sep 13 2019, 2:31 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 438| 438| 	let cmpOwnership = Engine.QueryInterface(this.entity, IID_Ownership);
| 439| 439| 	if (!cmpOwnership || cmpOwnership.GetOwner() != msg.to)
| 440| 440| 		return;
| 441|    |-	this.OnValueModification({"component": "Health"});
|    | 441|+	this.OnValueModification({ "component": "Health"});
| 442| 442| }
| 443| 443| 
| 444| 444| Health.prototype.RegisterHealthChanged = function(from)
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 438| 438| 	let cmpOwnership = Engine.QueryInterface(this.entity, IID_Ownership);
| 439| 439| 	if (!cmpOwnership || cmpOwnership.GetOwner() != msg.to)
| 440| 440| 		return;
| 441|    |-	this.OnValueModification({"component": "Health"});
|    | 441|+	this.OnValueModification({"component": "Health" });
| 442| 442| }
| 443| 443| 
| 444| 444| Health.prototype.RegisterHealthChanged = function(from)
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 439| 439| 	if (!cmpOwnership || cmpOwnership.GetOwner() != msg.to)
| 440| 440| 		return;
| 441| 441| 	this.OnValueModification({"component": "Health"});
| 442|    |-}
|    | 442|+};
| 443| 443| 
| 444| 444| Health.prototype.RegisterHealthChanged = function(from)
| 445| 445| {

binaries/data/mods/public/simulation/components/Health.js
| 442| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/661/display/redirect

Harbormaster completed remote builds in B9344: Diff 9737.Sep 13 2019, 2:31 PM
Nescio added a subscriber: Nescio.Sep 13 2019, 2:46 PM
Nescio added inline comments.
binaries/data/mods/public/simulation/components/Health.js
441 ↗(On Diff #9737)

Shouldn't there be spaces inside the braces?
E.g. { "": "" }

Nescio removed a subscriber: Nescio.Sep 13 2019, 2:47 PM
Freagarach added inline comments.Sep 13 2019, 5:02 PM
binaries/data/mods/public/simulation/components/Health.js
439 ↗(On Diff #9737)

Usually:

if (msg.to != INVALID_PLAYER)
    do something;
Silier updated this revision to Diff 9741.Sep 13 2019, 5:26 PM
Silier edited the summary of this revision. (Show Details)

less code

Vulcan added a comment.Sep 13 2019, 5:28 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/155/display/redirect

Vulcan added a comment.Sep 13 2019, 5:33 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.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 437| 437| {
| 438| 438| 	if (msg.to != INVALID_PLAYER)
| 439| 439| 		this.OnValueModification({ "component": "Health" });
| 440|    |-}
|    | 440|+};
| 441| 441| 
| 442| 442| Health.prototype.RegisterHealthChanged = function(from)
| 443| 443| {

binaries/data/mods/public/simulation/components/Health.js
| 440| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/665/display/redirect

Harbormaster completed remote builds in B9348: Diff 9741.Sep 13 2019, 5:33 PM
Silier mentioned this in rP22767: Add a system component to handle stat modifiers, make technologies and auras….Sep 13 2019, 5:55 PM
Silier edited the summary of this revision. (Show Details)
Silier updated this revision to Diff 9783.Sep 15 2019, 1:44 PM
Silier retitled this revision from Enforce to update cache in Health component when entity is promoted to Notify renamed entities about value modifications when created.
Silier edited the summary of this revision. (Show Details)
Vulcan added a comment.Sep 15 2019, 1:47 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/703/display/redirect

Freagarach added a comment.Sep 16 2019, 9:28 AM

This works, but @wraitii suggested on IRC yesterday that the OnOwnership-messages (amongst others) should be moved to the components itself (as you did earlier).
I would agree from a speed p.o.v. and new components are not added regularly so I would say: why not?

binaries/data/mods/public/simulation/components/ModifiersManager.js
214 ↗(On Diff #9783)

.

wraitii added a comment.Sep 16 2019, 9:41 AM
In D2286#95615, @Freagarach wrote:

This works, but @wraitii suggested on IRC yesterday that the OnOwnership-messages (amongst others) should be moved to the components itself (as you did earlier).
I would agree from a speed p.o.v. and new components are not added regularly so I would say: why not?

To give a bit more detail: the only housekeeping that should be done in ModifiersManager.prototype.OnGlobalOwnershipChanged is invalidating the caches for the entities (since changing owner could change modifiers). That could be done in a cleverer way, but I don't think it is the role of this manager to send messages to other components, since those can just locally subscribe to OwnershipChanged and ask for updated values where necessary then.

So yes, I would revert to an earlier version of the patch.

Silier updated this revision to Diff 9798.Sep 16 2019, 10:00 AM
Vulcan added a comment.Sep 16 2019, 10:04 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.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 428| 428| 
| 429| 429| 	if (this.regenRate != oldRegenRate || this.idleRegenRate != oldIdleRegenRate)
| 430| 430| 		this.CheckRegenTimer();
| 431|    |-}
|    | 431|+};
| 432| 432| 
| 433| 433| Health.prototype.OnValueModification = function(msg)
| 434| 434| {
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Health.js
| 440| 440| {
| 441| 441| 	if (msg.to != INVALID_PLAYER)
| 442| 442| 		this.RecalculateValues();
| 443|    |-}
|    | 443|+};
| 444| 444| 
| 445| 445| Health.prototype.RegisterHealthChanged = function(from)
| 446| 446| {

binaries/data/mods/public/simulation/components/Health.js
| 431| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Health.js
| 443| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/712/display/redirect

wraitii accepted this revision.Sep 16 2019, 10:07 AM

I'll accept this like that, I'm not certain how to handle the ownership change in general yet, but this fixes the issue efficiently enough for now.

This revision is now accepted and ready to land.Sep 16 2019, 10:07 AM
Vulcan added a comment.Sep 16 2019, 6:34 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/187/display/redirect

Harbormaster completed remote builds in B9390: Diff 9783.Sep 16 2019, 6:34 PM
Freagarach accepted this revision.EditedSep 16 2019, 6:59 PM

Might want to change title/summary again.
Works like a charm.

This revision was landed with ongoing or failed builds.Sep 16 2019, 7:07 PM
Closed by commit rP22912: Make health correctly recalculate its cached values when the entity changes… (authored by wraitii). · Explain Why
This revision was automatically updated to reflect the committed changes.
wraitii added a commit: rP22912: Make health correctly recalculate its cached values when the entity changes….

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
simulation/
components/
17 lines

Diff 9810

View Options

ps/trunk/binaries/data/mods/public/simulation/components/Health.js

Loading...
Wildfire Games · Phabricator