#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)
Change History (17)
by , 8 years ago
Attachment: | Health.js.patch added |
---|
comment:1 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 20 |
Summary: | Battlefield Medicine doesn't stop healing → [PATCH] Battlefield Medicine doesn't stop healing |
comment:3 by , 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 :)
follow-up: 5 comment:4 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
comment:5 by , 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 , 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 , 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
by , 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 , 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 , 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 , 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
comment:9 by , 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 , 8 years ago
Attachment: | IdleRegenRate5.patch added |
---|
Removed changes from UnitAI, cleaned up the code.
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.