Page MenuHomeWildfire Games

Move undeletable from health to identity
ClosedPublic

Authored by temple on Apr 16 2017, 7:00 AM.

Details

Summary

Old title: "Allow units without Health component and use it for relics", and summary:

As mentioned in rP19345, it should be possible to have units without a Health component.
This is in particular relevant for units which are supposed to be indestructable, so having healthpoints if they can never change would be pointless.

The diff moves the Undeletable property from Health to Identity, because it should be possible to mark entities as undeletable without Health component too.

The SetUndeletable method was deleted, because changing template properties is not well supported with AIs (the change would have to be pulled and hardcoded through the GUIInterface).
(For the same reason a template approach was favored for D104.)

The treasure seeker woman on survival is marked as undeletable with a new special filter template (refs #2951, D215).
(This special filter template will be renamed to indestructable and extended to remove the DamageReceiver component once we fix the AI for D230)

The updatePanelEntityHealthBar needs an early return and in the same turn removes some pointless unused option to show the Health point bar vertically or inverted horizontally.
(The player might expect the capture bar in the top panel, refs D289, but out of scope).

The undeletable property was added to Health in rP17408 fixing #3590 (https://github.com/0ad/0ad/commit/55cfb72a3231b3058063b13975c75e4e9f2aa2fb)

Test Plan

New test: Check that units no longer attack undeletable units: starting women on Survival of the Fittest and relics. (Rally points show the attack cursor, but I don't feel that's worth fixing at this point.) Check that petra's slightly broken and offer suggestions.

Old test:
Capture a relic using the "wololo" cheat. Train a hero quickly using the "iwanttopwnthem" cheat.
Start an AI match, give the AI a relic, attack it and see that it's not bugging (contrary to D230).
Try hunting chicken.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Vulcan added a subscriber: Vulcan.Apr 16 2017, 9:09 AM

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/782/ for more details.

The SetUndeletable method was deleted, because changing template properties is not well supported with AIs (the change would have to be pulled and hardcoded through the GUIInterface).
(For the same reason a template approach was favored for D104.)

Hm, tbh I don't like this. Marking entities as undeletable for triggers would be pretty damn useful.

In D341#13592, @wraitii wrote:

The SetUndeletable method was deleted, because changing template properties is not well supported with AIs (the change would have to be pulled and hardcoded through the GUIInterface).
(For the same reason a template approach was favored for D104.)

Hm, tbh I don't like this. Marking entities as undeletable for triggers would be pretty damn useful.

You might want to read the patch, especially the survival triggers part. The only thing that isn't possible anymore afterwards is changing it back and forth. If that is needed, it should be implemented on the AI side too.

You might want to read the patch, especially the survival triggers part. The only thing that isn't possible anymore afterwards is changing it back and forth. If that is needed, it should be implemented on the AI side too.

Well if I read the patch correctly it's now a template-defined (in Identity) parameter. Which is kind of annoying still, I don't think lack of AI support should necessarily mean this feature is removed, it could be useful for MP-only custom scenarios (say an RPG-like scenario) or what have-you.

sanderd17 added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
536

If there's a Health component defined, but the hitpoints are 0, the code will continue unlike the previous code. I don't think this is wanted, as it could cause two ownership changed messages in the same turn which might result in problems.

binaries/data/mods/public/simulation/components/UnitAI.js
4111

This check isn't obvious. Being capturable has nothing to do with being alive.

elexis added inline comments.Apr 18 2017, 1:26 AM
binaries/data/mods/public/simulation/components/Attack.js
536

How would this occur?

The capture points are independent of the health points, so if the capture rate is so high that 2 different players can capture the object in the same turn, this would have happened before too, independent of the health?

binaries/data/mods/public/simulation/components/UnitAI.js
4111

Depends on what we understand under alive (can be attacked?).
Probably will end up rewriting the 20 places using that function :-l

sanderd17 added inline comments.Apr 19 2017, 7:52 AM
binaries/data/mods/public/simulation/components/Attack.js
536

It's not about 2 players capturing it, it's about 1 player capturing it (so changing the ownership to that player) while another it's also being destroyed (so changing ownership to -1).

Since this change, it can happen on the same turn I'd guess. In any case, it's something that needs to be investigated (also wrt updating the correct stats like housing space).

bb requested changes to this revision.Apr 20 2017, 4:52 PM
bb added a subscriber: bb.

while at it also add a check in attack.CanAttack for the undeletable flag.

binaries/data/mods/public/simulation/components/GuiInterface.js
281

trailling comma

binaries/data/mods/public/simulation/components/UnitAI.js
4111

Indeed capturing has not much todo with living, a corpse can be captured too...
CanBeAttacked is wrong too since the function is also used for returning resources, healing etc.

Perhaps check for cmpOwnership.GetOwner() != -1 and cmpVisibility.corpse too (or have some bool in Identity defining living).

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
578

The file has trailling comma's all over the place, so either remove them or add one here too.

binaries/data/mods/public/simulation/helpers/Commands.js
415

don't think you want to return here, we are in a loop right? => continue

binaries/data/mods/public/simulation/templates/other/special_catafalque.xml
15 ↗(On Diff #1273)

We break some assumptions in the damageReciever here. Being able to receive damage without having health isn't really possible. So when removing the health component the damageReceiver/Armour component needs to be removed too.

This revision now requires changes to proceed.Apr 20 2017, 4:52 PM
temple commandeered this revision.Nov 19 2017, 1:39 AM
temple updated this revision to Diff 4272.
temple added a reviewer: elexis.

Just rebased, not ready for review.

Build failure - The Moirai have given mortals hearts that can endure.

svn: E135001: Unrecognized line ending style 'native
\ No newline at end of property' for '/mnt/data/jenkins-phabricator/workspace/phabricator/binaries/data/mods/public/simulation/templates/special_filter/undeletable.xml'

Haven't thought it through completely, but I think maybe we should separate melee/ranged attacks from capture attacks, i.e. make a capture component.

Speaking of things bb has some related patch for - it was thought of to make capturing a secondary attack #252. A capturing component doesn't sound ugly too though. Would indeed require thinking through and lookup of code uses in order to estimate if it will make things easier or more complicated.

About the patch, if I'm not mistaken, moving the undeletable part from Health to Identity can be split from the Health removal. Since the AI now supports new special templates well, perhaps it can even be committed if the patch was and still is correct.
(wraitiis use case of making entities undeletable on the fly could be done by replacing the entity with a different one having the "undeletable|templateName" template.)

bb added a comment.Nov 19 2017, 9:24 PM

Was actually thinking more the other way around in #252: merge the code handling damage and capture. The problem of having an own capture component is that units then will be able to capture and damage at the same time (and making the components work together to avoid that will lead to bugs as #4000 in other components). And I guess that is not what we want to happen. One could better use cmpAttack.CanAttack() to handle attacking non-health units.

In D341#41422, @bb wrote:

Was actually thinking more the other way around in #252: merge the code handling damage and capture. The problem of having an own capture component is that units then will be able to capture and damage at the same time (and making the components work together to avoid that will lead to bugs as #4000 in other components). And I guess that is not what we want to happen. One could better use cmpAttack.CanAttack() to handle attacking non-health units.

The issue with making capture an attack (as has been done) is that it is not really one. Attacks deal damage, capture does not. It does share quite a few things, which I guess why it was made one, yet there is still that issue with lots of code assuming that a target needs health and (possibly armour). So we already have a situation similar to #4000 where we also have two similar but not equal things done in one place becaused it seemed like an easy way. However this here doesn't seem as bad as #4000.

Not making it an attack would require some changes to UnitAI that might not be too large.

temple updated this revision to Diff 4279.Nov 20 2017, 2:39 AM
temple retitled this revision from Allow units without Health component and use it for relics to Move undeletable from health to identity.
temple edited the summary of this revision. (Show Details)
temple edited the test plan for this revision. (Show Details)

Move undeletable from health component to identity. Also add a check in CanAttack so now elephants will no longer try to attack relics.

As elexis suggested, removing the health component can go in another diff. I think probably we should split up capture from the other attacks first, otherwise it seems too big of a headache to get everything working correctly, e.g. the TargetIsAlive function in UnitAI.

I sent a relic into petra's base, and he ignored it for a while, then attacked it, and only later tried capturing. So there's some work to be done there.

temple added inline comments.Nov 20 2017, 2:42 AM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

I don't understand why this wasn't miraged like the others? Guess I'm not clear on how the miraging stuff works exactly.

bb requested changes to this revision.Nov 25 2017, 9:23 PM

Summarizing what we want and have:

  • With this patch we move the undeletable thing to identity, which is good, since ents without health could probably still be deletable. IMO undeletable should define if a user can delete the entity.
  • The invulnerable tag in the damage component now defines if the entity can take damage, as we want such a tag to be dynamical settable (promotion) it is good to have such a thing (this actually works how it should work, unit can be attacked, but just ignores that)
  • Besides those in future we want units without a health component which simply cannot be attacked with damage (so only capture). This is in a way the logical thing todo with those units, since there is nothing to attack.

Seeing this, the idea of the patch is indeed good and it is a pre-req for removing the health component from some units.

binaries/data/mods/public/simulation/components/Attack.js
256

Seeing my conciderations on top, this should become a !cmpHealth check (which won't work atm.), however the proposed check is wrong and shouldn't be added

binaries/data/mods/public/simulation/components/Identity.js
90

why is this optional?

170

The thing was mostly stored in case it was changed afterwards, could get the thing directly from the template now

binaries/data/mods/public/simulation/helpers/Commands.js
391–393

I guess since the identity values, are not supposed to change in time, so miraging is just useless, however one should test if some undeletable ent (read foundation) is still undeletable outside FoW

This revision now requires changes to proceed.Nov 25 2017, 9:23 PM
temple marked 4 inline comments as done.Nov 26 2017, 2:51 AM
temple added inline comments.
binaries/data/mods/public/simulation/components/Identity.js
90

Hmm. Looking through, I see special/player.xml has an identity element, same for special/spy.xml (neither have health). I'm not familiar with either of those, does it make sense for them to have an undeletable element?

wraitii added a comment.EditedNov 26 2017, 9:32 AM

The invulnerable tag in the damage component now defines if the entity can take damage, as we want such a tag to be dynamical settable (promotion) it is good to have such a thing (this actually works how it should work, unit can be attacked, but just ignores that)

Don't you mean invulnerable should go in the damage_receiver (i.e. armour) component?

I agree with making undetectable an Identity thing though, makes sense to me.

binaries/data/mods/public/simulation/components/Identity.js
90

player.xml is the "player" entity, i.e. it has your technology manager, aura manager, and a bunch of "per-player" things. Also defines the civ of a player and so on.

It probably makes sense for player.xml to be undetectable, though in vanilla SVN you can't really delete your own entity anyways.

spy.xml I assume is related to the spying feature, and I suppose leaving it deletable could be useful for gameplay (imagine that it costs a trickle of gold, you could stop that by deleting it?) but I'm really not sure.

temple added inline comments.Nov 27 2017, 11:30 PM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

Hmm. That makes sense, I think.
But identity is a miraged component, with GetClassesList used in CanAttack, why do we have that if it doesn't change?

bb added inline comments.Nov 28 2017, 2:20 PM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

Maybe I was wrong, read the code now more carefully:
A mirage creates a new entity with some (data but not all) in the mirage component, Further the mirage has just a couple of components (position and stuff AFAIK) thus the mirage has no Identity component (one could test with some foundation perhaps). So I guess things will break for mirages with the undeletable tag. So we will have to add this data to the mirage and use that here.

the identity component mirage was added by D750/rP19987 (maybe fix that comment while at it)

397

However, when looking out this code, the might be a potential abuse here (only the circumstances can't be achieved in current svn):

  • Have a capturable building, that can be miraged (this is the problem why it is impossible in current svn)
  • Mirage it
  • loose >50% but <100% of the capture points (this data won't come into the mirage!!!)
  • delete the building (That works now!!, while it shouldn't)

This can be fixed by checking for the mirage parent too, only that would allow the player to see that his building is more than half captured by an enemy through the FoW, so partially takes away the thing of "fogging".

IMO this is out of scope, but might need a discussion (potentially it could happen to the identity things too)

wraitii added inline comments.Nov 28 2017, 5:51 PM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

Is this super relevant? Miraged entities are presumably foes, which tend to be undetectable regardless.

Though changing classes we might want to do in the future, so it might be worth doing anyways.

bb added inline comments.Nov 28 2017, 5:56 PM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

classes are relevant at least: It is not about the fact it changes in time (was wrong about that in previous post). But the mirage ent doesn't have an identity component!!, so we need to store the data in the mirage for displaying the correct tooltip at least (see the revision adding that). And as f.e. foundations could be undeletable, that should be stored in the mirage at least

temple added inline comments.Nov 28 2017, 6:06 PM
binaries/data/mods/public/simulation/helpers/Commands.js
391–393

The miraged entity does have an identity component, but for some reason it doesn't have the classes. We can skip adding that to the mirage component if we just add all of identity to the mirage filter. I'll upload a patch.

397

Another out of scope thing I noticed: When an enemy destroys our foundation in the fog of war, we'll see the rubble. It seems either we shouldn't see the rubble, or the foundation should also disappear. (We won't see rubble if an enemy deletes his own building in the fog of war.)

temple updated this revision to Diff 4433.Nov 29 2017, 5:36 AM

Made undeletable mandatory. I don't have a systematic way of checking templates so maybe I missed some.
Included the changes from D1078 since it covered the same files, but that one should be done first.

In D341#41372, @Vulcan wrote:

Build failure - The Moirai have given mortals hearts that can endure.

svn: E135001: Unrecognized line ending style 'native
\ No newline at end of property' for '/mnt/data/jenkins-phabricator/workspace/phabricator/binaries/data/mods/public/simulation/templates/special_filter/undeletable.xml'

I thought I had been doing something wrong with the undeletable.xml file, but actually this is the old directory, so I don't know what Vulcan's doing.
I guess in history the base revision is wrong? I don't see how to change that though.

temple updated this revision to Diff 4442.Nov 29 2017, 5:23 PM
temple set the repository for this revision to rP 0 A.D. Public Repository.

Undid the D1078 change.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Nov 29 2017, 5:23 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
temple updated this revision to Diff 4447.Nov 29 2017, 9:11 PM

Systematic template check, had missed one.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Systematic template check, had missed one.

In case you don't know this: you can run the demo scenario "unit test" which will is supposed to load all templates.

In D341#43129, @wraitii wrote:

Systematic template check, had missed one.

In case you don't know this: you can run the demo scenario "unit test" which will is supposed to load all templates.

Thanks for the reminder. Although in this case it still misses the one I missed (template_structure_gaia_settlement.xml). Instead I adapted tools/templatesanalyzer to just look at identity and undeletable.

bb accepted this revision.EditedDec 3 2017, 6:14 PM

these two issues are easy to fix when committing

binaries/data/mods/public/simulation/components/Mirage.js
18

thx, relieving me from fixing my own concern :P

binaries/data/mods/public/simulation/components/tests/test_Attack.js
109 ↗(On Diff #4447)

looks like an artifact

binaries/data/mods/public/simulation/helpers/Commands.js
418

we already know the is no mirage, so don't request the miraged interface but the normal one

This revision is now accepted and ready to land.Dec 3 2017, 6:14 PM
temple added inline comments.Dec 3 2017, 7:10 PM
binaries/data/mods/public/simulation/components/tests/test_Attack.js
109 ↗(On Diff #4447)

yes, unnecessary

binaries/data/mods/public/simulation/helpers/Commands.js
418

right

This revision was automatically updated to reflect the committed changes.
elexis updated the Trac tickets for this revision.Jan 28 2018, 4:31 PM