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.

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.

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'.

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.

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.

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?

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.

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.

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.