Pondering the DexBonus | Foundry VTT | Page 1

hidden zenith Oct 4, 2024, 8:47 PM

#

@wild pebble

wild pebble Oct 4, 2024, 8:47 PM

#

Good thinking lol. Polluted the main channel plenty

#

Gonna grab the code block

const armorType = armors[0].system.type.value;
const baseDexBonus = CONFIG.DND5E.armorKey[armorType]?.maxDex ?? Infinity;
const currDexBonus = (armorData.dex ?? baseDexBonus) - baseDexBonus;
const potentialDexBonus = this.system.attributes.ac.bonuses[armorType] ?? 0;
ac.dex = baseDexBonus + Math.min(this.system.abilities.dex?.mod ?? 0, Math.max(currDexBonus, potentialDexBonus));

hidden zenith Oct 4, 2024, 8:48 PM

#

The simplest case to handle is probably when the item has a dex cap set.

wild pebble Oct 4, 2024, 8:49 PM

#

In that case, do we treat that cap as the base, or have we decided the base is always the CONFIG default

hidden zenith Oct 4, 2024, 8:49 PM

#

From a user standpoint, they'd probably want a simple and single property to configure via an AE.

wild pebble Oct 4, 2024, 8:50 PM

#

Should an override in an AE always win, then? Or are overrides a weird use case here

hidden zenith Oct 4, 2024, 8:51 PM

#

AEs would apply in their normal order of priority

#

They would just target the same property, and the final number if what would be used to determine the dex bonus

wild pebble Oct 4, 2024, 8:52 PM

#

Yeah, I meant vs the set value on the armor item. Unless you're thinking HB's way where all non-special armor gets a null value

#

Which honestly wouldn't be the worst migration script, just comparing the set value against the new CONFIG defaults and removing if the same

hidden zenith Oct 4, 2024, 8:55 PM

#

OK, so either (1) the item has a value set if it is meant to be explicit or (2) it has an empty input to defer to the config object

wild pebble Oct 4, 2024, 8:55 PM

#

Yeah that's cleanest I think

hidden zenith Oct 4, 2024, 8:55 PM

#

The system has certainly done worse changes for the end user to accomodate.

wild pebble Oct 4, 2024, 8:55 PM

#

Most people probably won't even notice the change lol

hidden zenith Oct 4, 2024, 8:56 PM

#

So if the AE is 7 and the armor has a value of 2, what should that be taken to mean?

wild pebble Oct 4, 2024, 8:57 PM

#

I'd say upgrade to 7. Generally I'd imagine the set value would only be for "this is better than usual" - not for worse. Though upon reflection, the entire field could be done away with, with magical armor that has a differing dex maximum just having an AE on it

#

I think my "upgrade to 7" is wrong though

hidden zenith Oct 4, 2024, 8:59 PM

#

Well is it meant to be "7 more than usual" or set to 7?

#

The basic idea is support for features that let you increase your armor dex bonus from 2 to 3

wild pebble Oct 4, 2024, 8:59 PM

#

Maybe "7 more than default"?

#

Okay, okay I think it'd go to 9 in the example, then

#

Max dex mod: actor data value + (armor value ?? armor default)

#

So long as the value in the actor's data is 0 by default regardless of armor type, I think that formula would account for everything

#

If no AE, you get the armor value, or the default for the armor if that's not there. If AE, apply it to whatever you'd normally be getting (again, the armor value if there, else the default). Special treatment for a null default

hidden zenith Oct 4, 2024, 9:03 PM

#

Yeah makes sense.

wild pebble Oct 4, 2024, 9:03 PM

#

Hm. I guess with light armor there'd be no way to set an actor-specific debuff for it though. I think that's fine

#

If you have max dex to light armor, it's probably because of the specific armor, not some condition

hidden zenith Oct 4, 2024, 9:05 PM

#

I think that's probably the way to go: if the type of armor does not have a cap, it cannot be modified

wild pebble Oct 4, 2024, 9:07 PM

#

Cool, so:

const armorType = armors[0].system.type.value;
const currDexBonus = armorData.dex ?? CONFIG.DND5E.armorKey[armorType];
const maxDexBonus = currDexBonus ? currDexBonus + (this.system.attributes.ac.bonuses[armorType] ?? 0) : Infinity;
ac.dex = Math.min(this.system.abilities.dex?.mod ?? 0, maxDexBonus);

hidden zenith Oct 4, 2024, 9:09 PM

#

Looks good

wild pebble Oct 4, 2024, 9:09 PM

#

Neat. Will see about trying to implement tomorrow, if you don't go for it

hidden zenith Oct 4, 2024, 9:10 PM

#

This placeholder should be changed as well.

wild pebble Oct 4, 2024, 9:11 PM

#

Ah yeah. Just grab from the config

hidden zenith Oct 4, 2024, 9:12 PM

#

This does mean there's no need for a migration, I think?

#

If a value is set in the item, it can remain.

wild pebble Oct 4, 2024, 9:13 PM

#

Huh. I thought we'd still need to clear out the ones that match the default but looking at the second line I wrote maybe not after all

hidden zenith Oct 4, 2024, 9:17 PM

#

One thing this changes, though, is that any medium armor with no value in that field will be treated as having a cap.

wild pebble Oct 4, 2024, 9:19 PM

#

That's true. That I suppose could be migrated. Though I guess the solution would be to give it like 100

#

Or a "no max dex" checkbox but I 1. Hate that and 2. Think it's overkill

hidden zenith Oct 4, 2024, 9:20 PM

#

That is what I was thinking though

#

Could be added to the Properties set.

wild pebble Oct 4, 2024, 9:20 PM

#

I mean uh I love it

#

Okay actually in properties I don't hate it

#

I was picturing it beside the existing input box

#

How many RAW medium armor sets have no cap?

hidden zenith Oct 4, 2024, 9:22 PM

#

Anything with no value set would be migrated to have "Unlimited Dexterity" (or a better name) checked in system.properties

#

Not many. Probably none.

#

But I think that's besides the point.

#

Right now you can make medium armor with no cap, and we shouldn't suddenly break that item.

#

Ideally, at least. Not like it has not happened before. And would be an easy fix if a new property was added.

wild pebble Oct 4, 2024, 9:24 PM

#

Yeah agreed. Was just curious how many items would be impacted by such a migration

#

Probably need a different name than just .bonuses.heavy for instance though - that looks like it means extra AC when wearing heavy armor

hidden zenith Oct 4, 2024, 9:44 PM

#

ac.ability.light, .medium, .heavy

wild pebble Oct 4, 2024, 9:45 PM

#

That'll do it

wild pebble Oct 5, 2024, 5:17 PM

#

hidden zenith Anything with no value set would be migrated to have "Unlimited Dexterity" (or a...

Thoughts on the abbreviation (if any - noticed some are just fully written out) for this property?

#

Also - such a property would have to ~~be valid~~ appear for all Equipment, right? Unless filtering was done on the properties in the template itself?

wild pebble Oct 5, 2024, 5:38 PM

#

And finally (I think) - where would the migration go for armor with no currently set max dex bonus? Just peeking around it looks like there's generally 2 types of migration, a lazy version that happens each time an item is opened (not ideal here, since we do want to let people leave it empty without it automatically going unlimited dex bonus) and one that goes on a new system version - but I'm not 100% sure I'm right about that

hidden zenith Oct 5, 2024, 8:36 PM

#

wild pebble Thoughts on the abbreviation (if any - noticed some are just fully written out) ...

The abbreviated properties are done so due to legacy reasons. Any new properties should be written out fully, like stealthDisadvantage and weightlessContents.

hidden zenith Oct 5, 2024, 8:36 PM

#

wild pebble Also - such a property would have to ~~be valid~~ appear for all Equipment, righ...

There is a method in the system data model for filtering the shown properties.

wild pebble Oct 5, 2024, 8:36 PM

#

Ah cool, will take a peek for the latter

#

Went with noMaxDex for the property name but definitely not married to it

hidden zenith Oct 5, 2024, 8:37 PM

#

wild pebble And finally (I think) - where would the migration go for armor with no currently...

There are one-time migrations and data model migrations, yes. The latter is called every time the item is updated, as well as when it is initialized.

hidden zenith Oct 5, 2024, 8:38 PM

#

wild pebble Went with `noMaxDex` for the property name but definitely not married to it

uncappedAbility perhaps

wild pebble Oct 5, 2024, 8:38 PM

#

Works for me, will change along with the property filtering

hidden zenith Oct 5, 2024, 8:39 PM

#

I had a PR once to do similar changes to armor, and part of it was to add the ability to change what Ability was used rather than hardcode it to Dexterity. If someone did something like that in the future, it would be nice if the property did not have 'dex' in the name.

wild pebble Oct 5, 2024, 8:39 PM

#

That makes sense, yeah

wild pebble Oct 5, 2024, 8:58 PM

#

General style question for the repo - there a preference between

ac.ability = Object.fromEntries(Object.keys(CONFIG.DND5E.armorMaxDex).map(i => [i, ""]))
``` and
```js
ac.ability = {};
for ( const type in CONFIG.DND5E.armorMaxDex ) {
  ac.ability[type] = "";
}

hidden zenith Oct 5, 2024, 9:00 PM

#

Well the first is 3 loops, isnt it? The fromEntries, the .keys and the map

wild pebble Oct 5, 2024, 9:00 PM

#

Ah yeah it is. Fewer lines, worse performance. Good point

hidden zenith Oct 5, 2024, 9:01 PM

#

I get shit on for my syntax all the time, but I would--

ac.ability = {};
for ( const k of Object.keys(CONFIG.DND5E.xxx) ) ac.ability[k] = "";

wild pebble Oct 5, 2024, 9:02 PM

#

I may do that - I don't like doing a for in if I can avoid it

#

Gotta run (away from PC at least) but put in the name changes & cleaned up a bit. Agreed it might make sense to merge with armorTypes but will also have to look at where that actually gets used to see how complex of a change that'd be

wild pebble Oct 5, 2024, 9:54 PM

#

Thinking idly - could probably just make the existing values into objects with label properties and leave shield without a maxAbilityBonus property or whatever - technically we'd then potentially run into a "undefined vs null" if someone were to check for armorTypes[someType].maxAbilityBonus, but I don't think that would matter. The system would only check it for an actual (non-shield) armor item

hidden zenith Oct 5, 2024, 9:55 PM

#

Sounds fine to me.
The object should, at the very least, contain nested objects. Flat enums are a no-no.

#

Did you consider the case where the worn armor is not light/medium/heavy? Actually not sure that's even possible, though, come to think of it.

wild pebble Oct 5, 2024, 10:03 PM

#

Natural, or something added via world script, are I think the only other options. But if a null or undefined value is found when doing AC calc it'll treat it as infinite if there's no default.
I was thinking armorTypes.heavy would be { label: "theExistingValue", maxAbility: 0 } for instance - that's what you mean by nested?

hidden zenith Oct 5, 2024, 10:04 PM

#

Yes

#

There is a patchConfig method you can use for this migration

wild pebble Oct 5, 2024, 10:05 PM

#

Ah yeah I guess this wouldn't have been the first time something like this gets updated

hidden zenith Oct 5, 2024, 10:05 PM

#

No, it's happened about 50 times

#

I think all the objects were flat at some point.

wild pebble Oct 5, 2024, 10:09 PM

#

At this point new ones should probably be too just to save effort later lol

wild pebble Oct 5, 2024, 11:45 PM

#

Only places I can find that make use of CONFIG.DND5E.armorTypes:
In config.mjs:

DND5E.equipmentTypes = {
  ...DND5E.miscEquipmentTypes,
- ...DND5E.armorTypes
+ ...Object.values(DND5E.armorTypes).map(t => t.label)
};
```proposed change above

In `migration.mjs` & `actor.mjs`:
`Object.keys(CONFIG.DND5E.armorTypes)` (can be ignored, keys aren't changing)

In `eqipment.mjs`:
`return this.type.value in CONFIG.DND5E.armorTypes;` (can be ignored, same reason)

In `item-sheet-2.mjs`:
```diff
context.equipmentTypes = [
  ...Object.entries(CONFIG.DND5E.miscEquipmentTypes).map(([value, label]) => ({ value, label })),
- ...Object.entries(CONFIG.DND5E.armorTypes).map(([value, label]) => ({ value, label, group: "DND5E.Armor" }))
+ ...Object.entries(CONFIG.DND5E.armorTypes).map(([value, { label }]) => ({ value, label, group: "DND5E.Armor" }))
];
```proposed change above

In `equipment.hbs`:
```diff
<optgroup label="{{ localize "DND5E.Armor" }}">
-   {{selectOptions config.armorTypes selected=system.type.value}}
+   {{selectOptions config.armorTypes selected=system.type.value labelAttr="label"}}
</optgroup>
```proposed change above

Then there's this, in `starting-equipment.mjs`, and I'm not entirely sure what modification would be necessary:
```js
static CATEGORIES = {
  armor: {
    label: "DND5E.Armor",
    config: "armorTypes"
  },
  focus: {
    label: "DND5E.Focus.Label",
    config: "focusTypes"
  },
  tool: {
    label: "TYPES.Item.tool",
    config: "toolTypes"
  },
  weapon: {
    label: "TYPES.Item.weapon",
    config: "weaponProficiencies"
  }
};

Checking to see how this is used now

#

(Also, in retrospect, this could just have been another push to the PR, so feel free to ignore & evaluate once pushed)

wild pebble Oct 7, 2024, 1:17 PM

#

Okey, all done now. Would appreciate a review if you have the time, to make sure I haven't done anything dumb

hidden zenith Oct 7, 2024, 1:18 PM

#

Already looked at it, no immediate problems stood out to me.

#

It's all in Kim/Arbron's hands now

wild pebble Oct 7, 2024, 1:19 PM

#

Sounds good, thanks

hidden zenith Oct 7, 2024, 11:26 PM

#

Are there first-party instances of bonuses that apply only when wearing light armor or no armor?

wild pebble Oct 7, 2024, 11:32 PM

#

I can't think of any single bonus that would apply to ~~either~~ both. Though admittedly my time spent actually playing/being familiar with dnd is dwarfed by my time coding for it so I’m not at all confident about that

wild pebble Oct 8, 2024, 12:04 AM

#

There are a good chunk of the unarmored ones that only apply if also un-shielded (like bracers of defense)

#

But probably beyond the scope of this PR to try to account for that. An "unshielded" bonus is fine, but then might need armored unshielded and unarmored unshielded. Too many keys

wild pebble Oct 8, 2024, 1:18 AM

#

Mm. Swashbuckler (monster not subclass). Suave Defense: add cha mod to AC when wearing light armor or no armor. Though as that's not a PC, unlikely to necessitate these attributes needing to be set

hidden zenith Oct 8, 2024, 8:29 AM

#

Thanks for checking. We have seen examples in premium content as well, so I'm not sure that it's not worth looking into.

wild pebble Oct 8, 2024, 11:10 AM

#

I guess a potential solution would be splitting the armored bonus into armor types, like the max ability one?

hidden zenith Oct 8, 2024, 11:30 AM

#

There's precedent, for sure. Like how attack/damage bonuses are split.

#

However, I think it makes sense to have both.

wild pebble Oct 8, 2024, 11:31 AM

#

Yeah true, better not to have 3 keys need to be set for just one effect

hidden zenith Oct 8, 2024, 11:31 AM

#

ac.bonuses.unarmored, .armored, .light, .medium, .heavy, .shield

wild pebble Oct 8, 2024, 11:32 AM

#

That second one supposed to be .armored?

hidden zenith Oct 8, 2024, 11:32 AM

#

Maybe it's worth it to ping Arbron or Kim on the PR and get their input.

#Pondering the DexBonus