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.

Sign up for GitHub

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

Track advantage modes #4838

Merged
merged 1 commit into from
Dec 11, 2024
+64 −2
Merged

Track advantage modes #4838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion module/data/fields/_module.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export * from "./activities-field.mjs";
export {default as AdvancementField} from "./advancement-field.mjs";
export {default as AdvancementDataField} from "./advancement-data-field.mjs";
export {default as AdvancementField} from "./advancement-field.mjs";
export {default as AdvantageModeField} from "./advantage-mode-field.mjs";
export {default as FormulaField} from "./formula-field.mjs";
export {default as IdentifierField} from "./identifier-field.mjs";
export {default as LocalDocumentField} from "./local-document-field.mjs";
Expand Down
59 changes: 59 additions & 0 deletions module/data/fields/advantage-mode-field.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* Subclass of NumberField that tracks the number of changes made to a roll mode.
*/
export default class AdvantageModeField extends foundry.data.fields.NumberField {
/** @inheritDoc */
static get _defaults() {
return foundry.utils.mergeObject(super._defaults, {
choices: [-1, 0, 1],
initial: 0,
label: "DND5E.AdvantageMode"
});
}

/* -------------------------------------------- */

/**
* Number of advantage modifications.
* @type {number}
*/
#advantage;

/* -------------------------------------------- */

/**
* Number of disadvantage modifications.
* @type {number}
*/
#disadvantage;

/* -------------------------------------------- */

/** @inheritDoc */
initialize(value, model, options={}) {
this.#advantage = Number(value === 1);
this.#disadvantage = Number(value === -1);
return value;
}

/* -------------------------------------------- */
/* Active Effect Integration */
/* -------------------------------------------- */

/** @override */
applyChange(value, model, change) {
Copy link
Contributor

@Fyorl Fyorl Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we essentially ignore the mode (if it's not CUSTOM) for these fields? I guess it's not really obvious what the mode should be, but I think it might be useful to reserve some modes in case we want to implement some special semantics here later. If people are creating AEs with any mode and having it work, then if we try to change the semantics of a mode later, it will break their AEs and we are on the hook for a migration.

I think OVERRIDE ADD (edited: see below for rationale here) is probably the one we should support, and the rest should explicitly do nothing (i.e. not make a change), so that it's clear that they do not work, and people don't start creating AEs with the other modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is numeric values, we have so far been using UPGRADE and DOWNGRADE, so I think those two should be supported. I think ADD makes sense as well (e.g. you add a new instance of advantage or disadvantage), and OVERRIDE is arguably similar.

CUSTOM is of course only for modules. I don't disagree that MULTIPLY should perhaps be ignored, but I did not consider it at the time.

Anyway that was my reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that, at the moment, ADD, OVERRIDE, UPGRADE, and DOWNGRADE all the do the same thing, and I wondered if maybe it would be useful to reserve some of these modes for special behaviour around advantage and disadvantage that we might want to implement later. The idea was to pre-empt people creating AEs with these modes and having their behaviour change out from underneath them later, but it sounds like people are already creating AEs despite these modes not actually being supported yet?

Anyway, UPGRADE and DOWNGRADE happen to work because it's numerical data, but advantage and disadvantage aren't numerical concepts. You can't say "roll this with advantage if not already rolling 'higher' than advantage", like you would with the Gauntlets of Ogre Power, for example, which say to 'set your Strength score to 19 if not already higher'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it sounds like people are already creating AEs despite these modes not actually being supported yet?

Not much, I hope, though yes they (we) have. It is worth pointing out though that advantage mode is supported for initiative and concentration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been looking over some features that involve modifications to advantage and disadvantage and have a couple of examples here.

  • The Clockwork Soul Sorcerer's Restore Balance forces a roll to have neither advantage, nor disadvantage. I think the most intuitive way to represent this is with an OVERRIDE 0.
  • The Barbarian's Brutal Strike forces a roll to lose advantage, but does not preclude the roll from having disadvantage. I think the most intuitive way to represent this is with a DOWNGRADE 0.

To be clear: These features we would not model with ActiveEffects in the first place, but they are examples of effects that exist within the game system. It's not a stretch to imagine a spell that does something similar to Restore Balance, but that lasts for a minute, for example.

So, with that in mind, I think it's important to reserve some of these application modes for future use. Given the above example applications using OVERRIDE and DOWNGRADE, that leaves ADD as the only remaining mode, and the one I think we should use for the advantage/disadvantage counting behaviour implemented in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me. If an actor has both OVERRIDE 1 and OVERRIDE -1 or both DOWNGRADE -1 and UPGRADE 1, do we simply want to let AEs handle them the standard way or should we treat this as 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these modes should probably do nothing, i.e. not modify the source value at all, to make it clear that they are unsupported at this time. People hopefully won't create a bunch of AEs using OVERRIDE, thinking that it works one way, and then find out later that we implemented different behaviour.

So, in summary: OVERRIDE, UPGRADE, DOWNGRADE, and MULTIPLY do not make any changes at all, and ADD applies advantage/disadvantage per the rules on multiple sources of advantage and disadvantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I was too fast for you, I've made a second PR here #4890
But I can adjust the remaining 3 methods if we feel strongly about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's fine. I only proposed they do nothing in the absence of actual supported functionality, but if you've added the support for them already then that's great. Sorry for the churn. I see there are two commits so I can just review the first one and if that's good we can go with that, thank you.

const delta = this._castChangeDelta(change.value);
if ( change.mode === CONST.ACTIVE_EFFECT_MODES.CUSTOM ) {
return this._applyChangeCustom(value, delta, model, change);
}
switch (delta) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch (delta) {
switch ( delta ) {

case 1:
this.#advantage++;
break;
case -1:
this.#disadvantage++;
break;
}
return Math.sign(this.#advantage) - Math.sign(this.#disadvantage);
}
}
4 changes: 3 additions & 1 deletion module/data/shared/roll-config-field.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import AdvantageModeField from "../fields/advantage-mode-field.mjs";

const { StringField, NumberField, SchemaField } = foundry.data.fields;

/**
Expand All @@ -20,7 +22,7 @@ export default class RollConfigField extends foundry.data.fields.SchemaField {
roll: new SchemaField({
min: new NumberField({...opts, label: "DND5E.ROLL.Range.Minimum"}),
max: new NumberField({...opts, label: "DND5E.ROLL.Range.Maximum"}),
mode: new NumberField({choices: [-1, 0, 1], initial: 0, label: "DND5E.AdvantageMode"}),
mode: new AdvantageModeField(),
...roll
}),
...fields
Expand Down