Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes bleed being applied when damage has been mitigated. Had been using... #6460

Merged
merged 1 commit into from Mar 7, 2014

Conversation

Projects
None yet
3 participants
@frycarson
Copy link
Contributor

commented Mar 5, 2014

... damage before armor or block had been taken into account. Issue #6432

Fixes bleed being applied when damage has been mitigated. Had been us…
…ing damage before armor or block had been taken into account. Issue #6432
if (hp_cur[hp_head] < 0) {
lifetime_stats()->damage_taken+=hp_cur[hp_head];
hp_cur[hp_head] = 0;
}
*/

This comment has been minimized.

Copy link
@KA101

KA101 Mar 6, 2014

Contributor

Two issues here.

  1. It's possible, though not certain, that the head's supposed to take double damage. That's not unknown in location-based HP. Worth discussing that, IMO.

  2. the if-statement there is so you don't take overkill damage on a fatal hit. hp_head <= 0 => Game Over, press spacebar. The game looks out for that enough to make me think it's worth keeping around.

This comment has been minimized.

Copy link
@frycarson

frycarson Mar 6, 2014

Author Contributor

I'll revert that bit of the change for now then until we determine whether double damage is intended or not. The if statement is in the other spot in the code where the damage is initially applied, and if double damage is the intent I think that should be moved to where the rest of the damage is applied to make the intent clear (player::apply_damage() as opposed to player::deal_damage()). I could probably also eliminate many of the other comments.

@KA101 KA101 assigned KA101 and unassigned KA101 Mar 6, 2014

@KA101

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2014

OK, pending a resolution to the head-damage question, then.

@Waladil

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2014

I'd really like this to get put into master -- I'd say leave the 2x head damage thing and maybe open a different PR to either remove it or change how that works so it (among other things) shows correct damage in the game log.

@KA101

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2014

Concur.

@KA101 KA101 self-assigned this Mar 7, 2014

@KA101 KA101 merged commit 3599a57 into CleverRaven:master Mar 7, 2014

1 check passed

default
Details
@frycarson

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2014

Did you strip the commenting to readd the 2x head damage, or do I need
to do that?

On 3/6/14, KA101 notifications@github.com wrote:

Merged #6460.


Reply to this email directly or view it on GitHub:
#6460

@KA101

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2014

I've had to clean up enough PRs recently that right now I don't really remember. Go ahead and PR it just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.