-
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
Further damage model improvements and extras #229
Conversation
345ccfe
to
9339e1e
Compare
9339e1e
to
fabbaa8
Compare
Normalizes line endings due to git attribute change
- Allow extra turns
…for mutant with akromorphosis automation
Don't panic! The culprit behind the big line count change came from the formatter redoing the line endings on |
if (actor.system instanceof CharacterDataModel) { | ||
const characterData = actor.system; | ||
const damageType = characterData.overrides.damageType; | ||
if (damageType && damageType !== 'untyped') { |
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.
Why the special case for untyped
?
It's a standard damage type, just like all the others.
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.
Ah, I wasn't sure whether someone would want to override to untyped
.
const { StringField, NumberField } = foundry.data.fields; | ||
return { | ||
turns: new NumberField({ initial: 1, min: 1, integer: true, nullable: false }), | ||
damageType: new StringField({ initial: '', choices: Object.keys(FU.damageTypes), blank: true, nullable: false }), |
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 don't blame you for not following the discussion, but we figured out that we'll need at least the following differentiated override possibilities:
- attacks (accuracy check)
- spells (magic check)
- skills (flat damage, not damage by free attacks or spells)
Additionally, we will need fields for both "normal" and "priority" changes.
The rationale is as follows:
An accessory may change the damage type of all attacks, spells and skills to a specific type (CRB p284).
A dance, like "Angel Dance", changes the damage type of attacks and spells, but not skills (HF p144).
The "Arcanum of the Sword" (CRB p181) changes the damage type of attacks and only attacks to "untyped".
It is reasonable for a character to have an accessory equipped that changes their damage to "earth", while dancing the "Angel Dance" and being merged with the "Arcanum of the Sword".
In this constellation their attacks should do "untyped" damage, their spells "light" damage and their skills "earth" damage.
Skills that change the damage type for a single attack or spell occupy something of a middle ground between "normal" and "priority" changes.
They take precedence over "normal", but yield to "priority".
They also come in "normal" and "priority" variations:
A tinkerers "Pyro Infusion" will add 5 damage to a "Shadow Strike", but won't change the damage type from "dark" to "fire".
I'd say implementing damage type changes for flat damage is outside the scope of this PR, but both attacks and spells should be supported independently.
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.
Very well. I think we could make sure of a nice table on a feature request that lists the use cases. I will sub-divide the damage override data structure.
@@ -14,7 +15,8 @@ export class DamageDataModelV2 extends foundry.abstract.DataModel { | |||
hasDamage: new BooleanField(), | |||
hrZero: new BooleanField(), | |||
value: new NumberField({ initial: 0, integer: true, nullable: false }), | |||
extra: new StringField({ nullable: true }), | |||
onRoll: new StringField({ nullable: true }), |
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'd prefer blank
to nullable
, since exposing these fields on the sheets will basically guarantee that they are filled with empty strings.
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.
Ah I wasn't aware of this option. Will do.
static defineSchema() { | ||
const { StringField, NumberField } = foundry.data.fields; | ||
return { | ||
turns: new NumberField({ initial: 1, min: 1, integer: true, nullable: false }), |
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.
Not a fan of having this under overrides
.
Personally, I feel like having this under bonuses
and starting with 0
is more intuitive.
This would make the usual usecases of "lose a turn"/"gain an additional turn" very straightforward and applicable to both PCs and NPCs: ["system.bonuses.turns", "add", "1"].
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.
Oh that's a great idea.
module/helpers/inline-affinity.mjs
Outdated
const localizedType = game.i18n.localize(FU.damageTypes[type]); | ||
const affValue = FU.affValue[value]; | ||
const localizedValue = game.i18n.localize(FU.affType[affValue]); | ||
if (value === '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 feel like this is conflating different usecases by lumping together changes to affinities and changes to outgoing damage.
It's not a 100% fit, but the inline weapon enchantments feel like a more sensible place for this.
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 wanted to avoid having to define a new inline command, but I may as well at this point.
@@ -48,7 +48,8 @@ | |||
"hasDamage": true, | |||
"value": 0, | |||
"type": "", | |||
"extra": "$sl+$tsc" | |||
"onRoll": "", | |||
"onApply": "$sl+$tsc" |
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.
This formula is actually not correct.
"Cheap Shot" only deals its bonus damage if the target is suffering from at least 1 status effect.
Untested, but (min($tsc, 1) * $sl) + $tsc
might work.
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.
Ah. Will try it.
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.
- Add new inline, @type that now supports damage - Redeisgn the damage type override fields - Cry a little
overrides
data model to characters; AddLone Wolf
effect to grant extra turns for PCs@AFFINITY[dark damage]