HomeWildfire Games

Capture The Relic gamemode.
AuditedrP19345

Description

Capture The Relic gamemode.

Patch By: Sandarac
Differential Revision: https://code.wildfiregames.com/D152

Event Timeline

leper added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

Apart from probably invalidating tons of assumptions, why does this thing even need DamageReceiver and Health? (The latter might be due to being undeletable, but maybe that shouldn't be in Health.)

Cost also seems pointless.

elexis added inline comments.Mar 26 2017, 12:47 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

probably invalidating tons of assumptions

Like? (Review was here https://code.wildfiregames.com/D152#8930 and afterwards some settings were discussed in irc and changed too)

why does this thing even need DamageReceiver and Health

Due to our favorite file from hell.
When a player captured the catafalque, he/she should be able to move it.
Then UnitAI computes the walk speed depending on the run speed which depends on the relative health apparently:

ERROR: JavaScript error: simulation/components/UnitAI.js line 4092
TypeError: cmpHealth is null
  UnitAI.prototype.GetRunSpeed@simulation/components/UnitAI.js:4092:6
  UnitAI.prototype.SelectAnimation@simulation/components/UnitAI.js:4323:45
  UnitAI.prototype.UnitFsmSpec.INDIVIDUAL.WALKING.enter@simulation/components/UnitAI.js:1605:5
  FSM.prototype.SwitchToNextState@simulation/helpers/FSM.js:376:8
  FSM.prototype.ProcessMessage@simulation/helpers/FSM.js:284:4
  UnitAI.prototype.PushOrder@simulation/components/UnitAI.js:3742:13
  UnitAI.prototype.ReplaceOrder@simulation/components/UnitAI.js:3853:3
  UnitAI.prototype.AddOrder@simulation/components/UnitAI.js:4951:3
  UnitAI.prototype.Walk@simulation/components/UnitAI.js:5056:3
  g_Commands.walk/<@simulation/helpers/Commands.js:145:4
  g_Commands.walk@simulation/helpers/Commands.js:144:3
  ProcessCommand@simulation/helpers/Commands.js:47:3

Implementing some dummy component could be done, but agreeing with Sandarac that this should be done outside of this proposal (lesson learnt from polar sea, no?)

Cost also seems pointless.

Indeed.

The current filepath seems wrong, because something inheriting template_unit should start with template_unit, not other/somethingelse.
We intend to add one catafalque template per civ, giving civ specific names, descriptions, bonuses / auras and thus also allow civ specific models.
How about template_unit_catafalque.xml (since I don't see a use for template_unit_special.xml) and then templates/units/{civ}_catafalque.xml?

elexis added inline comments.Mar 26 2017, 12:55 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

The new templates could also have casual health (to allow reuse) and implement an invincibility special filter used by the trigger script (which would also be used for the treasure seeker woman on survival of the fittest) as was intended by D230.

leper added inline comments.Mar 26 2017, 1:26 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

Like those in patches like the one that causes that missing Health breakage. Which sounds a lot like a broken attempt at fixing that one bug with fleeing units not being damaged... (r14517 that is)

So I'd be for fixing that broken code (or reevaluating whether we really want running speed to depend on health (and not just see health as an abstraction...)) code by not assuming that that is present there, and then dropping that from the template.

I'm against dummy components, especially if those we have can't deal with unrelated ones not being present (as opposed to healing not making sense when the healed thing has no health).

No issue with the path, but I'm not sure if putting special templates only useful for a single game mode next to actual game templates makes sense, or if we should just keep them in another folder to better group them.

elexis added inline comments.Mar 26 2017, 3:03 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

rP14517 (mind the P, which should also cause insertion of a reference to this document in that commit) doesn't look that bad to me. An early return if healt isn't defined gets rid of that specific stack. Nuking health is also good because it means that the health bar won't be shown anymore!

Still expecting the unknown when disabling components. Bit more testing of random functions for example reveals that one can't capture the thing anymore if cost is disabled (No cost ref found in UnitAI, nor Attack, nor Capturable)

leper added inline comments.Mar 26 2017, 5:52 AM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

Are you sure the Capture thing isn't caused by removing Health? IIRC there is some code that reduces the capture regen based on health.

Not having certain components should be supported.

Slightly unrelated but possibly supporting different win times based on how the victory is achieved might be nice at some point in the future (that is possibly open a ticket about it if that is considered useful).

elexis added inline comments.Mar 26 2017, 6:34 PM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

sure the Capture thing isn't caused by removing Health

It was caused by removing health, I tested incorrectly again (missing ctrl+s?), so disabling that next time I touch the template (likely when adding the civ specific template to a new special subdirectory).

supporting different win times based on how the victory is achieved

True, the endgame manager should store a different victory time per victory condition once we have #4014. And forum people ordered different defaults. Those should be numbers not dropdowns in the first place (also ticket somewhere).

why does this thing even need DamageReceiver

This line in UnitAI makes units not pickup ents without DamageReceiver in the range queries, thus not auto-capturing the thing, even if the surrounding code is fixed. Sounds like a performance gain we might not want to lose.

this.losRangeQuery = cmpRangeManager.CreateActiveQuery(this.entity, range.min, range.max, players, IID_DamageReceiver, cmpRangeManager.GetEntityFlagMask("normal"));

Also slightly offtopic, but we must teach UnitAI and BuildingAI to ignore invincible units.
For survival it was useful because the treasure picking woman could be stopped from picking up the treasure.
But for this gamemode, the relic draws arrowfire and elephants attacking it, as reported on the forums by our fans.
For promotion it has also been relevant as the cheering unit draws important arrowfire.

Something like this. Could also just keep the things template_unit entities are assumed to have.


Patch incomplete, something off with unitAI and rangemanager, relic sometimes picked up in queries, other times not.
Probably still many places in the code to change.

leper added inline comments.Mar 27 2017, 9:45 PM
/ps/trunk/binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
2

This line in UnitAI makes units not pickup ents without DamageReceiver in the range queries, thus not auto-capturing the thing, even if the surrounding code is fixed. Sounds like a performance gain we might not want to lose.

this.losRangeQuery = cmpRangeManager.CreateActiveQuery(this.entity, range.min, range.max, players, IID_DamageReceiver, cmpRangeManager.GetEntityFlagMask("normal"));

Which just shows that the attack code for capturing is broken. It should look for DamageReceivers if we want to damage them and Capturable if we want to capture. There is no performance gain here as there as we could just return the lowest entity id owned by some player, that would be even faster (but also broken).

Also slightly offtopic, but we must teach UnitAI and BuildingAI to ignore invincible units.

Might be as simple as removing them from a few range queries by tweaking some flags.

For survival it was useful because the treasure picking woman could be stopped from picking up the treasure.

Well that is using something for slightly unsupported purposes (but as I have both recommended doing that, and done that myself) we probably should fix that someday.

For promotion it has also been relevant as the cheering unit draws important arrowfire.

Well, that can be either solved by having players micro, or just wasting some time and attack strength if they don't.

Grugnas raised a concern with this commit.Apr 7 2017, 11:46 PM
Grugnas added a subscriber: Grugnas.
Grugnas added inline comments.
/ps/trunk/binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
177

Relic Victory Condition Timer shouldn't reset everytime diplo changes.

This commit now has outstanding concerns.Apr 7 2017, 11:46 PM
Sandarac added inline comments.
/ps/trunk/binaries/data/mods/public/maps/scripts/CaptureTheRelic.js
177

Right, it shouldn't reset for every diplomacy change. D305 will try to address this.

Grugnas accepted this commit.Jun 26 2017, 3:13 PM
All concerns with this commit have now been addressed.Jun 26 2017, 3:13 PM