Make monster::heal actually return amount healed

[actual-nh]

Summary

Bugfixes "Make monster::heal actually return the amount healed"

Purpose of change

It is assumed in multiple places, and stated in monster.h, that the monster::heal function returns the amount healed. It doesn't, but it should.

Describe the solution

To get the return value, do hp - old_hp, not maxhp - old_hp.

Describe alternatives you've considered

This came about via #17452, which was to prevent heal from returning a negative number; I see no way that it now can, but just in case I have put in a std::max( 0, hp - old_hp ). It may be preferable to not do the max, or to emit an error if the heal amount somehow manages to be negative.

Testing

CI and by @eltank; thanks very much!

Additional context

See dev discord.

[eltank]

Please change monster::on_load() as well, otherwise it will become [more] broken by this change.

    if( healed < heal_amount && get_speed_base() < type->speed ) {
        int old_speed = get_speed_base();
        set_speed_base( std::min( get_speed_base() + heal_amount - healed, type->speed ) );
        healed_speed = get_speed_base() - old_speed;
    }

We need to give the monster its full speed if they did a full heal.
Also I think the partial speed heal should be proportional to hp healed as a fraction of total hp.

    if( get_speed_base() < type->speed ) {
        int old_speed = get_speed_base();
        if( hp >= type->hp ) {
            set_speed_base( type-> speed );
        } else {
            set_speed_base( std::min( old_speed + healed * type->speed / type->hp, type->speed ) );
        }
        healed_speed = get_speed_base() - old_speed;
    }

[actual-nh]

Please change monster::on_load() as well, otherwise it will be broken by this change.

    if( healed < heal_amount && get_speed_base() < type->speed ) {
        int old_speed = get_speed_base();
        set_speed_base( std::min( get_speed_base() + heal_amount - healed, type->speed ) );
        healed_speed = get_speed_base() - old_speed;
    }

I'm not sure how this change would break the above. Healed is the amount that was actually healed. If the potential heal amount is more than how much actually got healed, the "extra" goes to increasing speed... rather oddly, I agree.

We need to give the monster its full speed if they did a full heal.
Also I think the partial speed heal should be proportional to hp healed as a fraction of total hp.

I agree with both of the above; will do the below, thank you.

    if( get_speed_base() < type->speed ) {
        int old_speed = get_speed_base();
        if( hp >= type->hp ) {
            set_speed_base( type-> speed );
        } else {
            set_speed_base( std::min( old_speed + healed * type->speed / type->hp, type->speed ) );
        }
        healed_speed = get_speed_base() - old_speed;
    }

[eltank]

I'm not sure how this change would break the above. Healed is the amount that was actually healed. If the potential heal amount is more than how much actually got healed, the "extra" goes to increasing speed... rather oddly, I agree.

Currently that if condition evaluates to true (kind of by accident) if the monster was close to max hp. After your change (and without the additional modifications) the condition would evaluate to false when the monster is close to max and regen is capped. That would prevent it from healing speed. But the code was broken anyway when monster hp is low (the if condition evaluates to false). The suggested change should fix all that.

[actual-nh]

I'm not sure how this change would break the above. Healed is the amount that was actually healed. If the potential heal amount is more than how much actually got healed, the "extra" goes to increasing speed... rather oddly, I agree.

Currently that if condition evaluates to true (kind of by accident) if the monster was close to max hp. After your change (and without the additional modifications) the condition would evaluate to false when the monster is close to max and regen is capped. That would prevent it from healing speed. But the code was broken anyway when monster hp is low (the if condition evaluates to false). The suggested change should fix all that.

Actually, healed will be less than heal_amount if it's capped, meaning it will heal speed. But I agree that the code is confusing and doesn't make much sense, especially considering that there doesn't seem to be any other healing of speed in the reality bubble!

[actual-nh]

Should the proportional healing of speed be rounded up?

[eltank]

Should the proportional healing of speed be rounded up?

Probably doesn't matter since they'll likely do a full heal anyway, but you can change to lround or round up or even roll_remainder if you prefer. This is just the simplest option.

[actual-nh]

Should the proportional healing of speed be rounded up?

Probably doesn't matter since they'll likely do a full heal anyway, but you can change to lround or round up or even roll_remainder if you prefer. This is just the simplest option.

Just in case, making sure there's always a bit of healing of speed; also fixing a typo that I'm surprised astyle didn't object to - space between type-> and speed.

[eltank]

Probably out of scope for this PR, but regenerating monsters seem to only regenerate speed when they enter the reality bubble (on_load) and not while in the reality bubble (in which case they only regen hp).

Not sure what the implications are. AFAIK the only way monsters can lose base speed is during resurrection, which I think can only happen in the bubble anyway. So when would speed regen on load even trigger?

The main implication of that is: I don't know how to test that the speed regen change works.
Hmm, maybe wait for a dissolute to rez, go away then come back later.

[actual-nh]

Probably out of scope for this PR, but regenerating monsters seem to only regenerate speed when they enter the reality bubble (on_load) and not while in the reality bubble (in which case they only regen hp).

Not sure what the implications are. AFAIK the only way monsters can lose base speed is during resurrection, which I think can only happen in the bubble anyway. So when would speed regen on load even trigger?

Scenario: Something manages to revive, but you move and it's no longer in the reality bubble. You come back, it should have regenerated speed as well as hp in the meantime. (Of course, it should also be doing that while in the reality bubble... hmm.)

The main implication of that is: I don't know how to test that the speed regen change works.
Hmm, maybe wait for a dissolute to rez, go away then come back later.

Sounds good to me!

Oh. I think I just realized why it's only at times (resurrection or loading) that the monster isn't interacting that the speed is changing. If a monster regenerated sufficient speed to do another action by the end of a turn, but didn't have that speed at the beginning of the turn, where does it fall in the ordering of actions? Admittedly, the same is true of a PC donning a ring of speed/haste/whatever...

[eltank]

If a monster regenerated sufficient speed to do another action by the end of a turn, but didn't have that speed at the beginning of the turn, where does it fall in the ordering of actions?

Effect processing (including regen) happens before it gets its moves (at the beginning of the turn) -- Creature::process_turn()

[eltank]

Suggested change

	set_speed_base( std::min( old_speed + speed_delta, type->speed ) );
	type->speed ) );
	set_speed_base( std::min( old_speed + speed_delta, type->speed ) );

Something got messed up here. Let's remove the junk.

[actual-nh]

Github is insisting that there's no change in your suggestion - doing a pull to my local machine to check.

[eltank]

First round of testing. You can see that before this change, dissolute regen was reported as "visibly regenerating", which should be displayed when healing more than 50 points. But dissolutes only have 1 point of regen.

After the change, the report is "healing slowly", which is what is expected for this monster:

Note that even after regaining all hp, they can barely move after rez if you keep then in the bubble while they heal up (unchanged by this PR)

[eltank]

Test 2. Kill a dissolute, wait for it to rez, teleport away, wait 1s and teleport back. With and without this change the outcome is the same. They somehow get a full heal (which seems broken) and recover their speed. So none of that partial heal code is kicking in...

The problem seems to be that the number of turns since last update is incorrect:

Expected 1 or 2, but it was 5 million!

[actual-nh]

Test 2. Kill a dissolute, wait for it to rez, teleport away, wait 1s and teleport back. With and without this change the outcome is the same. They somehow get a full heal (which seems broken) and recover their speed. So none of that partial heal code is kicking in...

Odd. Did you catch the debug message at the end of on_load()?

[actual-nh]

The problem seems to be that the number of turns since last update is incorrect:

Expected 1 or 2, but it was 5 million!

I was wondering if it might be something with that. to_turns<int>( dt ) (or just maybe const time_duration dt = calendar::turn - last_updated) would appear not to be behaving correctly.

[eltank]

I was wondering if it might be something with that. to_turns<int>( dt ) (or just maybe const time_duration dt = calendar::turn - last_updated) would appear not to be behaving correctly.

No, that part is working fine.
The last_updated value is set correctly in on_unload but then it gets lost. When on_load executes it is 0.

[actual-nh]

The last_updated value is set correctly in on_unload but then it gets lost. When on_load executes it is 0.

Oh, interesting. (I note that npc::on_unload() doesn't even try to set last_updated? Ah. Probably because Character::update_body does it.)

[eltank]

It looks like that last_update bug is going to take a while to debug.
In the mean time, I verified that this PR does fix one bug, didn't notice any anomalies while fighting dissolutes and "regenerating zombie"s and the code looks good to me so this should be merge-able.

[eltank]

I fixed that other bug, so I repeated test 2. I teleported out after the dissolute got up, waited 15 turns, teleported back, and saw:

Partial hp restored and partial speed restored as expected.

[actual-nh]

I fixed that other bug, so I repeated test 2. I teleported out after the dissolute got up, waited 15 turns, teleported back, and saw:

Partial hp restored and partial speed restored as expected.

Excellent! (What was the bug, BTW? I look forward to a PR to fix it; that will also be very helpful for having morale and anger properly affected by time out of the reality bubble.)

[eltank]

Excellent! (What was the bug, BTW? I look forward to a PR to fix it; that will also be very helpful for having morale and anger properly affected by time out of the reality bubble.)

#51194

But... now they heal extremely slowly after reviving so I want to get feedback on whether we need to buff that.