Page MenuHomeWildfire Games

Handle entity renaming properly in regicide mode
ClosedPublic

Authored by smiley on Dec 16 2018, 4:58 PM.

Details

Summary

Currently, if a hero entity were renamed as in when promoting and packing, the player would be defeated as the stored hero ent would be dead. Now, if a hero entity were to be renamed, the list would be updated.

Test Plan

Add a promotion component to a hero and promote it in a regicide match

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

smiley created this revision.Dec 16 2018, 4:58 PM
Owners added a subscriber: Restricted Owners Package.Dec 16 2018, 4:58 PM
smiley added inline comments.Dec 16 2018, 5:00 PM
binaries/data/mods/public/maps/scripts/Regicide.js
18 ↗(On Diff #7045)

!playerID would probably suffice I guess. Unlikely -1 would ever get involved here. But as I wasn't sure and really haven't checked, left it as is.

Vulcan added a subscriber: Vulcan.Dec 16 2018, 5:01 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/Regicide.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/Regicide.js
|  10|  10| {
|  11|  11| 	if (this.regicideHeroes.indexOf(data.entity) == -1)
|  12|  12| 		return;
|  13|    |-	
|    |  13|+
|  14|  14| 	let cmpOwnership = Engine.QueryInterface(data.entity, IID_Ownership);
|  15|  15| 	if (cmpOwnership)
|  16|  16| 	{
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'action'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 186| 186| 		}
| 187| 187| 		let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
| 188| 188| 		data.timer = cmpTimer.SetInterval(this.entity, IID_Trigger, "DoAction",
| 189|    |-			data.delay || 0, data.interval, { "action" : action });
|    | 189|+			data.delay || 0, data.interval, { "action": action });
| 190| 190| 	}
| 191| 191| 	else if (event == "OnRange")
| 192| 192| 	{
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'data'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 223| 223| 
| 224| 224| 	for (let action in this[eventString])
| 225| 225| 		if (this[eventString][action].enabled)
| 226|    |-			this.DoAction({ "action": action, "data":data });
|    | 226|+			this.DoAction({ "action": action, "data": data });
| 227| 227| };
| 228| 228| 
| 229| 229| Trigger.prototype.OnGlobalInitGame = function(msg)

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

smiley retitled this revision from Handle entity renaming probably in regicide mode to Handle entity renaming properly in regicide mode.Dec 16 2018, 5:08 PM
smiley edited the summary of this revision. (Show Details)Dec 16 2018, 5:19 PM
smiley edited the summary of this revision. (Show Details)Dec 16 2018, 5:22 PM
elexis added a subscriber: elexis.Dec 17 2018, 12:39 AM

refs rP18544

Is there a relevant mod already that renames heroes?

(Having multiple heroes is possible with cheats, and it's not so far fetched for mods. But only 2 lines of this patch have to be changed then, it can be done separately.)

binaries/data/mods/public/maps/scripts/Regicide.js
9 ↗(On Diff #7045)

the other names use the FooRegicideBar pattern

(Refs an abandoned revision proposal for splitting Trigger extensions, as they currently overwrite the same prototype, a map and regicide both tracking heroes can easily lead to name conflicts)

18 ↗(On Diff #7045)

(this.regicideHeroes[playerID] !== undefined also a possibility in such cases, but > 0 seems fine and indeed safer than !x)

Is there a relevant mod already that renames heroes?

DE got a hero which can mount and unmount. HC also got a hero which kinda does the same thing. Although, a promotable hero doesnt really sound far fetched even for vanilla.

bb added a subscriber: bb.Dec 26 2018, 5:31 PM
bb added inline comments.
binaries/data/mods/public/maps/scripts/Regicide.js
9 ↗(On Diff #7045)

Proposing RenameRegicideHero

smiley marked an inline comment as done.Dec 27 2018, 5:34 AM
smiley added inline comments.
binaries/data/mods/public/maps/scripts/Regicide.js
9 ↗(On Diff #7045)

Go ahead with the rename. Assumed it’s small enough to be done while commiting.

smiley updated this revision to Diff 7210.Jan 3 2019, 8:35 AM

Rename the function to what @bb proposed.

Vulcan added a comment.Jan 3 2019, 8:37 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'action'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 186| 186| 		}
| 187| 187| 		let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
| 188| 188| 		data.timer = cmpTimer.SetInterval(this.entity, IID_Trigger, "DoAction",
| 189|    |-			data.delay || 0, data.interval, { "action" : action });
|    | 189|+			data.delay || 0, data.interval, { "action": action });
| 190| 190| 	}
| 191| 191| 	else if (event == "OnRange")
| 192| 192| 	{
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'data'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 223| 223| 
| 224| 224| 	for (let action in this[eventString])
| 225| 225| 		if (this[eventString][action].enabled)
| 226|    |-			this.DoAction({ "action": action, "data":data });
|    | 226|+			this.DoAction({ "action": action, "data": data });
| 227| 227| };
| 228| 228| 
| 229| 229| Trigger.prototype.OnGlobalInitGame = function(msg)
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/Regicide.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/Regicide.js
|  10|  10| {
|  11|  11| 	if (this.regicideHeroes.indexOf(data.entity) == -1)
|  12|  12| 		return;
|  13|    |-	
|    |  13|+
|  14|  14| 	let cmpOwnership = Engine.QueryInterface(data.entity, IID_Ownership);
|  15|  15| 	if (cmpOwnership)
|  16|  16| 	{
Executing section cli...

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

Stan added reviewers: bb, elexis.Jan 3 2019, 8:38 AM
bb added a comment.Jan 4 2019, 3:53 PM

Excuses for not replying to previous comments, and making you do an extra iteration...

binaries/data/mods/public/maps/scripts/Regicide.js
9 ↗(On Diff #7210)

I really wonder why that check function is above the init function, I would move both down (unrelated to this patch though)

11 ↗(On Diff #7210)

so we already know the index here, thus we could use that instead of finding the owner and stuff...

smiley marked an inline comment as done.Jan 4 2019, 4:10 PM
smiley added inline comments.
binaries/data/mods/public/maps/scripts/Regicide.js
11 ↗(On Diff #7210)

Ah indeed. This one got in a previous state where I had added support for multiple "heroes".

smiley updated this revision to Diff 7240.Jan 4 2019, 4:15 PM
Vulcan added a comment.Jan 4 2019, 4:34 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'action'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 186| 186| 		}
| 187| 187| 		let cmpTimer = Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer);
| 188| 188| 		data.timer = cmpTimer.SetInterval(this.entity, IID_Trigger, "DoAction",
| 189|    |-			data.delay || 0, data.interval, { "action" : action });
|    | 189|+			data.delay || 0, data.interval, { "action": action });
| 190| 190| 	}
| 191| 191| 	else if (event == "OnRange")
| 192| 192| 	{
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'data'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Trigger.js
| 223| 223| 
| 224| 224| 	for (let action in this[eventString])
| 225| 225| 		if (this[eventString][action].enabled)
| 226|    |-			this.DoAction({ "action": action, "data":data });
|    | 226|+			this.DoAction({ "action": action, "data": data });
| 227| 227| };
| 228| 228| 
| 229| 229| Trigger.prototype.OnGlobalInitGame = function(msg)
Executing section cli...

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

bb accepted this revision.Jan 4 2019, 9:00 PM

Tested against DE, correctness forces me to press accept

in some discussion on irc, the wonder was mentioned as related: what happens there on a entity rename is that the timer gets reset, which probably is wrong and will need a fix

This revision is now accepted and ready to land.Jan 4 2019, 9:00 PM
This revision was automatically updated to reflect the committed changes.