#999 closed enhancement (fixed)
[PATCH] Healing
Reported by: | historic_bruno | Owned by: | leper |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 10 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
The Heal Action is used by the Healer unit to regenerate the health of the player's organic units. While the action is applied to a viable damaged target, his hitpoints increase until restored to maximum, while the entity performs his "Heal" animation.
Note that as with most actions, multiple entities can be tasked to the same target to speed up the rate of regeneration.
Attachments (11)
Change History (38)
comment:1 by , 12 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
by , 12 years ago
Attachment: | #999-2012-02-23.patch added |
---|
comment:2 by , 12 years ago
Keywords: | patch review added |
---|---|
Summary: | Healing → [PATCH] Healing |
Adds a button to the healer units. Click it and move over the target and click again.
There are still a lot of TODOs in the code (some because of not yet present files). It should allow for a rather generic Ability panel (there are some TODOs referring that) but that part isn't fully implemented and I will probably need to implement another ability to test if it works.
All needed files or modifications that are needed that I would prefer if someone else (eg: Draw some buttons, create an animation, sounds) could do this:
- "heal" animation for all units with class Healer
Maybe fix the action-heal cursor as the current one doesn't have transparency.A variation of said cursor for action-heal-disabled- A good looking session/portraits/abilities/heal.png
- order_heal sound
- heal_impact sound
I would be glad if someone could review this and tell me if this approach is good and maybe has some suggestions.
by , 12 years ago
comment:3 by , 12 years ago
Milestone: | Backlog → Alpha 9 |
---|
comment:4 by , 12 years ago
Just a recap of the changes needed:
Don't have a separate Range for healing. Use the units range. (else it'll just be duplicating data)(see next comment)Rename <Speed> to <Amount>(see next comment)Add <Delay> which increases delay between healings(see next comment)Add leveling abilities with different speeds/amount per rank (see schema below)(see next comment)- Healing units should find any injured units within their range and auto heal them
- Enable ally player healing
- Make file indentation consistent with the rest of the file
comment:5 by , 12 years ago
Priority: | Should Have → Must Have |
---|
After discussion with Mythos and historicbruno, it's best to reuse the Loot and Promotion systems. Some XML element names have changed too. Range is back, and a new XML element, Rate, has been added. Be sure to use as much existing code as possible. These changes should make the healing system very flexible.
So each unit would have a <Loot>, for e.g. a spearman might have:
<Loot> <xp>100</xp> </Loot>
Then a healer with three ranks (b = basic, a = advanced, e = elite) would have:
<Promotion> <RequiredXp>1000</RequiredXp> <Entity>units/{civ}_healer_a</Entity> </Promotion> <Heal> <Range>20</Range> <HP>5</HP> <Rate>2</Rate> <Recharge>5</Recharge> </Heal>
Range = The range the unit has to be within the target unit in order to heal HP = The amount of HP the healer does every time he heals someone Rate = The rate at which a healer heals the same unit Recharge = The delay between fully healing one unit and then starting to heal another
To explain rate and recharge, Mythos explained it this way: "Heal a dude at 5 hps every 2 seconds until he is healed. Then there is a delay or "recharge" time of 5 seconds before he can heal another unit."
comment:6 by , 12 years ago
Yep! What k776 posted above is a lot closer to how the XML entity templates currently work for other things. Each subsequent rank of Healer would have improved stats in the <Heal> element. Dividing up the rate, hp value, recharge time, range, etc. into their own tags allows us greater flexibility in tweaking the stats for each rank of healer. They also allow us to have technologies that affect each of these things, if we wanted to.
There will need to be support for the healer Actor to use a "heal" animation, something like this:
<animations> <animation file="biped/inf_staff_idle_a.dae" name="Idle" speed="200"/> <animation file="biped/inf_staff_walk_a.dae" name="Walk" speed="20"/> * <animation file="biped/heal_a.dae" name="Heal" speed="20"/> <animation file="infantry/general/death/inf_01.psa" name="Death" speed="400"/> </animations>
If the patch already includes the Actor enhancement, then please ignore. :)
Ideally the Healer, once a Healing action is initiated by the player, will continue to heal damaged units within vision range. He'll see the damaged unit, come to within <Range>, and begin to heal him. I'm thinking a sound effect should occur the moment the healer begins healing a new unit (something like Age of Kings' "healing chime" sound would work).
All right, those are my thoughts for now. :)
comment:7 by , 12 years ago
This is what the Heal xml element looks like (at the moment)
<Heal> <Range>20</Range> <HP>5</HP> <Rate>2000</Rate> <HealableClasses>Support Infantry Cavalry</HealableClasses> </Heal>
Rate is in miliseconds (though it could be changed to seconds). HealableClasses is to specify what is healable (the target must have one of these classes).
Recharge isn't implemented as I first need to make my mind up how to properly do this (suggestions welcome ;-) ).
Done:
- Healing allies is done (I haven't tested it thoroughly but it should work flawlessly).
- Fixed formations
- Added promote ability. This is only used to demonstrate the extensibility of the ability implementation and shouldn't get into the game.
- I added ranks to the healers (svn copy to all of those files to make it apply without problems)
- Modified some xml files to fix errors due to the rename of the *healer to *healer_b
- Gaining XP is based on healed HP * TargetXP/TargetHP
- Healers flee when they are attacked and if they don't have the StandGround stance enabled
Healers do not switch to another unit that needs to be healed if they finish their current healing activity. (Will probably do this next)
by , 12 years ago
Attachment: | #999_template_updates.patch added |
---|
Ranks for healers. They don't change any Heal options yet.
comment:8 by , 12 years ago
Idle healers now heal wounded units that enter their line line of sight.
There are some TODO comments in the code:
- promote ability: (see post above) (GuiInterface.js)
- Animation in HEALING in UnitAI.js is commented out as we don't have an animation yet
- Should the SetupRangeQuery and SetupHealRangeQuery functions be moved to a SetupRangeQuerys function? (UnitAI.js)
- UnitAI.prototype.CanHeal should we care about the <Healable> element in <Health> or should we introduce a <Unhealable> element that would fit better with the <HealableClasses>?
- CauseHeal in Heal.js needs a sound file.
- input.js: Currently we display the enabled heal cursor even on units that don't need to be healed. Is this worth changing?
- input.js: The heal (and the going to be removed promote) ability(s) are missing sounds.
- unit_commands.js: We should probably read tooltips from files or something.
Please review and test it.
by , 12 years ago
Attachment: | #999-2012-02-26.patch added |
---|
comment:9 by , 12 years ago
I have an idea how to implement <Recharge>. But before I start working on this I have some questions:
- Should the time only apply if a healer healed a unit that now has full health or if he healed a unit (doesn't matter if he healed it to max health)?
- Should it only apply if the healer is healing another unit. Imagine the following:
A healer is healing unit A. The user commands the healer to move somewhere, stops it (or the move is completed in less time than the recharge time) and tasks the healer to heal unit A again.
Do we want the user to wait or should we just use the prepare time (that should be less than recharge time)? (Using prepare time in this case would prevent punishing inexpirienced players or punishing mistakes).
comment:10 by , 12 years ago
Perhaps the Healer needs to recharge after every X HPs (~100) of healing.
comment:11 by , 12 years ago
After some discussion with Mythos we decided that <Recharge> isn't needed. If someone wants to implement this take a look at Timer.js GetTime()
and save it on leaving HEALING
and adjust the prepare time on enter
accordingly.
I removed the <Healable> element and added a <Unhealable> element to Healh.
Now to the Heal element:
<Heal> <Range>30</Range> <HP>5</HP> <Rate>2000</Rate> <UnhealableClasses></UnhealableClasses> <HealableClasses>Organic</HealableClasses> </Heal>
Range: The target must be within this distance from the healer to be healed.
HP: How many HP will be healed each healing turn.
Rate: The time between two healing turns in miliseconds.
UnhealableClasses: If the target has one of those classes it can't be healed by the healer. Can be empty (<UnhealableClasses/>)
HealableClasses: The target must have one of those classes to be healable. Can be Organic if we want all units (that are not mechanical or ships) to be healable.
All units that had <Healable>true</Healable> now have <Unhealable>false</Unhealable> and all that had false now have true.
Please test and review.
comment:12 by , 12 years ago
I've only taken a brief look so far. I will try and go over it more thoroughly soon.
One thing that I have seen so far is that you define PerformHeal() which doesn't do anything useful. I think it would be best to eliminate the function and just call CauseHeal() directly.
comment:13 by , 12 years ago
Good point. I will remove CauseHeal but I won't submit a new patch as I'd rather wait for a full review. Thanks for looking into it.
follow-up: 15 comment:14 by , 12 years ago
I have done a bit of testing with this patch.
The way that you have set up the line of sight range update in UnitAI means that it only tests units entering the healers range, so if you walk healers to a battle with some troops they just stand there initially without trying to heal anything.
Also healers should have their heal action bound to right click by default, so I don't have to select a button to target them.
I am also getting an error: ERROR: JavaScript error: simulation/helpers/Commands.js line 79 TypeError: cmpUnitAI.Heal is not a function ([object Object],0,[object Array])@simulation/helpers/Commands.js:79 ProcessCommand(1,[object Object])@simulation/helpers/Commands.js:78
This happens when I manually tell a healer to heal a unit.
follow-up: 16 comment:15 by , 12 years ago
Replying to quantumstate:
The way that you have set up the line of sight range update in UnitAI means that it only tests units entering the healers range, so if you walk healers to a battle with some troops they just stand there initially without trying to heal anything.
The healer should walk there and then go into idle state witch checks if there are any healable units in range. The losHealRange is just meant for the case that there are no healable units at all and a healable one enters the range.
I will look into the rest.
follow-up: 17 comment:16 by , 12 years ago
Replying to leper:
The healer should walk there and then go into idle state witch checks if there are any healable units in range. The losHealRange is just meant for the case that there are no healable units at all and a healable one enters the range.
I will look into the rest.
This is correct, but with my understanding of the code I think the following will happen:
The units with healer will walk to a location. They will stop, the healer goes into an idle state but all units have full health so the healer remains idle. Now the enemy starts hurting my units but my healers just sit their idle because the LosRangeUpdate function has not fired since my units started getting hurt.
by , 12 years ago
Attachment: | #999-2012-03-07.patch added |
---|
comment:17 by , 12 years ago
I couldn't reproduce the JS error you got, but I fixed a probably unrelated one.
Healers now have heal bound to right click.
Replying to quantumstate:
The units with healer will walk to a location. They will stop, the healer goes into an idle state but all units have full health so the healer remains idle. Now the enemy starts hurting my units but my healers just sit their idle because the LosRangeUpdate function has not fired since my units started getting hurt.
This is fixed using a rather ugly hack (see Philips comments in irc) but it shouldn't be so bad as it is limited to healers only (and only if they are idle).
comment:18 by , 12 years ago
Milestone: | Alpha 9 → Alpha 10 |
---|
comment:19 by , 12 years ago
I gone over the code in detail, in general it looks good, I have a few comments about things which I think could be improved.
As we discussed briefly on irc I think all the changes to unit_command.js should be scrapped. (There is a promote units cheat in the developer console (Alt-D) which is good for testing).
Healers should not walk to enemy units when in a group being given an attack command on an enemy unit. UnitAI is generally a mess with this kind of stuff though. One idea is to use a heal command on the enemy unit, then the healer would get to healing range and stop because it can't heal that unit. I would guess making this happen would be messy so this might warrant further discussion about how best to do things. The current behavior is annoying for gameplay though.
Heal.js 43: Rate is a non-optional attribute so a default value is not necessary
Health.js 139: This function normally returns an object so returning a boolean for the invalid case is unconventional. It would be better to return null or undefined (there doesn't seem to be a solid convention about which).
UnitAI.js 2010: I don't think the IsHealer() check is worth having. I don't think it will have any significant performance benefit (I tried measuring it and found it was faster without the if but that was within the inaccuracy of my measurement) and it might catch people out in the future if they try using GlobalHealthChanged for non healer units.
comment:20 by , 12 years ago
Removed the gui code (the action-heal-disabled cursor isn't used anymore as a result of this).
If healers get an attack order and can't attack the target they only move until they are within heal range. Changed the rest as recommended. Thanks again for the review.
follow-up: 23 comment:21 by , 12 years ago
Few comments from reviewing the patch:
Garrison.js
: Are we sure that "healing" is the same concept for garrisoning and healers? I've never thought of it that way, but maybe it's logical.Heal.js
: When I seeHP
I think of the unit's max hitpoints, would another name be more suitable, likeHealthIncrease
orHPIncreasePerHeal
? Garrison also has some confusing element names that we should clean up some day.Heal.GetUnhealableClasses
seems to be confusing the return types, sometimes it will return an array but if the classes are not defined it returns an empty string. I think you should return an empty array[]
instead (orundefined
but then you'd have to check for that everywhere).- What if we combined all the healability logic into
Health.IsUnhealable
:this.template.Unhealable == "true" || this.GetHitpoints() <= 0 || this.GetHitpoints() >= this.GetMaxHitpoints()
if I'm not mistaken they are only used all together, and it would let us change the criteria for unhealable units in one location instead of several scripts. We could even change it toIsHealable
and not have to invert the return value (mostly we seem interested if a unit is healable, not if its unhealable). - That would also seemingly mean there's no need for
UnitAI.TargetIsAtMaxHitpoints
. Heal.js
,UnitAI.js
: It improves legibility if you use spaces in the ternary operator (a ? b : c
) like we do for binary operators.Health.Increase
can return undefined now, so you should check for that inHeal.PerformHeal
before increasing XP.- We discussed finding healable units in range in IRC. I personally think RangeManager should be used in place of
OnGlobalHealthChanged
messages, either modified with Philip's suggestion or not. I would advise that we spend the time to find the best solution, or get as close as possible if it won't take a long time, rather than rushing through something that happens to work (especially with respect to UnitAI which is already messy and has many TODOs). The problem is once we have healing implemented there will be minimal incentive to go back and correct the TODOs. - The changes to
UnitAI.GetQueryRange
are not very clear,and it's not easily extended if we later want another distinction besides healer and attacker (or maybe healers that can attack) or additional stances. Could the common cmpRanged+range parts be moved into another function andGetQueryRange
accept an IID as a parameter? It would be called like this depending on which range you wanted:var range1 = this.GetQueryRange(IID_Attack); var range2 = this.GetQueryRange(IID_Heal);
- In
UnitAI.CanHeal
there's really no reason to mentioncmpIdentity.GetClassesList
as a worse alternative tocmpIdentity.HasClass
:) template_unit_support_healer.xml
: you might as well remove theAuras
element from the template to prevent confusion, we won't be using auras for healing anyway.Commands.js
: probably should reword the debug warning to something like:..."not owned by player "+player+" or their ally"
- In the actor XML, it looks like you should use two spaces instead of tabs for indenting.
Hmm I guess that turned into a bunch of comments, but most of them are brief considerations :)
comment:22 by , 12 years ago
Garrison.js: Are we sure that "healing" is the same concept for garrisoning and healers? I've never thought of it that way, but maybe it's logical.
I have no idea about which is most logical from a programming standpoint, but at least in gameplay terms it should be the same as far as I understand it (apart from it requiring a specific action in the healers case (although it can be automated) and happens as a side-effect of garrisoning). The only reason why it could be good not to make them the same is if we would want (or a modder would want) to add a tech that enables healing when garrisoned, but have healers always heal. But I guess that's possible in some other way?
comment:23 by , 12 years ago
I fixed most of your comments apart from the name change of HP (as I can't really say that <HPIncreasePerHeal> is better than <HP> that is a child of <Heal>) and the RangeManager one.
Replying to historic_bruno:
- We discussed finding healable units in range in IRC. I personally think RangeManager should be used in place of
OnGlobalHealthChanged
messages, either modified with Philip's suggestion or not. I would advise that we spend the time to find the best solution, or get as close as possible if it won't take a long time, rather than rushing through something that happens to work (especially with respect to UnitAI which is already messy and has many TODOs). The problem is once we have healing implemented there will be minimal incentive to go back and correct the TODOs.
I'm in favor of using RangeManager modified with Philips suggestion as maintaining an array in UnitAI isn't a really good solution. I don't know if I can redesign/extend RangeManager on my own so help would be appreciated.
I also changed the check in GarrisonHolder.js healable check to use the Health.js IsUnhealable() logic.
Thanks for the review.
by , 12 years ago
Attachment: | #999-2012-03-19.patch added |
---|
by , 12 years ago
Attachment: | #999-2012-03-23.patch added |
---|
comment:24 by , 12 years ago
Implements a way to get units within range to show up in a losRangeUpdate if they send a MT_ShowAsModified message.
Note the comments in Health.js. It may be best to use SetTimeout to call a function to set showAsModified to false so that the entity only shows up once, and not during every following update (as in each turn during MT_Update).
Another option would be to remove the parameter from MT_ShowAsModified and just loop over all entities at the end of ExecuteActiveQuerys(), but we could miss some messages if they occur between MT_Update that calls ExecuteActiveQuerys() and the end of that loop.
This patch also removes the use of IsUnit as that was removed in [11365].
comment:25 by , 12 years ago
UnitAI.js
:INDIVIDUAL.IDLE.LosRangeUpdate
now doesthis.FindNewHealTargets()
, but even if that returns true then it'll continue to runRespondToTargetedEntities
which might make it abandon its heal order and start attacking someone, if I follow it correctly. Is that what it should do, or should it return immediately if it finds a heal target?
- And/or: Could
LosRangeUpdate
be split into two different messages (call the new oneLosHealRangeUpdate
, and getUnitAI.prototype.OnRangeUpdate
to send the appropriate message depending onmsg.tag
).
- Calling
FindNewHealTargets
onLosRangeUpdate
is inefficient, since it callsResetActiveQuery
- the query will be evaluated twice (the first time triggersLosRangeUpdate
, the second is triggered by theResetActiveQuery
). Also,FindNewHealTargets
will be looping over every single entity that's in range, whenever a single unit enters/leaves range. The point of active queries is that you don't have to do that, you only have to loop over the few entities that entered range since the previous update. SoLosRangeUpdate
ought to do the same as it does for attack ranges (i.e. likethis.RespondToTargetedEntities(msg.data.added)
so it only looks at the entities added to the in-range set).
Health.js
probably shouldn't assumeamount != 0
. Either we should enforce that (make it report an error ifamount == 0
, and check all the current callers to make sure they avoid that case), or handle it correctly (it shouldn't trigger the no-longer-at-max-health behaviour when reducing by 0).
GetQueryRange
: The "var range = ...
" looks broken in theIID_Attack
case - it's just callingGetBestAttack
and notGetRange
.
CCmpRangeManager
: Don't likeshowAsModified
:-( . I think something like the following would be much cleaner (and more extensible):- Replace
EntityData
's "sort-of-bool showAsModified
" with "u8 flags
". - Replace
Query
's "bool showModified
" with "u8 flagsMask
". - Remove the special handling of
q.showModified
inExecuteActiveQueries
. - Extend
TestEntityQuery
so that it does "if (!(entity.flags & q.flagsMask)) return false;
". - Remove
MT_ShowAsModified
and setHealth.js
to callcmpRangeManager.SetEntityFlag(this.entity, "injured", true)
when health is reduced below max, and set flag tofalse
when increased back up to max. (Or you could equivalently replaceMT_ShowAsModified
with a similarMT_SetEntityFlag
, but I think a direct method call is simpler and cleaner than a message here, since nothing other thanRangeManager
is interested in the flag change.) - Set
UnitAI.js
to callrangeMan.CreateActiveQuery(..., flags)
whereflags
israngeMan.GetEntityFlagMask("injured")
for heal queries and-1
for attack queries. - Implement
CCmpRangeManager::SetEntityFlag
andGetEntityFlagMask
so that they map the string"injured"
onto1
. (This can be hardcoded for now. In the future we could easily add other flag bits - maybe e.g. a "hostile" flag on animals when they start attacking, so we can have defensive towers that ignore animals until they become hostile; or e.g. an "unstealthed" flag (defaulting true) which can be toggled by spies or cloaking-device-equipped mechs or whatever, so they can sneak up on other units without triggering their range queries. Perhaps those are a bit spurious but I think we can support many flags with very little more complexity than supporting one flag, so we might as well do so.)
- Replace
With that flag system, the desired behaviour of detecting already-in-range units as soon as they get injured should come naturally from the query's flag filtering, in the same way as filtering on owner.
(Am I missing any serious problems or drawbacks with this?)
Work in progress. Heal classes specified in template.