search

RedReign / FoundryVTT-BetterRolls5e

A module for modifying certain sheet functions on Foundry VTT Character sheets for D&D 5th Edition.
GNU General Public License v3.0
37 stars 67 forks source link

Fix hasUses when using a string rather than a number #321

Closed mech-tools closed 3 years ago

mech-tools commented 3 years ago

Hello,

This is a fix for uses consumption.

With the latest 5e version, the max field can accept strings and numbers. A lot of items are now referencing @attributes or @classes to get the maximum usage. image

Previously the value was cast to a Number which resulted in NaN making BetterRolls believe that the item didn't have charges configured.

This PR just remove the assumption that max uses is always a Number.

CarlosFdez commented 3 years ago

Interesting. Does 5e core use Roll.replaceFormulaData() for that or does it just check for non-falsey?

If they use Roll.replaceFormulaData() and check the numerical result of that then we should prob do that, but if they just check for non-falsey then I'll merge this in.

mech-tools commented 3 years ago

@CarlosFdez Not that I could see.

5e system doesn't even test the max value: https://gitlab.com/foundrynet/dnd5e/-/blob/master/module/item/entity.js#L540

As for knowing if an item has uses, they rely on the dropdown rather than mix/max: https://gitlab.com/foundrynet/dnd5e/-/blob/master/module/item/entity.js#L434

mech-tools commented 3 years ago

@CarlosFdez Well I was wrong. They transform the data upon rolling: https://gitlab.com/foundrynet/dnd5e/-/blob/master/module/item/entity.js#L254

But I don't think that's relevant in our case. Cause just like 5e system, we don't need to test max. They don't.

CarlosFdez commented 3 years ago

Ah, seems they just convert it to a boolean. Should be fine then, though I think its better if the conversion happened.