Paths

Table of Contentst

Handle entity renaming properly in regicide mode
ClosedPublic
Actions

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

Details

Reviewers

bb
elexis

Commits

rP22019: Handle entity renames in regicide

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

lyv created this revision.Dec 16 2018, 4:58 PM

Owners added a subscriber: Restricted Owners Package.Dec 16 2018, 4:58 PM

lyv 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.

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/

Harbormaster completed remote builds in B6507: Diff 7045.Dec 16 2018, 5:01 PM

lyv 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

lyv edited the summary of this revision. (Show Details)Dec 16 2018, 5:19 PM

lyv edited the summary of this revision. (Show Details)Dec 16 2018, 5:22 PM

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`

lyv marked an inline comment as done.Dec 27 2018, 5:34 AM

lyv 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.

Rename the function to what @bb proposed.

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/

Harbormaster completed remote builds in B6671: Diff 7210.Jan 3 2019, 8:37 AM

Stan added reviewers: bb, elexis.Jan 3 2019, 8:38 AM

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

bb mentioned this in D1690: Fix Danubius ownershipchange subscription following rP21445.Jan 4 2019, 4:00 PM

lyv marked an inline comment as done.Jan 4 2019, 4:10 PM

lyv 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".

lyv updated this revision to Diff 7240.Jan 4 2019, 4:15 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/

Harbormaster completed remote builds in B6697: Diff 7240.Jan 4 2019, 4:34 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

Closed by commit rP22019: Handle entity renames in regicide (authored by bb). · Explain WhyJan 4 2019, 9:09 PM

This revision was automatically updated to reflect the committed changes.

bb mentioned this in D1732: Wondervictory entity rename.Jan 5 2019, 8:54 PM