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.

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

Unarmed Damage Die Implementation #148

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jean-bougeois-cruz
Copy link

@jean-bougeois-cruz jean-bougeois-cruz commented Nov 11, 2024

Type
What type of pull request is this? (e.g., Bug fix, Feature, Refactor, etc.)

  • Bug fix
  • Feature
  • Refactor
  • Other (please describe):

Description
Add logic to calculate damage die for unarmed strikes.

Related Issue
Closes #91

How Has This Been Tested?
Built and ran on local foundry. Screenshots below.

Screenshots (if applicable)
I had one rank in Athletics to confirm damage scales with Athletics but die scales with Str attribute.

Str 6

image

Str 1

image

Str 4

image

Str 8

image

Str 9

image

Checklist:

  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes do not introduce any new warnings or errors.
  • My PR does not contain any copyrighted works that I do not have permission to use.
  • I have tested my changes on Foundry VTT version: [insert version here].

Additional context
Add any other context or information here that would be useful for reviewers.

@MangoFVTT
Copy link
Collaborator

I have some concern about this implementation in that this means nobody can change the unarmed strike damage at all (outside of effects that apply to all damage). I see 2 probably better solutions:

  1. Make the unarmed strike damage formula a roll formula visible in the item's damage field, using math functions to calculate the correct amount of dice. This will be a really long formula probably, so it would look really ugly, but it would at least be easily changeable.

  2. Store the possible values of the unarmed strike as a range in CONFIG.COSMERE. That way it can be edited by a world script for users who wish to do so, and it also makes it so its not buried somewhere in the code that may be annoying to find.

@jean-bougeois-cruz
Copy link
Author

jean-bougeois-cruz commented Nov 11, 2024

I have some concern about this implementation in that this means nobody can change the unarmed strike damage at all (outside of effects that apply to all damage). I see 2 probably better solutions:

1. Make the unarmed strike damage formula a roll formula visible in the item's damage field, using math functions to calculate the correct amount of dice. This will be a really long formula probably, so it would look really ugly, but it would at least be easily changeable.

2. Store the possible values of the unarmed strike as a range in CONFIG.COSMERE. That way it can be edited by a world script for users who wish to do so, and it also makes it so its not buried somewhere in the code that may be annoying to find.

@MangoFVTT That is a valid concern. I like option 2, I can work on implementing that. If option 1 is preferable over 2 I'm cool with that as well.

@jean-bougeois-cruz
Copy link
Author

@MangoFVTT I added the possible die values of unarmed strike to CONFIG.COSMERE and am using that to determine unarmed damage. I've tested for all possible STR values and it's working for me locally.

Feel free to give me any suggestions or nits you want, this is my first time developing in Foundry so I'm out of my element!

@MangoFVTT
Copy link
Collaborator

Looks good to me, much better than hardcoding them for sure 👍

@stanavdb
Copy link
Collaborator

I have some further concerns that apply to the current implementation as well. I think the approach of having the unarmed strike damage depend on the id of the item is flawed. It's opaque to the user why the unarmed strike deals the damage it does and ignores the formula.
Having some item just ignore its config and deal some arbitrary amount of damage (for most users at least), just feels weird.

I think we should solve this using the formula. Though I am aware that fitting a function to the table may not be easy.

On another note; We should definitely move the other "tables" that we use for the derived statistics to the config as well. I don't believe they are right now.

@MangoFVTT
Copy link
Collaborator

MangoFVTT commented Nov 12, 2024

@stanavdb Perhaps the best way to do it would be to have the unarmed strike scale be an @'able property? In the dnd5e system for example you can refer to things like @barbarian.rage.scale that just automatically calculates the correct number but still allows you to call it in any damage field without hardcoding it to an item name or specific item. We could do the same here

@stanavdb
Copy link
Collaborator

@stanavdb Perhaps the best way to do it would be to have the unarmed strike scale be an @'able property? In the dnd5e system for example you can refer to things like @barbarian.rage.scale that just automatically calculates the correct number but still allows you to call it in any damage field without hardcoding it to an item name or specific item. We could do the same here

That's a great suggestion! Seems like the best way to do it.

@jean-bougeois-cruz
Copy link
Author

@stanavdb Perhaps the best way to do it would be to have the unarmed strike scale be an @'able property? In the dnd5e system for example you can refer to things like @barbarian.rage.scale that just automatically calculates the correct number but still allows you to call it in any damage field without hardcoding it to an item name or specific item. We could do the same here.

I'm going to see if I can figure out how to do this! I should be able too. If it's too steep a learning curve for me, I'll let you know so it can be taken care of by someone more fitting.

@stanavdb
Copy link
Collaborator

I'm going to see if I can figure out how to do this! I should be able too. If it's too steep a learning curve for me, I'll let you know so it can be taken care of by someone more fitting.

It should be fairly easy. Just move the calculations over to the getRollData function on the actor document.

@stanavdb
Copy link
Collaborator

I'm converting this a draft, as its still in progress

@stanavdb stanavdb marked this pull request as draft November 21, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants