-
Notifications
You must be signed in to change notification settings - Fork 22
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
Buff-Effect to deal damage #15
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…om/RKO/battleSim into feature/buff_effect_to_deal_damage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments about changes this would probably need, but this is a cool feature. I dread having to go through the entire bestiary to find every single poison effect but oh well .-.
</> | ||
) : (modifier === 'halfOnSave') ? ( | ||
<> | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The container wasn't meant to contain more than one child element until now
So, to make this look better, add this after line 35 of actionForm.module.scss
:
gap: 4px;
align-items: center;
if (buffClone.halfOnSave) | ||
appliedDamage = appliedDamage / 2 | ||
|
||
actualDamage += appliedDamage * buffMagnitude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formula is wrong here: if a target takes half damage on save, then you split it into two scenarios:
- The target saves, they take half the average damage
- The target fails, they take the average damage
On average, this should deal more damage than an effect which deals 0 damage if the target saves.
const chanceToFail = calculateChanceToFail(attacker, target, action.riderEffect.dc)
const baseDamage = Math.max(evaluateDiceFormula(buffClone.applyDamage), 0)
let avgDamage = baseDamage * chanceToFail
if (buffClone.halfOnSave) {
avgDamage += (1 - chanceToFail) * appliedDamage / 2
}
actualDamage += hitChance * avgDamage
getStats(stats, target).damageTaken += totalDamageApplied //TODO Apply damage dealt number to owner of buff? | ||
} ); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formula is wrong here - if the target takes half damage on a successful save, it should take more damage on average (see the comment below, on useAtkAction
for more detail)
I think this probably needs to use the buff.magnitude field as well - this field represents the odds of the buff being present (for a rider effect, it's the hit chance of the attack, times the chance for the target to fail the save against the debuff). So if the damage debuff deals 40 damage per round, but only has 50% chance to be present, then it should only deal 20 damage, not 40.
There's probably a way to extract the damage logic from useAtkAction into a separate function, so you can re-use it here. That way we won't have to worry about temp hp, or tracking the attacker's damage dealt stat - that's already handled by the bit of code in the useAtkAction function.
This will still probably require adding a "buff.bufferId" field, so we can find the original attacker
@@ -36,6 +36,8 @@ const BuffSchema = z.object({ | |||
dc: DiceFormulaSchema.optional(), | |||
save: DiceFormulaSchema.optional(), | |||
condition: CreatureConditionSchema.optional(), | |||
applyDamage: DiceFormulaSchema.optional(), | |||
halfOnSave: z.boolean().optional(), //TODO I need to combine the applyDamage and halfOnSave to a single... object? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big preferences here, but one of the requirements I have for this project is that updates must be backwards compatible with whatever the users have in their localStorage. So if you're going to wrap these two properties in an object later, might as well do it before this PR gets merged 👍
No description provided.