kakaroto / Beyond20

D&D Beyond Character Sheet Integration in Roll20
GNU General Public License v3.0
497 stars 145 forks source link

Weapon Attack Damage not applied in Calculation Native Rolls #1040

Closed cckndson closed 1 year ago

cckndson commented 1 year ago

Describe the bug When using Native Rolls in Foundry VTT and making a weapon attack specifically and rolling with Beyond20, the actual weapon damage won't apply. It works for spells and features such as sneak attack, but not the actual weapon damage which appears to be because when the item card is made the weapon attack damage is in brackets such as {1d8+5}[piercing] instead of 1d8+5[piercing]. I have tried to overwrite this by simply removing the brackets but when rolling via Beyond20 it simply overwrites the item on the next roll and re-inserts the brackets. It is not just the existence of something like sneak attack either as it does the same for characters without said features. Testing from within Foundry has shown that removing the brackets does fix the issue, and leaving them causes the same issue even when not rolling from Beyond20 so it is purely the brackets causing the issue.

To Reproduce Steps to reproduce the behavior:

  1. Turn on Use Native Rolls in Foundry VTT
  2. Have Midi QOL and have it set to apply damage to cards.
  3. Roll a melee or ranged attack from DNDBeyond.
  4. Damage calculations don't work as expected because of the brackets around the damage.
  5. See error

Expected behavior For damage to be calculated and applied like spells, features, and anything else that doesn't appear to be a weapon attack. Ranged and melee both react the same.

Screenshots image image image image

Browser Info (please complete the following information):

Additional context This is probably such a small use case that it doesn't matter and probably one of those things that seem simple like "remove the brackets" but takes a lot more coding than just "removing the brackets" but wanted to bring it up as I saw similar things but not quite the experience I was receiving.

cckndson commented 1 year ago

I forgot to add a screenshot for what happens when the brackets are removed.

image

kakaroto commented 1 year ago

Thanks for the report! That's definitely an interesting issue. I think however that the bug is in the midi-qol module rather than in Beyond20, because I think {formula}[damage type] is a valid format in Foundry VTT. The reason we use it is because if we did 1d4 + 3[piercing] for example, it would think that the 1d4 has no damage type and only the 3 is of type piercing damage. Here's a good example showing how Foundry displays the rolls and their damages with and without the curly braces : image Here's another screenshot showing Foundry correctly recognizing multiple rolls as being of the same type when {} are used. image

As you can see, we don't put them around the sneak attack because the damage is in one operation, but we add the braces around formulas that have more than one mathematical operand, so that the user can properly see which damage applies to which type. This is especially useful in the case of Beyond20 where a lot of users will have things like a Smite, Dread ambusher, hexblade curse, or Sneak attack, etc.. always enabled in the roll and we let the player select which damages to apply and which to ignore depending on the situation, so if you can't see what the total is for each type, you can't make that decision. Similarly, midi-qol should be able to use that information to halve damage if you have fire resistance for example, and things like that. Without the { } around a 1d8 + 4 for example, to specify exactly what's of that damage type, it would only halve the 4, instead of the entire 1d8 + 4 as fire damage.

Due to the screenshot above, showing that the formula is valid, and Foundry VTT recognizes it as valid and recognizes that the dice rolled inside the formula in the curly braces does apply to the damage type specified between brackets, I believe the issue is in midi-qol not properly parsing or recognizing that formula so it breaks when it needs to apply the damage.

Removing the curly braces would unfortunately have a negative impact on our side, as they were added for a reason. I'm therefore reporting the bug to the Midi-QOL module to see what their developer says about this use case.

Thanks!

kakaroto commented 1 year ago

Link back to the issue reported on midi-qol repository : https://gitlab.com/tposney/midi-qol/-/issues/1087

cckndson commented 1 year ago

I appreciate you looking into it. I suspected it wouldn't be as simple as "remove the brackets" and figured you probably get something similar for issues that seem minor but in actuality it takes a lot to change that "minor" issue. I will follow the other thread and in the meantime figure out if I can do something on my end to make it work closer to the expected way.

kakaroto commented 1 year ago

I appreciate you looking into it. I suspected it wouldn't be as simple as "remove the brackets" and figured you probably get something similar for issues that seem minor but in actuality it takes a lot to change that "minor" issue. I will follow the other thread and in the meantime figure out if I can do something on my end to make it work closer to the expected way.

It turned out to be as simple as "replace the brackets with parenthesis" 😁 I've just released an update to the module which fixes it.

cckndson commented 1 year ago

I appreciate you looking into it. I suspected it wouldn't be as simple as "remove the brackets" and figured you probably get something similar for issues that seem minor but in actuality it takes a lot to change that "minor" issue. I will follow the other thread and in the meantime figure out if I can do something on my end to make it work closer to the expected way.

It turned out to be as simple as "replace the brackets with parenthesis" 😁 I've just released an update to the module which fixes it.

Thanks, I appreciate you. Have a new patreon member for your hard work!

Also I noticed crits don't get calculated. That a Beyond20 issue or Midi?

Aeristoka commented 1 year ago

I appreciate you looking into it. I suspected it wouldn't be as simple as "remove the brackets" and figured you probably get something similar for issues that seem minor but in actuality it takes a lot to change that "minor" issue. I will follow the other thread and in the meantime figure out if I can do something on my end to make it work closer to the expected way.

It turned out to be as simple as "replace the brackets with parenthesis" 😁 I've just released an update to the module which fixes it.

Thanks, I appreciate you. Have a new patreon member for your hard work!

Also I noticed crits don't get calculated. That a Beyond20 issue or Midi?

Can you provide more context on that? Screenshots and such?

cckndson commented 1 year ago

I appreciate you looking into it. I suspected it wouldn't be as simple as "remove the brackets" and figured you probably get something similar for issues that seem minor but in actuality it takes a lot to change that "minor" issue. I will follow the other thread and in the meantime figure out if I can do something on my end to make it work closer to the expected way.

It turned out to be as simple as "replace the brackets with parenthesis" 😁 I've just released an update to the module which fixes it.

Thanks, I appreciate you. Have a new patreon member for your hard work! Also I noticed crits don't get calculated. That a Beyond20 issue or Midi?

Can you provide more context on that? Screenshots and such?

Absolutely.

Here is a screenshot of damage calculation non-crit. image

Here is a screenshot of damage calculation of a crit. Rules for my games is that a crit is rolled dice + max damage dice. But this doesn't calculate crits in any way. image

Here is a screenshot of me rolling from the character sheet in Foundry and critting. No sneak attack on it but it does max out the dice for the 8 additional piercing for max damage dice. image

Please let me know if there is anything else I can provide.

kakaroto commented 1 year ago

oh yeah, I forgot about that, they mentionned it in the midi-qol issue. That one looks to be (maybe) a dnd5e issue, as it doesn't double dice inside of the parenthesis formula (or curly braces for that matter).

Will need to reach out to someone knowledgeable about the dnd5e system internals, hopefully they know why or how to fix it