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

feat(UI): make empty-handed unarmed damage bonus more visible #4241

Merged
merged 9 commits into from
Feb 22, 2024

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented Feb 20, 2024

Purpose of change

It was brought up in #4160 that it might be good to actually tell the player about the bonus somewhere.

Describe the solution

C++ changes:

  1. In character_display.cpp and character_display.h, defined new function character_display::display_empty_handed_base_damage. This prints a number that not only sums up the skill bonus you get, but for bonus points tallies up bonuses from the Fingertip Razors CBM and from mutations (i.e. cut_dmg_bonus, rand_cut_bonus, and the like). In the case of rand_bash_bonus and rand_cut_bonus however it only takes into account min, since this only generates a single number as the output.
  2. Also in character_display.cpp, draw_skills_tab now shows the output of the above number next to your unarmed skill, just like how dodge skill shows your character's effective dodge rating.
  3. In player.cpp, set character_martial_arts::pick_style so the "pick a style" menu also shows this number below your stats summary, and as a bonus it also shows your effective dodge since both this numbers are relevant.

JSON changes:

  1. Rewrote the description for unarmed skill. All other melee skills actually tell you what they do, so now instead of just saying you'll hurt yourself (technically false unless you count getting flexed on for trying to throw hands with a hulk at low skill), it clarifies it affects accuracy and is used for techniques (also no longer implying that skill alone unlocks martial arts techniques, given those are tied to styles), and it now mentions the empty-handed bonus.

Describe alternatives you've considered

Making the function properly track the result of random damage bonuses by, if present, printing min and max as two separate numbers.

Testing

  1. Compiled and load-tested.
  2. Bumped up my unarmed skill, confirmed the number on the @ screen goes up by the expected amount.
  3. Installed fingertip razors, confirmed it goes up by +8.
  4. Put on a shield, the bonus from razors goes down to only +4.
  5. Put on gloves and observed the bonus from bionics went away entirely.
  6. Took off the gloves and shield, gave myself Chitinous Armor. Confirmed I get +4 from its bash_dmg_bonus of 2 if both hands are free, and +2 if only one hand is.
  7. Swapped in Slime Hands, confirmed this gives the expected +5 with a shield, +10 with both hands free.
  8. Recruited an NPC and looked at their status screen, confirmed it correctly shows the damage bonus.
  9. Checked affected C++ files for astyle.

image
image

Additional context

Checklist

@github-actions github-actions bot added src changes related to source code. JSON related to game datas in JSON format. labels Feb 20, 2024
@chaosvolt chaosvolt changed the title feat(interface): make empty-handed unarmed damage bonus more visible feat(UI): make empty-handed unarmed damage bonus more visible Feb 20, 2024
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make this member function (aka methods), Character::display_empty_handed_base_damage() into a free function display_empty_handed_base_damage() in character_display.cpp.

Rationale

We should avoid using/adding member functions at all, in particular for large classes like Character.

  1. Character class is gigantic, and is depended by everything. Changing a single line of character.h usually results in 300 files having to be rebuilt again, which means ~5 minutes of extra time on average PC.
  2. display_empty_handed_base_damage() only has single usage in bool character_martial_arts::pick_style() and nowhere else.
  3. it look like does not require any private information of Character, which means its functionality can be replicated as regular functions.

therefore, it looks like there's no pros of it being a member function but only cons, thus i suggest we move this function into character_display.cpp.

@chaosvolt
Copy link
Member Author

chaosvolt commented Feb 21, 2024

Ah, seems reasonable then. What of the use in player.cpp? I assume that won't work the same if it's not a character function?

EDIT: I think I have an idea for that.

@chaosvolt
Copy link
Member Author

Okay so this errors:

1>C:\Users\Vincent\Documents\GitHub\Cataclysm-BN\src\character_display.h(39,61): error C2270: 'display_empty_handed_base_damage': modifiers not allowed on nonmember functions (compiling source file ..\src\armor_layers.cpp)

I don't even remotely know what the fuck that means.

@chaosvolt chaosvolt marked this pull request as draft February 21, 2024 02:41
Copy link
Contributor

autofix-ci bot commented Feb 21, 2024

The Autofix app has found code style violation and automatically formatted this Pull Request.

I locally edit my commits (e.g: git, github desktop)

Please choose following options:

I'd like to accept the automated commit
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

This PR is complete and I don't want to edit it anymore

It's safe to ignore this message.

I edit this PR through web UI

You can ignore this message and continue working.

I have no idea what this message is talking about

You can ignore this message and continue working. If you find any problem, please ask for help and ping @scarf005.

src/character_display.h Outdated Show resolved Hide resolved
chaosvolt and others added 2 commits February 20, 2024 20:58
Co-authored-by: scarf <greenscarf005@gmail.com>
@chaosvolt
Copy link
Member Author

And augh I hate this already ffs

1>C:\Users\Vincent\Documents\GitHub\Cataclysm-BN\src\player.cpp(230,90): error C2664: 'int character_display::display_empty_handed_base_damage(Character &)': cannot convert argument 1 from 'const avatar' to 'Character &'
1>C:\Users\Vincent\Documents\GitHub\Cataclysm-BN\src\player.cpp(230,86): message : Conversion loses qualifiers
1>C:\Users\Vincent\Documents\GitHub\Cataclysm-BN\src\character_display.h(39,5): message : see declaration of 'character_display::display_empty_handed_base_damage'

src/character_display.h Outdated Show resolved Hide resolved
src/character_display.cpp Outdated Show resolved Hide resolved
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the input parameter should be made const as it's not modifying Character in any way; this helps us ensure it's not chainging character state and make debugging easier

Co-authored-by: scarf <greenscarf005@gmail.com>
src/character_display.cpp Outdated Show resolved Hide resolved
chaosvolt and others added 2 commits February 20, 2024 22:00
Co-authored-by: scarf <greenscarf005@gmail.com>
@chaosvolt chaosvolt marked this pull request as ready for review February 21, 2024 04:14
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeet

@chaosvolt chaosvolt merged commit c0cd820 into cataclysmbnteam:main Feb 22, 2024
13 checks passed
@chaosvolt chaosvolt deleted the punchy-description branch February 22, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants