Traing bravery

[Meridian]

By the way, while we're at it, I'd vote for including Karadoc's fix as well.

Yes, I will.

you'll get a lot by using any outfit with a large combat stress increase (like Rags).

Btw. I never really understood just from pedia entries, which outfits increase combat stress and which decrease it.

For me "Combat stress: +4" and "Combat stress: -4" can both be interpreted both ways.

How about changing to "Combat stress increase: 4" and "Combat stress decrease: 4" instead? Or vice versa, whichever interpretation is actually correct. Or even completely without numbers, just say which effect it has.

[Dioxine]

Your proposition is sound, but it's 8 letters longer, and that's an insane waste of space for the tiny amount of text one can use in an Armor entry

[Meridian]

"More stress" and "Less stress" then?

[Dioxine]

Less Stress: +4 or Less Stress: -4? That's confusing as hell
Stress +4 or Stress -4 are IMO much more precise, since '-' means 'less', doesn't it?

[Meridian]

Less Stress: +4 or Less Stress: -4? That's confusing as hell
Stress +4 or Stress -4 are IMO much more precise, since '-' means 'less', doesn't it?

No no, just "Less stress" and "More stress", without any numbers. Or something along those lines, in non-broken english

[Dioxine]

But Sire, numbers are very important! Especially since some armors yield negligible differences, while other major ones (like +8, which is equal to -40 Bravery).
I think I can do it like Higher Stress (x) and Lower Stress (x), w/o any +/- if they're ambigous for some reason; but what is less ambiguous and more internationally comprehensible than mathematical symbols?

[Meridian]

Do as you wish basically. It is correct, it's just confusing (for me).

It's the double negative that is the confusing part.

"Combat stress: -8" means +40 bravery, or something like that... one negative is the word "stress", second negative is the number "-8"... giving you a positive result.

That's what I am having problems with every time I look at that description. The minus sign and the lack of minus sign are not strong enough to convey the message immediately and without thinking. Words less and more are strong enough (for me).

[Eddie]

How about "Morale bonus" and "Morale penalty"?

[Dioxine]

It doesn't penalize Morale though. You always have 100 Morale pool.

[yrizoud]

I think "Stress tolerance" fits the bill. It's a quality (+x = good, -x=bad), you have the word "stress" in common with the "Combat stress" Piratez topic, and it's an actual expression in the English language.

A more gamey name would be "morale regen"

[Meridian]

I have a suggestion: only award the experience if the most recent attack was from an enemy.

I have made 2 changes to your code:
- also if the attack on player comes from a neutral (not only from enemy), it will work... this is not possible yet, but will be once we merge changes from vanilla
- healing enemies should work as it worked before... I think your change allowed only healing enemies hurt by enemies, not by player

Please review, I'm quite confused from all the ands, ors and nots right now:
https://github.com/MeridianOXC/OpenXcom/commit/b2713dc6c5b789ae2dae0b99461863a6a0a4c3a2

[karadoc]

I have made 2 changes to your code:
- also if the attack on player comes from a neutral (not only from enemy), it will work... this is not possible yet, but will be once we merge changes from vanilla
- healing enemies should work as it worked before... I think your change allowed only healing enemies hurt by enemies, not by player

Please review, I'm quite confused from all the ands, ors and nots right now:
https://github.com/MeridianOXC/OpenXcom/commit/b2713dc6c5b789ae2dae0b99461863a6a0a4c3a2

The diff you posted looks totally wrong... but it isn't. It's just that you've put the changes in two separate commits, and so it all looks a bit different.
I'm pretty sure my code still worked fine for healing enemies. The conditions we are adding are to check whether or not we should abort the function; not continue to add experience. So the added conditions wouldn't have changed the fact that healing enemies worked in the first place.

I don't think we need to check the original faction of the target; because the wound still needs to be caused by our enemy. I kind of like the idea of gaining bravery for healing neutrals who were wounded by the enemy. So I'd probably skip the original faction check for that reason, and for simplicity.

I can't see any problems with your code. And I commend you moving the weapon condition to the top. Good call on that.

But since you've moved things around a bit, I'll suggest a few more minor tweaks changes for readability. The main thing is that the 'else' block after checking that there's a target doesn't need to be a block at all. Because if there's no target, we abort immediately.

Here's what I suggest for now:

Code: [Select]

if (!weapon) return false;

if (!target) return false;

https:// only give experience for actions performed on enemies...
https:// ... unless the action is to heal a fatal wound which was inflicted by an enemy (or neutral)
if (weapon->getRules()->getBattleType() == BT_MEDIKIT && target->killedBy() != FACTION_PLAYER)
{
https:// continue as normal
}
else
{
https:// only enemies count, not friends or neutrals
if (target->getOriginalFaction() != FACTION_HOSTILE) return false;

https:// mind-controlled enemies don't count though!
if (target->getFaction() != FACTION_HOSTILE) return false;
}
I usually wouldn't use an empty if block like that; but I've left if that way because it does kind of make the conditions easier to follow, somehow.

It would probably be nice to have some consistency in how the `if() return` things are done. In the current version of the code, there are three versions...

Code: [Select]

if (condition) return false;

if (condition)
{
return false;
}

if (condition)
return false;
If it were up to me, I'd use the final one all the time. But I'm pretty sure I've read that the style guidelines for this project say not to use that one ever! (All `if`s are meant to have braces.)
So I've changed it to use the shortest one, just to keep it... short. Its probably good to be able to see all the ways the code can return early without having to scroll.


In any case, your current code looks fine to me. I'm just tinkering at the edges now.

[karadoc]

While we're talking about healing stuff, I'd like to mention another minor bug, and fix it.

The bug is that the player cannot heal enemy units while they are mind-controlled. (In fact, by the look of it, enemy units also cannot be wounded while they are mind-controlled.)

Here's the relevant code:

Code: [Select]

bool BattleUnit::isWoundable() const
{
return !_armor->getBleedImmune(!(_type=="SOLDIER" || (Options::alienBleeding && _faction != FACTION_PLAYER)));
}

And my suggested fixed version... which I haven't tested yet:

Code: [Select]

bool BattleUnit::isWoundable() const
{
return !_armor->getBleedImmune(!(_type=="SOLDIER" || (Options::alienBleeding && __originalFaction != FACTION_PLAYER)));
}
I don't think the woundability of a unit should depend on whether it is mind controlled; so it makes sense to check their original faction rather than their current faction.

--

By the way, one minor problem with our bravery changes is that melee attack of the super-medikit will probably give xp when used on a friendly unit. The problem is that we're just checking if the item is a medikit; whereas what we really want to know is if we're trying to heal something...   I don't know an easy fix for that off the top of my head.

[karadoc]

I have bad news. It still doesn't give experience for healing wounds on friendly soldiers.

I was mistaken about `killedBy()` being a reliable way to tell who caused the wounds.

Apparently killedBy doesn't get set when a player character is wounded. Here is the relevant code.

Code: [Select]

if (unit && target->getFaction() != FACTION_PLAYER)
{
https:// if it's going to bleed to death and it's not a player, give credit for the kill.
if (wounds < target->getFatalWounds())
{
target->killedBy(unit->getFaction());
}
}

The obvious solution would be to just remove that condition, so that killedBy does get set. That's probably what I'd do if it was my game. But I don't know what the justification is for how it currently works, and changing it would probably be yet another divergence from the core game. Maybe there are a good reason for leaving it as is. Maybe it would be better to just let medikits give experience for all wounds, caused by the enemy or otherwise. That's how it worked previously anyway.

Sorry for the mistake about killedBy.