Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3606 closed defect (fixed)

[PATCH] Battlefield Medicine doesn't stop healing

Reported by: ffm Owned by: elexis
Priority: Should Have Milestone: Alpha 20
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

The technology Battlefield Medicine in the Temple heals not only when idle.

Attachments (6)

Health.js.patch (573 bytes ) - added by Matthew Guttag 8 years ago.
This patch changes the health regen to only work on idle units, I am unsure if this is correct or if the description of the technology to only work on idle units is incorrect.
IdleRegenRate.patch (5.9 KB ) - added by Matthew Guttag 8 years ago.
This patch adds the IdleRegenRate element to health and modifies Battlefield Medicine to use it
IdleRegenRate2.patch (6.2 KB ) - added by Matthew Guttag 8 years ago.
The logic for checking the idle state is now moved to UnitAI and will correctly handle turreted units. IdleRegenRate is now an optional element.
IdleRegenRate3.patch (6.6 KB ) - added by Matthew Guttag 8 years ago.
Updated version of the patch fixes comparisons on line 162, IsIdleOrIdlyGarrisoned renamed to is resting, IdleRegenRate is not optional.
IdleRegenRate4.patch (6.2 KB ) - added by Matthew Guttag 8 years ago.
Requested changes made
IdleRegenRate5.patch (5.7 KB ) - added by Matthew Guttag 8 years ago.
Removed changes from UnitAI, cleaned up the code.

Download all attachments as: .zip

Change History (17)

by Matthew Guttag, 8 years ago

Attachment: Health.js.patch added

This patch changes the health regen to only work on idle units, I am unsure if this is correct or if the description of the technology to only work on idle units is incorrect.

comment:1 by Matthew Guttag, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 20
Summary: Battlefield Medicine doesn't stop healing[PATCH] Battlefield Medicine doesn't stop healing

comment:2 by elexis, 8 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 17571:

Only regain health while idle (Battlefield Medicine tech). Fixes #3606, patch by Guuts.

comment:3 by elexis, 8 years ago

Keywords: review removed

Thanks for the patch! I only changed it to use an early return. Finally someone pointed at the right place in the code to fix this :)

comment:4 by mimo, 8 years ago

Resolution: fixed
Status: closedreopened

This patch has some flaws:

  • This regenRate is not only used by the BattleField medecine, and the other users do not necessarily require the unit to be idle. For example, the temple aura has no such requirement. We may still change this temple requirement (but that's a design choice), but what if for example some mod want to implement some negative regenRate ? there would be no reason to have it only when idle. I think the right solution would be to add a new element IdleRegenRate in the Health schema.
  • This battleField Medecine should also apply to garrisoned units, which iirc are not in IDLE state.
Last edited 8 years ago by mimo (previous) (diff)

in reply to:  4 comment:5 by bb, 8 years ago

Replying to mimo:

We may still change this temple requirement (but that's a design choice), but what if for example some mod want to implement some negative regenRate?

This last is even already there with the ibirian fire-ship: it has an negative regenrate (-2 if I am correct). Also its is used for my trampling patch.

by Matthew Guttag, 8 years ago

Attachment: IdleRegenRate.patch added

This patch adds the IdleRegenRate element to health and modifies Battlefield Medicine to use it

comment:6 by mimo, 8 years ago

Thanks for the new patch. There is nonetheless something which does not look nice with this approach (sorry, i didn't think about it yesterday when I proposed it). It is that the concept of idle unit is really a UnitAI concept, which does not mean anything for all structures and trees. So maybe it is better to make IdleRegenRate optional (and then you would have to test its existence each time you use it), or if anybody has a better idea ?

Edit: after discussion on irc with sanderd17, having it optional should be good enough

Otherwise, concerning the patch itself:

ExecuteRegeneration and ExecuteIdleRegeneration could be merged with let regen = this.GetRegenRate(); if (idle or garrisoned)

regen += this.GetIdleRegenRate();

Be careful with garrison: turrets (like units on walls) are garrisoned, but can be fighting. Turrets which do nothing are in the IDLE state, so I guess the test on IsIdle should be fine for them. So it must be enough to exclude turrets from the isGarrisoned test, and for that,you can require that it has no position. But it could be cleaner to add a UnitAI function which would test on the UnitAI state ("IDLE" for all idle ones (either normal or turret), and (iirc so to be checked) "GARRISON.GARRISONED" and "AUTOGARRISON" for garrison not turret) and call this function from ExecuteRegeneration.

new line 155: the two > should be >=

in the json file, Health/Idle should be Health/IdleRegenRate

Last edited 8 years ago by mimo (previous) (diff)

by Matthew Guttag, 8 years ago

Attachment: IdleRegenRate2.patch added

The logic for checking the idle state is now moved to UnitAI and will correctly handle turreted units. IdleRegenRate is now an optional element.

comment:7 by mimo, 8 years ago

Thanks for updating a new version of the patch,

but there is still the problem with the > of line 166 which should be >=

I still believe there is no reason to add the new function ExecuteIdleRegeneration, and that it should be merged with ExecuteRegeneration.

The name of the UnitAI function IsIdleOrIdlyGarrisoned is a bit strange, why not something simpler (for example IsResting), or even to remove it and do everything in the Health component as you did before. I adviced you to create it because i thought you would need to access the UnitAI State which should stay internal to UnitAI. But as you manage to use only already existing functions which encapsulate that, you can use them directly from Health.

Another advice which i gave you was maybe not quite right: going to an optional element. I mixed things and thought all gaia trees had a Health component, so that would have been worth, but in fact they don't. So going to optional is only to remove structures, and that is maybe not worth the complication.

by Matthew Guttag, 8 years ago

Attachment: IdleRegenRate3.patch added

Updated version of the patch fixes comparisons on line 162, IsIdleOrIdlyGarrisoned renamed to is resting, IdleRegenRate is not optional.

comment:8 by mimo, 8 years ago

The patch is nearly ok for me,

if you can show up on irc 0ad-dev, that's simpler for final details, and under which name do you want to be quoted in the commit message ? Otherwise here are my remaining comments:

as we don't have the optional anymore, lines 52-57 + 59 can be removed. same thing for 372, 373 and 379

regroup 369+370 and 377+378 if (this.regenRate != oldRegenRate OR this.idleRegenRate != oldIdleRegenRate)

this.CheckRegenTimer();

ExecuteRegeneration can still be shortened if you first compute the total regenRate let regen = RegenRate; if (IdleRegenRate != 0 && tests on cmpUnitAI are satisfied)

regen += IdleRegenRate;

and then call only once Increase or Reduce

Last edited 8 years ago by mimo (previous) (diff)

by Matthew Guttag, 8 years ago

Attachment: IdleRegenRate4.patch added

Requested changes made

comment:9 by mimo, 8 years ago

Some latest cleanup comments (see also http://trac.wildfiregames.com/wiki/Coding_Conventions if you've not yet seen it)

There is no need of braces for 1 line blocks (lines 132 and 134)

the UnitAI function could be simplified, with only a return statement

UnitAI.prototype.IsResting = function() 
{
      return this.IsIdle() || (this.IsGarrisoned() && !this.IsTurret());
}

and then, once it is inlined that way, i'm not sure this function is really needed as this line can be put directly in ExecuteRegeneration (see previous comment 7).

by Matthew Guttag, 8 years ago

Attachment: IdleRegenRate5.patch added

Removed changes from UnitAI, cleaned up the code.

comment:10 by mimo, 8 years ago

Resolution: fixed
Status: reopenedclosed

In 17590:

add specific regeneration when idle, fixes #3606, patch by Guuts

comment:11 by mimo, 8 years ago

Thanks for the patch

Note: See TracTickets for help on using tickets.