Better relic error handling for edge cases
ClosedPublic
Actions

Authored by elexis on Apr 1 2017, 5:11 PM.

Details

Reviewers

Commits

rP19378: Handle relics that are destroyed for any reason and maps that have no gaia…

Summary

As reported by fatherbushido, there is a "New RMS Test" map that does nothing besides placing civic centers.
There map was introduced for #6 in rP9096 and there is exactly no reason to keep it, so lets delete it.
If this map, or any other (for example atlas test) map without gaia entities, i.e. without spawn points was started in capture the relic victory gamemode,
there would be some unreadable error messages. Instead throw a readable message and add an early return.

Also relics can become destroyed for any reason, for example by some trigger script like D229 or when using the developer cheat to control all units.
The code should handle that case gracefully, show a warning instead of an error and restart the victory timer properly.

Test Plan

Start the new RMS demo map without the patch, see it bug. Apply the CaptureTheRelic.js patch and see it being handled gracefully.
Start a regular map with the victory condition, use the "wololo" cheat to capture all but one relics, use Alt+D to control all units and delete the last one.
See that the timer is started. Delete all relics and see that there is no further error.

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

elexis created this revision.Apr 1 2017, 5:11 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/676/ for more details.

Harbormaster completed remote builds in B986: Diff 1051.Apr 1 2017, 5:56 PM

This diff only modifies CaptureTheRelic.js.

This revision now requires changes to proceed.Apr 4 2017, 4:36 AM

Coincidentally this mistake ended up with the right patch. At least fatherbushido wants to keep that map and stated that there was a guy playing only that one in deathmatch mode.
Not entirely convinced, perhaps the map should have at least some visual actors and become round. The map would have never been accepted if someone proposed it and probably has no right to exist in anything calling itself beta. But calling that out of scope for now.

This revision now requires changes to proceed.Apr 4 2017, 5:05 AM

elexis requested review of this revision.Apr 4 2017, 5:05 AM

elexis retitled this revision from Better relic error handling for edge cases, nuke empty map to Better relic error handling for edge cases.

This revision now requires changes to proceed.Apr 4 2017, 5:06 AM

In D283#11426, @elexis wrote:

Coincidentally this mistake ended up with the right patch. At least fatherbushido wants to keep that map and stated that there was a guy playing only that one in deathmatch mode.
Not entirely convinced, perhaps the map should have at least some visual actors and become round. The map would have never been accepted if someone proposed it and probably has no right to exist in anything calling itself beta. But calling that out of scope for now.

Ah, okay. But I do think that "map" should be modified or just removed at some point.

The diff works as expected.

This revision is now accepted and ready to land.Apr 4 2017, 6:51 AM

In D283#11426, @elexis wrote:

Coincidentally this mistake ended up with the right patch. At least fatherbushido wants to keep that map and stated that there was a guy playing only that one in deathmatch mode.
Not entirely convinced, perhaps the map should have at least some visual actors and become round. The map would have never been accepted if someone proposed it and probably has no right to exist in anything calling itself beta. But calling that out of scope for now.

That deatmatch stuff was more for the funny story. I am not convinced either by keeping it or removint it. That's just a map I used really often for testing stuff. If there is a strong reason to remove it, let's do it.

when an AI has captured a relic and that relic is destroyed, there will be an AI error spam:

ReferenceError: evt is not defined
  m.GameTypeManager.prototype.removeGuardsFromCriticalEnt@simulation/ai/petra/gameTypeManager.js:272:2
  m.GameTypeManager.prototype.checkEvents@simulation/ai/petra/gameTypeManager.js:173:4
  m.GameTypeManager.prototype.update@simulation/ai/petra/gameTypeManager.js:494:2
  m.HQ.prototype.update@simulation/ai/petra/headquarters.js:2286:2
  m.PetraBot.prototype.OnUpdate@simulation/ai/petra/_petrabot.js:125:3
  m.BaseAI.prototype.HandleMessage@simulation/ai/common-api/baseAI.js:71:2
ERROR: JavaScript error: simulation/ai/petra/gameTypeManager.js line 272

The same error is triggered when capturing a relic from an AI with wololo cheat (didn't test with normal capture).

When a player has all relics left (and that is more than 1) and one of them is destroyed, the timer is restarted, but he still has all relics, dunno how this should be handled.

In D283#11469, @bb wrote:
when an AI has captured a relic and that relic is destroyed, there will be an AI error spam:
ReferenceError: evt is not defined
  m.GameTypeManager.prototype.removeGuardsFromCriticalEnt@simulation/ai/petra/gameTypeManager.js:272:2
  m.GameTypeManager.prototype.checkEvents@simulation/ai/petra/gameTypeManager.js:173:4
  m.GameTypeManager.prototype.update@simulation/ai/petra/gameTypeManager.js:494:2
  m.HQ.prototype.update@simulation/ai/petra/headquarters.js:2286:2
  m.PetraBot.prototype.OnUpdate@simulation/ai/petra/_petrabot.js:125:3
  m.BaseAI.prototype.HandleMessage@simulation/ai/common-api/baseAI.js:71:2
ERROR: JavaScript error: simulation/ai/petra/gameTypeManager.js line 272
The same error is triggered when capturing a relic from an AI with wololo cheat (didn't test with normal capture).

When a player has all relics left (and that is more than 1) and one of them is destroyed, the timer is restarted, but he still has all relics, dunno how this should be handled.

Ah, this is actually because of a copy-and-paste error in rP19356, and is unrelated to this diff.

Closed by commit rP19378: Handle relics that are destroyed for any reason and maps that have no gaia… (authored by elexis). · Explain WhyApr 5 2017, 6:14 AM

This revision was automatically updated to reflect the committed changes.

Sandarac mentioned this in D229: Extinct Volcano (Rising Water map).Apr 23 2017, 12:00 AM

Revision Contents
Changeset List

Path

Size

ps/

trunk/

binaries/

data/

mods/

public/

maps/

scripts/

CaptureTheRelic.js

20 lines

Diff 1101

View Options

ps/trunk/binaries/data/mods/public/maps/scripts/CaptureTheRelic.js

Better relic error handling for edge casesClosedPublicActions