Page MenuHomeWildfire Games
Differential D1694

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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6507
Build 10763: Vulcan BuildJenkins
Build 10762: arc lint + arc unit

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

!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/

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

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

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

lyv added a comment.Dec 17 2018, 5:33 AM

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

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

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

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

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

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

11

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

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

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

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
maps/
scripts/
15 lines
simulation/
components/
6 lines

Diff 7045

View Options

binaries/data/mods/public/maps/scripts/Regicide.js

Loading...
View Options

binaries/data/mods/public/simulation/components/Trigger.js

Loading...
Wildfire Games · Phabricator