Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

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

XML.Entity.Actions.Heal

Attachments (11)

#999-2012-02-23.patch (21.5 KB ) - added by leper 12 years ago.
Work in progress. Heal classes specified in template.
heal.png (2.9 KB ) - added by leper 12 years ago.
#999-2012-02-24.patch (53.6 KB ) - added by leper 12 years ago.
Work in progress.
#999_template_updates.patch (11.3 KB ) - added by leper 12 years ago.
Ranks for healers. They don't change any Heal options yet.
#999-2012-02-26.patch (37.9 KB ) - added by leper 12 years ago.
#999-2012-02-29.patch (45.2 KB ) - added by leper 12 years ago.
Ready for review
#999-2012-03-07.patch (46.4 KB ) - added by leper 12 years ago.
#999-2012-03-08.patch (46.9 KB ) - added by leper 12 years ago.
Fixes conflicts caused by 11279 (#30).
#999-2012-03-16.patch (45.8 KB ) - added by leper 12 years ago.
Added placeholder animation
#999-2012-03-19.patch (45.2 KB ) - added by leper 12 years ago.
#999-2012-03-23.patch (54.4 KB ) - added by leper 12 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 by leper, 12 years ago

Owner: set to leper
Status: newassigned

by leper, 12 years ago

Attachment: #999-2012-02-23.patch added

Work in progress. Heal classes specified in template.

comment:2 by leper, 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.

Last edited 12 years ago by leper (previous) (diff)

by leper, 12 years ago

Attachment: heal.png added

comment:3 by Kieran P, 12 years ago

Milestone: BacklogAlpha 9

comment:4 by Kieran P, 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
Last edited 12 years ago by Kieran P (previous) (diff)

comment:5 by Kieran P, 12 years ago

Priority: Should HaveMust 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 michael, 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. :)

Last edited 12 years ago by michael (previous) (diff)

by leper, 12 years ago

Attachment: #999-2012-02-24.patch added

Work in progress.

comment:7 by leper, 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 leper, 12 years ago

Attachment: #999_template_updates.patch added

Ranks for healers. They don't change any Heal options yet.

comment:8 by leper, 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 leper, 12 years ago

Attachment: #999-2012-02-26.patch added

comment:9 by leper, 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 michael, 12 years ago

Perhaps the Healer needs to recharge after every X HPs (~100) of healing.

by leper, 12 years ago

Attachment: #999-2012-02-29.patch added

Ready for review

comment:11 by leper, 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.

Last edited 12 years ago by leper (previous) (diff)

comment:12 by Jonathan Waller, 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 leper, 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.

comment:14 by Jonathan Waller, 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.

in reply to:  14 ; comment:15 by leper, 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.

in reply to:  15 ; comment:16 by Jonathan Waller, 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 leper, 12 years ago

Attachment: #999-2012-03-07.patch added

in reply to:  16 comment:17 by leper, 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).

by leper, 12 years ago

Attachment: #999-2012-03-08.patch added

Fixes conflicts caused by 11279 (#30).

comment:18 by Kieran P, 12 years ago

Milestone: Alpha 9Alpha 10

comment:19 by Jonathan Waller, 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 leper, 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.

by leper, 12 years ago

Attachment: #999-2012-03-16.patch added

Added placeholder animation

comment:21 by historic_bruno, 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 see HP I think of the unit's max hitpoints, would another name be more suitable, like HealthIncrease or HPIncreasePerHeal? 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 (or undefined 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 to IsHealable 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 in Heal.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 and GetQueryRange 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 mention cmpIdentity.GetClassesList as a worse alternative to cmpIdentity.HasClass :)
  • template_unit_support_healer.xml: you might as well remove the Auras 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 :)

Last edited 12 years ago by historic_bruno (previous) (diff)

comment:22 by Erik Johansson, 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?

in reply to:  21 comment:23 by leper, 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 leper, 12 years ago

Attachment: #999-2012-03-19.patch added

by leper, 12 years ago

Attachment: #999-2012-03-23.patch added

comment:24 by leper, 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].

Last edited 12 years ago by leper (previous) (diff)

comment:25 by Philip Taylor, 12 years ago

  • UnitAI.js: INDIVIDUAL.IDLE.LosRangeUpdate now does this.FindNewHealTargets(), but even if that returns true then it'll continue to run RespondToTargetedEntities 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 one LosHealRangeUpdate, and get UnitAI.prototype.OnRangeUpdate to send the appropriate message depending on msg.tag).
  • Calling FindNewHealTargets on LosRangeUpdate is inefficient, since it calls ResetActiveQuery - the query will be evaluated twice (the first time triggers LosRangeUpdate, the second is triggered by the ResetActiveQuery). 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. So LosRangeUpdate ought to do the same as it does for attack ranges (i.e. like this.RespondToTargetedEntities(msg.data.added) so it only looks at the entities added to the in-range set).
  • Health.js probably shouldn't assume amount != 0. Either we should enforce that (make it report an error if amount == 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 the IID_Attack case - it's just calling GetBestAttack and not GetRange.
  • CCmpRangeManager: Don't like showAsModified :-( . 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 in ExecuteActiveQueries.
    • Extend TestEntityQuery so that it does "if (!(entity.flags & q.flagsMask)) return false;".
    • Remove MT_ShowAsModified and set Health.js to call cmpRangeManager.SetEntityFlag(this.entity, "injured", true) when health is reduced below max, and set flag to false when increased back up to max. (Or you could equivalently replace MT_ShowAsModified with a similar MT_SetEntityFlag, but I think a direct method call is simpler and cleaner than a message here, since nothing other than RangeManager is interested in the flag change.)
    • Set UnitAI.js to call rangeMan.CreateActiveQuery(..., flags) where flags is rangeMan.GetEntityFlagMask("injured") for heal queries and -1 for attack queries.
    • Implement CCmpRangeManager::SetEntityFlag and GetEntityFlagMask so that they map the string "injured" onto 1. (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.)

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

comment:26 by leper, 12 years ago

Resolution: fixed
Status: assignedclosed

In 11536:

Implement healing. Fixes #999.

comment:27 by leper, 12 years ago

Keywords: review removed

Thanks for the reviews.

Note: See TracTickets for help on using tickets.