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

Pruning the old to-hit syntax - Part 9 #76327

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Karol1223
Copy link
Contributor

@Karol1223 Karol1223 commented Sep 10, 2024

Summary

None

Purpose of change

Continuation from #76281

Describe the solution

Worth to point out that I also removed damage values from a bunch of stuff that didn't have a defined to_hit anyway, as a bunch of apparel was doing damage where it should not have in any reasonable situation done so

Affected files:

  • armor\cardboard.json
  • armor\exotic.json
  • armor\eyewear.json
  • armor\head_attachments.json
  • armor\helmets.json
  • armor\hoods.json
  • armor\integrated.json
  • armor\jewelry.json
  • armor\legs_armor.json
  • armor\masks.json
  • armor\pets_dog_armor.json
  • armor\storage.json

Describe alternatives you've considered

Testing

Additional context

The next PR should finish off the armor folder

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Code: Tests Measurement, self-control, statistics, balancing. Items: Armor / Clothing Armor and clothing astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Sep 10, 2024
@Karol1223 Karol1223 closed this Sep 10, 2024
@Karol1223 Karol1223 reopened this Sep 10, 2024
@akrieger
Copy link
Member

Legit looking failures.

(~[slow] ~[.],starting_items)=>   CHECK( calc_expected_dps( weap.first ) == weap.second )
(~[slow] ~[.],starting_items)=> with expansion:
Error: (~[slow] ~[.],starting_items)=>   Approx( 5.3147823341 ) == 8.42
(~[slow] ~[.],starting_items)=> with message:
(~[slow] ~[.],starting_items)=>   integrated_chitin3_claws's dps changed, if it's intended replace the value in
(~[slow] ~[.],starting_items)=>   the respective file in data/mods/TEST_DATA/expected_dps_data.
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> ../tests/effective_dps_test.cpp:294: FAILED:
(~[slow] ~[.],starting_items)=>   CHECK( calc_expected_dps( weap.first ) == weap.second )
(~[slow] ~[.],starting_items)=> with expansion:
Error: (~[slow] ~[.],starting_items)=>   Approx( 7.4637222144 ) == 11.31
(~[slow] ~[.],starting_items)=> with message:
(~[slow] ~[.],starting_items)=>   integrated_giant_pincer's dps changed, if it's intended replace the value in
(~[slow] ~[.],starting_items)=>   the respective file in data/mods/TEST_DATA/expected_dps_data.

@Karol1223
Copy link
Contributor Author

Karol1223 commented Sep 23, 2024

Legit looking failures.

(~[slow] ~[.],starting_items)=>   CHECK( calc_expected_dps( weap.first ) == weap.second )
(~[slow] ~[.],starting_items)=> with expansion:
Error: (~[slow] ~[.],starting_items)=>   Approx( 5.3147823341 ) == 8.42
(~[slow] ~[.],starting_items)=> with message:
(~[slow] ~[.],starting_items)=>   integrated_chitin3_claws's dps changed, if it's intended replace the value in
(~[slow] ~[.],starting_items)=>   the respective file in data/mods/TEST_DATA/expected_dps_data.
(~[slow] ~[.],starting_items)=> 
(~[slow] ~[.],starting_items)=> ../tests/effective_dps_test.cpp:294: FAILED:
(~[slow] ~[.],starting_items)=>   CHECK( calc_expected_dps( weap.first ) == weap.second )
(~[slow] ~[.],starting_items)=> with expansion:
Error: (~[slow] ~[.],starting_items)=>   Approx( 7.4637222144 ) == 11.31
(~[slow] ~[.],starting_items)=> with message:
(~[slow] ~[.],starting_items)=>   integrated_giant_pincer's dps changed, if it's intended replace the value in
(~[slow] ~[.],starting_items)=>   the respective file in data/mods/TEST_DATA/expected_dps_data.

They look legit, and they're the reason I'm not doing anything with this PR, but I don't know whether they actually are legit.

From my understanding, unarmed weapons do not use to-hit at all when worn. The ones here are integrated, so should only ever be worn. For the weapons the tests cry about, to-hit is the only aspect I changed.

What this means is either unarmed weapons do use to-hit, or the DPS calculation (and by extent DPS display in item description) lies to the player because it calculates the DPS with to-hit in mind when in reality it doesn't use it. I am assuming it is the latter, and that the tests don't make an exception for integrated armor either, and the test character just wields it anyway (which isn't possible in standard gameplay to my understanding)

As such, there's a problem of... it looks like unarmed weapons have two separate DPS values at once - one when wielded, and one when worn. This is bad.

@Maleclypse
Copy link
Member

@anothersimulacrum I think you might be the best person to answer the above question.

@anothersimulacrum
Copy link
Member

anothersimulacrum commented Oct 11, 2024

To-hit is not applied when using worn weapons. I think this should be considered a bug.

Hit is determined:

int hit_spread = t.deal_melee_attack( this, hit_roll() );

Worn weapons enter the equation:

Cataclysm-DDA/src/melee.cpp

Lines 775 to 786 in 11886ea

if( contact_area != sub_body_part_sub_limb_debug ) {
// todo: simplify this by using item_location everywhere
// so only cur_weapon = worn.current_unarmed_weapon remains
// Check if our vector allows armor-derived damage
if( vector_id->armor_bonus ) {
item *worn_weap = worn.current_unarmed_weapon( contact_area );
cur_weapon = worn_weap ? item_location( *this, worn_weap ) : item_location();
cur_weap = cur_weapon ? *cur_weapon : null_item_reference();
add_msg_debug( debugmode::DF_MELEE, "Vector allows armor damage calculation, chosen weapon %s",
cur_weap.display_name() );
}
}

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Tests Measurement, self-control, statistics, balancing. Items: Armor / Clothing Armor and clothing [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants