Fixing error thrown when unloading gun when there is no inventory available #48999

lancewf · 2021-05-22T21:28:12Z

Summary

BugFixes "Unloading gun you're wielding when you have nowhere to store its ammo leads to an error"

Purpose of change

Fixes #29839
Fixes #47001
Fixes #48842

To reproduce the bug:

Spawn a ruger_redhawk revolver
Unload the weapon with "U"
Drop the .44 bullets
Drop all other items, but the revolver
Load one bullet in the revolver with "r"
Unload the weapon again with "U". After this action (before the code change) the player would be promoted to drop their weapon. Then when dropping the weapon an error would be displayed.

This problem happens with shotguns or any other gun without a magazine that takes more than one round.

Describe the solution

The problem only happened when a player only had inventory space for the unloaded bullets in the gun they are unloading. The game thinks it has inventory space to store the bullets, but it does not. Then it tries to drop the gun and store the bullets in the gun at the same time. The solution is to not check for inventory space in the gun that is being unloaded. Then no inventory space will be found and the bullets will be dropped on the ground.

Describe alternatives you've considered

An alternative is the #48895 PR that I have confirmed solves #29839 also.

Although they both solve the issue they are solving different problems in the code. This PR excludes the item being unloaded's inventory volume from being counted to placed the unloaded items. The other PR fixes how we check if the wielded item is being unloaded. Both changes are needed, but either will fix the three issues.

Below is the section of code that both PRs fix. Line 12807 is where this PR fixes the problem. This line is checking if there is space to store the unloaded items. Line 12812 is where the other PR fixes the problem. This line checks if the item unloaded is in the item wielded. If Line 12807 is fixed (this PR) then the code will not get to line 12812 (other PR) because it will find that there is not enough space to store the unloaded item and it will drop it or place it in the vehicle (line 12808). If line 12807 is not fixed and line 12812 (other PR) is, it will get to line 12812 and set allow_wield to false because the unloaded item is part of the wielded item. Then the items will be dropped from line 12814.

Cataclysm-DDA/src/character.cpp

Lines 12801 to 12829 in 38a368d

    
           bool Character::add_or_drop_with_msg( item &it, const bool /*unloading*/, const item *avoid ) 
        
           { 
        
               if( it.made_of( phase_id::LIQUID ) ) { 
        
                   liquid_handler::consume_liquid( it, 1, avoid ); 
        
                   return it.charges <= 0; 
        
               } 
        
               if( !this->can_pickVolume( it ) ) { 
        
                   put_into_vehicle_or_drop( *this, item_drop_reason::too_large, { it } ); 
        
               } else if( !this->can_pickWeight( it, !get_option<bool>( "DANGEROUS_PICKUPS" ) ) ) { 
        
                   put_into_vehicle_or_drop( *this, item_drop_reason::too_heavy, { it } ); 
        
               } else { 
        
                   const bool allow_wield = !weapon.has_item( it ) && weapon.magazine_current() != &it; 
        
                   const int prev_charges = it.charges; 
        
                   auto &ni = this->i_add( it, true, avoid, /*allow_drop=*/false, /*allow_wield=*/allow_wield ); 
        
                   if( ni.is_null() ) { 
        
                       // failed to add 
        
                       put_into_vehicle_or_drop( *this, item_drop_reason::tumbling, { it } ); 
        
                   } else if( &ni == &it ) { 
        
                       // merged into the original stack, restore original charges 
        
                       it.charges = prev_charges; 
        
                       put_into_vehicle_or_drop( *this, item_drop_reason::tumbling, { it } ); 
        
                   } else { 
        
                       // successfully added 
        
                       add_msg( _( "You put the %s in your inventory." ), ni.tname() ); 
        
                       add_msg( m_info, "%c - %s", ni.invlet == 0 ? ' ' : ni.invlet, ni.tname() ); 
        
                   } 
        
               } 
        
               return true; 
        
           }

We need to fix line 12807 (this PR) because it will make the code less confusing to get to line 12812 when there is not enough volume to place the unloaded item. How I read this code is "Do we have enough volume", "Do we have too much weight", then place the item.

We need to fix line 12812 (other PR) because it is not working correctly. It is checking if the unloaded item is in the weapon which it is and currently it is returning that it is not.

Testing

There are unit tests created for the problem. There were no problems found when testing this change with other types of weapons.

Manual testing with the wielded item below. With full and partially loaded items.

sw_610
sw_500
m2browning_sawn
ruger_redhawk
sw629
ruger_lcr_38
sw_619
sew kit
STANGA magazine

Additional context

Signed-off-by: Lance Finfrock <lancewf@gmail.com> Added unit test Signed-off-by: Lance Finfrock <lancewf@gmail.com>

pehamm · 2021-05-22T21:56:29Z

src/character.cpp

 {
-    if( weapon.can_contain( it ) ) {
+    if( weapon.can_contain( it ) && ( avoid == nullptr || &weapon != avoid )) {


Suggested change

if( weapon.can_contain( it ) && ( avoid == nullptr || &weapon != avoid )) {

if( weapon.can_contain( it ) && ( avoid == nullptr || &weapon != avoid ) ) {

I believe this is why the astyle check fails

Signed-off-by: Lance Finfrock <lancewf@gmail.com>

lancewf · 2021-05-22T22:32:20Z

src/activity_actor.cpp

@@ -1646,7 +1646,7 @@ void unload_activity_actor::unload( Character &who, item_location &target )
        if( contained->ammo_type() == ammotype( "plutonium" ) ) {
            contained->charges /= PLUTONIUM_CHARGES;
        }
-        if( who.as_player()->add_or_drop_with_msg( *contained, true ) ) {
+        if( who.as_player()->add_or_drop_with_msg( *contained, true, &it ) ) {


This avoids counting the volume of the item that is being unloaded as space the unloaded items can go to.

lancewf · 2021-05-22T22:43:59Z

src/character.cpp

 {
-    if( weapon.can_contain( it ) ) {
+    if( weapon.can_contain( it ) && ( avoid == nullptr || &weapon != avoid ) ) {
        return true;
    }
    for( const item &w : worn ) {


We might want to add the avoid check here also, but I could not find a case with non-weapons that cause a problem.

actual-nh

These will hopefully fix the astyle objections.

tests/unload_naked_test.cpp

Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>

lancewf · 2021-05-23T00:09:44Z

@actual-nh Thank you for fixing the astyle check.

I am not sure how to read the problem with the Clang-tidy error. I am not even sure which file is the problem, but I am assuming it is the "unload_naked_test.cpp".

pehamm · 2021-05-23T00:28:50Z

You can ignore the clang-tidy error, it is caused by a problem with a python library needed for the workflow and should be fixed once #49000 is merged.

RoyBerube · 2021-05-23T13:04:16Z

This looks very similar to what is being addressed in PR #48895, although the code fixes it in a different way.

actual-nh · 2021-05-23T16:15:53Z

This looks very similar to what is being addressed in PR #48895, although the code fixes it in a different way.

Perhaps a combination of the approaches? The tests from this PR are definitely a good thing to include.

lancewf · 2021-05-23T16:38:46Z

This looks very similar to what is being addressed in PR #48895, although the code fixes it in a different way.

I confirmed that #48895 does fix this issue. Although they both solve the issue they are solving different problems in the code. This PR excludes the volume from the item being unloaded for a place to put the items. The other PR fixes how we check if the wielded item is being unloaded. To me, it looks like both changes are needed.

I am testing them both together now.

lancewf · 2021-05-23T22:56:10Z

src/character.cpp

+                break;
+            }
+        }
+        const bool allow_wield = !wielded_has_it && weapon.magazine_current() != &it;


This section of code was cherry-picked from #48895. I think both these changes are needed for this fix.
If we want these changes we can merge #48895 first or only merge this PR.

Signed-off-by: Lance Finfrock <lancewf@gmail.com>

lancewf · 2021-05-23T23:09:35Z

I cherry-picked the commit from #48895 (I hope this is ok @RoyBerube) to combine the two PRs. There were no problems found when testing the combined PRs with a few guns, a magazine, and the sewing kit.

actual-nh · 2021-05-28T19:46:59Z

@lancewf: #48895 is now in; do you need to rebase this PR?

lancewf · 2021-05-28T19:54:41Z

@actual-nh We should not need to because the commit (60976a6) from #48895 was pulled into this branch.

…ilable (CleverRaven#48999) * Adding unloaded gun to avoid * Added unit test * Unload partial ammo of wielded item without duplication Signed-off-by: Lance Finfrock <lancewf@gmail.com> Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com> Co-authored-by: RoyBerube <royberube1965@gmail.com>

Adding unloaded gun to avoid

11a5454

Signed-off-by: Lance Finfrock <lancewf@gmail.com> Added unit test Signed-off-by: Lance Finfrock <lancewf@gmail.com>

actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) Code: Tests Measurement, self-control, statistics, balancing. Items: Ammo / Guns Ammunition for all kinds of weapons and these weapons themselves labels May 22, 2021

pehamm reviewed May 22, 2021

View reviewed changes

Fixing lint failure

93e0ce4

Signed-off-by: Lance Finfrock <lancewf@gmail.com>

lancewf commented May 22, 2021

View reviewed changes

actual-nh reviewed May 22, 2021

View reviewed changes

tests/unload_naked_test.cpp Outdated Show resolved Hide resolved

tests/unload_naked_test.cpp Outdated Show resolved Hide resolved

tests/unload_naked_test.cpp Outdated Show resolved Hide resolved

lancewf and others added 3 commits May 22, 2021 16:27

Update tests/unload_naked_test.cpp

92d2e6c

Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>

Update tests/unload_naked_test.cpp

feb0f7b

Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>

Update tests/unload_naked_test.cpp

14aa305

Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>

Unload partial ammo of wielded item without duplication

60976a6

lancewf commented May 23, 2021

View reviewed changes

Fixing linting on tests

be19316

Signed-off-by: Lance Finfrock <lancewf@gmail.com>

kevingranade merged commit ce412c8 into CleverRaven:master May 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing error thrown when unloading gun when there is no inventory available #48999

Fixing error thrown when unloading gun when there is no inventory available #48999

lancewf commented May 22, 2021 •

edited

Loading

pehamm May 22, 2021

lancewf May 22, 2021

lancewf May 22, 2021

actual-nh left a comment

lancewf commented May 23, 2021

pehamm commented May 23, 2021 •

edited

Loading

RoyBerube commented May 23, 2021

actual-nh commented May 23, 2021

lancewf commented May 23, 2021

lancewf May 23, 2021

lancewf commented May 23, 2021

actual-nh commented May 28, 2021

lancewf commented May 28, 2021

	bool Character::add_or_drop_with_msg( item &it, const bool /unloading/, const item *avoid )
	{
	if( it.made_of( phase_id::LIQUID ) ) {
	liquid_handler::consume_liquid( it, 1, avoid );
	return it.charges <= 0;
	}
	if( !this->can_pickVolume( it ) ) {
	put_into_vehicle_or_drop( *this, item_drop_reason::too_large, { it } );
	} else if( !this->can_pickWeight( it, !get_option<bool>( "DANGEROUS_PICKUPS" ) ) ) {
	put_into_vehicle_or_drop( *this, item_drop_reason::too_heavy, { it } );
	} else {
	const bool allow_wield = !weapon.has_item( it ) && weapon.magazine_current() != ⁢
	const int prev_charges = it.charges;
	auto &ni = this->i_add( it, true, avoid, /allow_drop=/false, /allow_wield=/allow_wield );
	if( ni.is_null() ) {
	// failed to add
	put_into_vehicle_or_drop( *this, item_drop_reason::tumbling, { it } );
	} else if( &ni == &it ) {
	// merged into the original stack, restore original charges
	it.charges = prev_charges;
	put_into_vehicle_or_drop( *this, item_drop_reason::tumbling, { it } );
	} else {
	// successfully added
	add_msg( _( "You put the %s in your inventory." ), ni.tname() );
	add_msg( m_info, "%c - %s", ni.invlet == 0 ? ' ' : ni.invlet, ni.tname() );
	}
	}
	return true;
	}

	if( weapon.can_contain( it ) && ( avoid == nullptr \|\| &weapon != avoid )) {
	if( weapon.can_contain( it ) && ( avoid == nullptr \|\| &weapon != avoid ) ) {

Fixing error thrown when unloading gun when there is no inventory available #48999

Fixing error thrown when unloading gun when there is no inventory available #48999

Conversation

lancewf commented May 22, 2021 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

pehamm May 22, 2021

Choose a reason for hiding this comment

lancewf May 22, 2021

Choose a reason for hiding this comment

lancewf May 22, 2021

Choose a reason for hiding this comment

actual-nh left a comment

Choose a reason for hiding this comment

lancewf commented May 23, 2021

pehamm commented May 23, 2021 • edited Loading

RoyBerube commented May 23, 2021

actual-nh commented May 23, 2021

lancewf commented May 23, 2021

lancewf May 23, 2021

Choose a reason for hiding this comment

lancewf commented May 23, 2021

actual-nh commented May 28, 2021

lancewf commented May 28, 2021

lancewf commented May 22, 2021 •

edited

Loading

pehamm commented May 23, 2021 •

edited

Loading