From a4481bfb42ea012b56b85896927ab2c28e562587 Mon Sep 17 00:00:00 2001 From: Isaac Freund Date: Mon, 17 Jun 2019 16:40:02 +0200 Subject: [PATCH] Fix invlet tests Rewrite the item management of the the invlet test to use an item unique id hack to make it more robust and functional now that items on the map are unordered. Note: this test seems to have been giving false positives since #30603 was merged, since the AUTO_INV_ASSIGN option was not being properly set. This only became apparent after implementing the more robust item management code with unique ids. This also fixes a few remaing calls in the tests to a removed vehicle function which still compiled due to implicit conversion of 0 to a pointer. --- tests/invlet_test.cpp | 267 ++++++++++++++++++++++------------- tests/vehicle_drag.cpp | 2 +- tests/vehicle_efficiency.cpp | 2 +- tests/visitable_remove.cpp | 4 +- 4 files changed, 168 insertions(+), 107 deletions(-) diff --git a/tests/invlet_test.cpp b/tests/invlet_test.cpp index fb629936885fc..52ab096f6ea52 100644 --- a/tests/invlet_test.cpp +++ b/tests/invlet_test.cpp @@ -46,6 +46,35 @@ enum test_action { TEST_ACTION_NUM, }; +// This is a massive hack that makes this test work without totally rewriting it after #31406 +// The number of null items in item.components is used as a unique id for the purposes of this test +// +// The reason components is used instead of some other property of items is that this isn't checked +// when determining if two items stack and therefore has no side effects. +static void set_id( item &it, int id ) +{ + it.components = std::list( static_cast( id ), item() ); +} + +static int get_id( const item &it ) +{ + return static_cast( it.components.size() ); +} + +template +static item *retrieve_item( const T &sel, int id ) +{ + item *obj = nullptr; + sel.visit_items( [&id, &obj]( const item * e ) { + if( get_id( *e ) == id ) { + obj = const_cast( e ); + return VisitResponse::ABORT; + } + return VisitResponse::NEXT; + } ); + return obj; +} + static std::string location_desc( const inventory_location loc ) { switch( loc ) { @@ -206,46 +235,63 @@ static invlet_state check_invlet( player &p, item &it, const char invlet ) return UNEXPECTED; } -static void drop_at_feet( player &p, const int pos ) +static void drop_at_feet( player &p, const int id ) { - auto size_before = g->m.i_at( p.pos() ).size(); + size_t size_before = g->m.i_at( p.pos() ).size(); + + item *found = retrieve_item( p, id ); + REQUIRE( found ); + int pos = p.get_item_position( found ); p.moves = 100; p.drop( pos, p.pos() ); p.activity.do_turn( p ); + REQUIRE( g->m.i_at( p.pos() ).size() == size_before + 1 ); } -static void pick_up_from_feet( player &p, int pos ) +static void pick_up_from_feet( player &p, int id ) { map_stack items = g->m.i_at( p.pos() ); size_t size_before = items.size(); - REQUIRE( size_before > pos ); + + item *found = retrieve_item( map_cursor( p.pos() ), id ); + REQUIRE( found ); + p.moves = 100; p.assign_activity( activity_id( "ACT_PICKUP" ) ); - // This is a hack to avoid rewriting this test case after changes to how items are stored - // TODO: Refactor this to use item_locations instead of indexes (which are unstable) - p.activity.targets.emplace_back( map_cursor( p.pos() ), &*items.get_iterator_from_index( pos ) ); + p.activity.targets.emplace_back( map_cursor( p.pos() ), found ); p.activity.values.push_back( 0 ); p.activity.do_turn( p ); - REQUIRE( g->m.i_at( p.pos() ).size() == size_before - 1 ); + + REQUIRE( items.size() == size_before - 1 ); } -static void wear_from_feet( player &p, int pos ) +static void wear_from_feet( player &p, int id ) { - int size_before = static_cast( g->m.i_at( p.pos() ).size() ); - REQUIRE( size_before > pos ); - // This is a temporary hack to get this (currently broken) test to compile - p.wear_item( *g->m.i_at( p.pos() ).get_iterator_from_index( pos ), false ); - g->m.i_rem( p.pos(), &*g->m.i_at( p.pos() ).get_iterator_from_index( pos ) ); + map_stack items = g->m.i_at( p.pos() ); + size_t size_before = items.size(); + + item *found = retrieve_item( map_cursor( p.pos() ), id ); + REQUIRE( found ); + + p.wear_item( *found, false ); + g->m.i_rem( p.pos(), found ); + + REQUIRE( items.size() == size_before - 1 ); } -static void wield_from_feet( player &p, int pos ) +static void wield_from_feet( player &p, int id ) { - int size_before = static_cast( g->m.i_at( p.pos() ).size() ); - REQUIRE( size_before > pos ); - // This is a temporary hack to get this (currently broken) test to compile - p.wield( *g->m.i_at( p.pos() ).get_iterator_from_index( pos ) ); - g->m.i_rem( p.pos(), &*g->m.i_at( p.pos() ).get_iterator_from_index( pos ) ); + map_stack items = g->m.i_at( p.pos() ); + size_t size_before = items.size(); + + item *found = retrieve_item( map_cursor( p.pos() ), id ); + REQUIRE( found ); + + p.wield( *found ); + g->m.i_rem( p.pos(), found ); + + REQUIRE( items.size() == size_before - 1 ); } static void add_item( player &p, item &it, const inventory_location loc ) @@ -261,7 +307,7 @@ static void add_item( player &p, item &it, const inventory_location loc ) p.wear_item( it ); break; case WIELDED_OR_WORN: - if( p.weapon.is_null() ) { + if( !p.is_armed() ) { p.wield( it ); } else { // since we can only wield one item, wear the item instead @@ -274,18 +320,21 @@ static void add_item( player &p, item &it, const inventory_location loc ) } } -static item &item_at( player &p, const int pos, const inventory_location loc ) +static item &item_at( player &p, const int id, const inventory_location loc ) { switch( loc ) { - case GROUND: - // This is a temporary hack to get this (currently broken) test to compile - return *g->m.i_at( p.pos() ).get_iterator_from_index( pos ); + case GROUND: { + item *found = retrieve_item( map_cursor( p.pos() ), id ); + REQUIRE( found ); + return *found; + } case INVENTORY: - return p.i_at( pos ); case WORN: - return p.i_at( -2 - pos ); - case WIELDED_OR_WORN: - return p.i_at( -1 - pos ); + case WIELDED_OR_WORN: { + item *found = retrieve_item( p, id ); + REQUIRE( found ); + return *found; + } default: FAIL( "unimplemented" ); break; @@ -293,7 +342,7 @@ static item &item_at( player &p, const int pos, const inventory_location loc ) return null_item_reference(); } -static void move_item( player &p, const int pos, const inventory_location from, +static void move_item( player &p, const int id, const inventory_location from, const inventory_location to ) { switch( from ) { @@ -304,17 +353,17 @@ static void move_item( player &p, const int pos, const inventory_location from, FAIL( "unimplemented" ); break; case INVENTORY: - pick_up_from_feet( p, pos ); + pick_up_from_feet( p, id ); break; case WORN: - wear_from_feet( p, pos ); + wear_from_feet( p, id ); break; case WIELDED_OR_WORN: if( p.weapon.is_null() ) { - wield_from_feet( p, pos ); + wield_from_feet( p, id ); } else { // since we can only wield one item, wear the item instead - wear_from_feet( p, pos ); + wear_from_feet( p, id ); } break; } @@ -322,21 +371,21 @@ static void move_item( player &p, const int pos, const inventory_location from, case INVENTORY: switch( to ) { case GROUND: - drop_at_feet( p, pos ); + drop_at_feet( p, id ); break; case INVENTORY: default: FAIL( "unimplemented" ); break; case WORN: - p.wear( pos, false ); + p.wear( item_at( p, id, from ), false ); break; case WIELDED_OR_WORN: if( p.weapon.is_null() ) { - p.wield( p.i_at( pos ) ); + p.wield( item_at( p, id, from ) ); } else { // since we can only wield one item, wear the item instead - p.wear( pos, false ); + p.wear( item_at( p, id, from ), false ); } break; } @@ -344,10 +393,10 @@ static void move_item( player &p, const int pos, const inventory_location from, case WORN: switch( to ) { case GROUND: - drop_at_feet( p, -2 - pos ); + drop_at_feet( p, id ); break; case INVENTORY: - p.takeoff( -2 - pos ); + p.takeoff( item_at( p, id, from ) ); break; case WORN: case WIELDED_OR_WORN: @@ -359,21 +408,21 @@ static void move_item( player &p, const int pos, const inventory_location from, case WIELDED_OR_WORN: switch( to ) { case GROUND: - drop_at_feet( p, -1 - pos ); - if( pos == 0 && !p.worn.empty() ) { + drop_at_feet( p, id ); + if( !p.is_armed() && !p.worn.empty() ) { // wield the first worn item - p.wield( p.i_at( -2 ) ); + p.wield( p.worn.front() ); } break; case INVENTORY: - if( pos == 0 ) { - p.i_add( p.i_rem( -1 ) ); + if( p.is_wielding( item_at( p, id, from ) ) ) { + p.i_add( p.i_rem( &item_at( p, id, from ) ) ); } else { - p.takeoff( -1 - pos ); + p.takeoff( item_at( p, id, from ) ); } - if( pos == 0 && !p.worn.empty() ) { + if( !p.is_armed() && !p.worn.empty() ) { // wield the first worn item - p.wield( p.i_at( -2 ) ); + p.wield( p.worn.front() ); } break; case WORN: @@ -418,40 +467,43 @@ static void invlet_test( player &dummy, const inventory_location from, const inv item tshirt( "tshirt" ); item jeans( "jeans" ); + set_id( tshirt, 1 ); + set_id( jeans, 2 ); + // add the items to the starting position add_item( dummy, tshirt, to ); add_item( dummy, jeans, to ); // assign invlet to the first item - assign_invlet( dummy, item_at( dummy, 0, to ), invlet, first_invlet_state ); + assign_invlet( dummy, item_at( dummy, 1, to ), invlet, first_invlet_state ); // remove the first item - move_item( dummy, 0, to, from ); + move_item( dummy, 1, to, from ); // assign invlet to the second item - assign_invlet( dummy, item_at( dummy, 0, to ), invlet, second_invlet_state ); + assign_invlet( dummy, item_at( dummy, 2, to ), invlet, second_invlet_state ); item *final_first = nullptr; item *final_second = nullptr; switch( action ) { case REMOVE_1ST_REMOVE_2ND_ADD_1ST_ADD_2ND: - move_item( dummy, 0, to, from ); - move_item( dummy, 0, from, to ); - move_item( dummy, 0, from, to ); - final_first = &item_at( dummy, 0, to ); - final_second = &item_at( dummy, 1, to ); + move_item( dummy, 2, to, from ); + move_item( dummy, 1, from, to ); + move_item( dummy, 2, from, to ); + final_first = &item_at( dummy, 1, to ); + final_second = &item_at( dummy, 2, to ); break; case REMOVE_1ST_REMOVE_2ND_ADD_2ND_ADD_1ST: - move_item( dummy, 0, to, from ); + move_item( dummy, 2, to, from ); + move_item( dummy, 2, from, to ); move_item( dummy, 1, from, to ); - move_item( dummy, 0, from, to ); final_first = &item_at( dummy, 1, to ); - final_second = &item_at( dummy, 0, to ); + final_second = &item_at( dummy, 2, to ); break; case REMOVE_1ST_ADD_1ST: - move_item( dummy, 0, from, to ); + move_item( dummy, 1, from, to ); final_first = &item_at( dummy, 1, to ); - final_second = &item_at( dummy, 0, to ); + final_second = &item_at( dummy, 2, to ); break; default: FAIL( "unimplemented" ); @@ -493,17 +545,21 @@ static void stack_invlet_test( player &dummy, inventory_location from, inventory g->m.i_clear( dummy.pos() ); // some stackable item that can be wielded and worn - item tshirt( "tshirt" ); + item tshirt1( "tshirt" ); + item tshirt2( "tshirt" ); + + set_id( tshirt1, 1 ); + set_id( tshirt2, 2 ); // add two such items to the starting position - add_item( dummy, tshirt, from ); - add_item( dummy, tshirt, from ); + add_item( dummy, tshirt1, from ); + add_item( dummy, tshirt2, from ); // assign the stack with invlet - assign_invlet( dummy, item_at( dummy, 0, from ), invlet, CACHED ); + assign_invlet( dummy, item_at( dummy, 1, from ), invlet, CACHED ); - // wield or wear one of the items - move_item( dummy, 0, from, to ); + // wield or wear the first item + move_item( dummy, 1, from, to ); std::stringstream ss; ss << "1. add a stack of two same items to " << location_desc( from ) << std::endl; @@ -511,17 +567,17 @@ static void stack_invlet_test( player &dummy, inventory_location from, inventory ss << "3. " << move_action_desc( 0, from, to ) << std::endl; ss << "expect the two items to have different invlets" << std::endl; ss << "actually the two items have " << - ( item_at( dummy, 0, to ).invlet != item_at( dummy, 0, from ).invlet ? "different" : "the same" ) << + ( item_at( dummy, 1, to ).invlet != item_at( dummy, 2, from ).invlet ? "different" : "the same" ) << " invlets" << std::endl; INFO( ss.str() ); - REQUIRE( item_at( dummy, 0, from ).typeId() == tshirt.typeId() ); - REQUIRE( item_at( dummy, 0, to ).typeId() == tshirt.typeId() ); + REQUIRE( item_at( dummy, 1, to ).typeId() == tshirt1.typeId() ); + REQUIRE( item_at( dummy, 2, from ).typeId() == tshirt2.typeId() ); // the wielded/worn item should have different invlet from the remaining item - CHECK( item_at( dummy, 0, to ).invlet != item_at( dummy, 0, from ).invlet ); + CHECK( item_at( dummy, 1, to ).invlet != item_at( dummy, 2, from ).invlet ); // clear invlets - assign_invlet( dummy, item_at( dummy, 0, from ), invlet, NONE ); - assign_invlet( dummy, item_at( dummy, 0, to ), invlet, NONE ); + assign_invlet( dummy, item_at( dummy, 1, to ), invlet, NONE ); + assign_invlet( dummy, item_at( dummy, 2, from ), invlet, NONE ); } static void swap_invlet_test( player &dummy, inventory_location loc ) @@ -544,25 +600,28 @@ static void swap_invlet_test( player &dummy, inventory_location loc ) item tshirt2( "tshirt" ); tshirt2.mod_damage( -1 ); + set_id( tshirt1, 1 ); + set_id( tshirt2, 2 ); + // add the items add_item( dummy, tshirt1, loc ); add_item( dummy, tshirt2, loc ); // assign the items with invlets - assign_invlet( dummy, item_at( dummy, 0, loc ), invlet_1, CACHED ); - assign_invlet( dummy, item_at( dummy, 1, loc ), invlet_2, CACHED ); + assign_invlet( dummy, item_at( dummy, 1, loc ), invlet_1, CACHED ); + assign_invlet( dummy, item_at( dummy, 2, loc ), invlet_2, CACHED ); // swap the invlets (invoking twice to make the invlet non-player-assigned) - dummy.reassign_item( item_at( dummy, 0, loc ), invlet_2 ); - dummy.reassign_item( item_at( dummy, 0, loc ), invlet_2 ); + dummy.reassign_item( item_at( dummy, 1, loc ), invlet_2 ); + dummy.reassign_item( item_at( dummy, 1, loc ), invlet_2 ); // drop the items - move_item( dummy, 0, loc, GROUND ); - move_item( dummy, 0, loc, GROUND ); + move_item( dummy, 1, loc, GROUND ); + move_item( dummy, 2, loc, GROUND ); // get them again - move_item( dummy, 0, GROUND, loc ); - move_item( dummy, 0, GROUND, loc ); + move_item( dummy, 1, GROUND, loc ); + move_item( dummy, 2, GROUND, loc ); std::stringstream ss; ss << "1. add two items of the same type to " << location_desc( loc ) << @@ -573,23 +632,23 @@ static void swap_invlet_test( player &dummy, inventory_location loc ) ss << "4. move the items to " << location_desc( GROUND ) << std::endl; ss << "5. move the items to " << location_desc( loc ) << " again" << std::endl; ss << "expect the items to keep their swapped invlets" << std::endl; - if( item_at( dummy, 0, loc ).invlet == invlet_2 && item_at( dummy, 1, loc ).invlet == invlet_1 ) { + if( item_at( dummy, 1, loc ).invlet == invlet_2 && item_at( dummy, 2, loc ).invlet == invlet_1 ) { ss << "the items actually keep their swapped invlets" << std::endl; } else { ss << "the items actually does not keep their swapped invlets" << std::endl; } INFO( ss.str() ); - REQUIRE( item_at( dummy, 0, loc ).typeId() == tshirt1.typeId() ); - REQUIRE( item_at( dummy, 1, loc ).typeId() == tshirt2.typeId() ); + REQUIRE( item_at( dummy, 1, loc ).typeId() == tshirt1.typeId() ); + REQUIRE( item_at( dummy, 2, loc ).typeId() == tshirt2.typeId() ); // invlets should not disappear and should still be swapped - CHECK( item_at( dummy, 0, loc ).invlet == invlet_2 ); - CHECK( item_at( dummy, 1, loc ).invlet == invlet_1 ); - CHECK( check_invlet( dummy, item_at( dummy, 0, loc ), invlet_2 ) == CACHED ); - CHECK( check_invlet( dummy, item_at( dummy, 1, loc ), invlet_1 ) == CACHED ); + CHECK( item_at( dummy, 1, loc ).invlet == invlet_2 ); + CHECK( item_at( dummy, 2, loc ).invlet == invlet_1 ); + CHECK( check_invlet( dummy, item_at( dummy, 1, loc ), invlet_2 ) == CACHED ); + CHECK( check_invlet( dummy, item_at( dummy, 2, loc ), invlet_1 ) == CACHED ); // clear invlets - assign_invlet( dummy, item_at( dummy, 0, loc ), invlet_2, NONE ); - assign_invlet( dummy, item_at( dummy, 1, loc ), invlet_1, NONE ); + assign_invlet( dummy, item_at( dummy, 1, loc ), invlet_2, NONE ); + assign_invlet( dummy, item_at( dummy, 2, loc ), invlet_1, NONE ); } static void merge_invlet_test( player &dummy, inventory_location from ) @@ -621,20 +680,24 @@ static void merge_invlet_test( player &dummy, inventory_location from ) g->m.i_clear( dummy.pos() ); // some stackable item - item tshirt( "tshirt" ); + item tshirt1( "tshirt" ); + item tshirt2( "tshirt" ); + + set_id( tshirt1, 1 ); + set_id( tshirt2, 2 ); // add the item - add_item( dummy, tshirt, INVENTORY ); - add_item( dummy, tshirt, from ); + add_item( dummy, tshirt1, INVENTORY ); + add_item( dummy, tshirt2, from ); // assign the items with invlets - assign_invlet( dummy, item_at( dummy, 0, INVENTORY ), invlet_1, first_invlet_state ); - assign_invlet( dummy, item_at( dummy, 0, from ), invlet_2, second_invlet_state ); + assign_invlet( dummy, item_at( dummy, 1, INVENTORY ), invlet_1, first_invlet_state ); + assign_invlet( dummy, item_at( dummy, 2, from ), invlet_2, second_invlet_state ); // merge the second item into inventory - move_item( dummy, 0, from, INVENTORY ); + move_item( dummy, 2, from, INVENTORY ); - item &merged_item = item_at( dummy, 0, INVENTORY ); + item &merged_item = item_at( dummy, 1, INVENTORY ); invlet_state merged_invlet_state = check_invlet( dummy, merged_item, expected_merged_invlet ); char merged_invlet = merged_item.invlet; @@ -650,7 +713,7 @@ static void merge_invlet_test( player &dummy, inventory_location from ) ss << "the stack actually has " << invlet_state_desc( merged_invlet_state ) << " invlet " << merged_invlet << std::endl; INFO( ss.str() ); - REQUIRE( merged_item.typeId() == tshirt.typeId() ); + REQUIRE( merged_item.typeId() == tshirt1.typeId() ); CHECK( merged_invlet_state == expected_merged_invlet_state ); CHECK( merged_invlet == expected_merged_invlet ); } @@ -658,25 +721,25 @@ static void merge_invlet_test( player &dummy, inventory_location from ) #define invlet_test_autoletter_off( name, dummy, from, to ) \ SECTION( std::string( name ) + " (auto letter off)" ) { \ - get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "false" ); \ + get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "disabled" ); \ invlet_test( dummy, from, to ); \ } #define stack_invlet_test_autoletter_off( name, dummy, from, to ) \ SECTION( std::string( name ) + " (auto letter off)" ) { \ - get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "false" ); \ + get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "disabled" ); \ stack_invlet_test( dummy, from, to ); \ } #define swap_invlet_test_autoletter_off( name, dummy, loc ) \ SECTION( std::string( name ) + " (auto letter off)" ) { \ - get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "false" ); \ + get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "disabled" ); \ swap_invlet_test( dummy, loc ); \ } #define merge_invlet_test_autoletter_off( name, dummy, from ) \ SECTION( std::string( name ) + " (auto letter off)" ) { \ - get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "false" ); \ + get_options().get_option( "AUTO_INV_ASSIGN" ).setValue( "disabled" ); \ merge_invlet_test( dummy, from ); \ } diff --git a/tests/vehicle_drag.cpp b/tests/vehicle_drag.cpp index b806dd22d837b..b7b1e6e1461cb 100644 --- a/tests/vehicle_drag.cpp +++ b/tests/vehicle_drag.cpp @@ -77,7 +77,7 @@ static vehicle *setup_drag_test( const vproto_id &veh_id ) // Remove all items from cargo to normalize weight. // turn everything on for( const vpart_reference vp : veh_ptr->get_all_parts() ) { - while( veh_ptr->remove_item( vp.part_index(), 0 ) ); + veh_ptr->get_items( vp.part_index() ).clear(); veh_ptr->toggle_specific_part( vp.part_index(), true ); } // close the doors diff --git a/tests/vehicle_efficiency.cpp b/tests/vehicle_efficiency.cpp index 5ef56be3430c7..c89d792d80f72 100644 --- a/tests/vehicle_efficiency.cpp +++ b/tests/vehicle_efficiency.cpp @@ -198,7 +198,7 @@ static long test_efficiency( const vproto_id &veh_id, int &expected_mass, // Remove all items from cargo to normalize weight. for( const vpart_reference vp : veh.get_all_parts() ) { - while( veh.remove_item( vp.part_index(), 0 ) ); + veh_ptr->get_items( vp.part_index() ).clear(); vp.part().ammo_consume( vp.part().ammo_remaining(), vp.pos() ); } for( const vpart_reference vp : veh.get_avail_parts( "OPENABLE" ) ) { diff --git a/tests/visitable_remove.cpp b/tests/visitable_remove.cpp index a32ad5e6f6eaf..60af686b56e5e 100644 --- a/tests/visitable_remove.cpp +++ b/tests/visitable_remove.cpp @@ -423,9 +423,7 @@ TEST_CASE( "visitable_remove", "[visitable]" ) const int part = vp->part_index(); REQUIRE( part >= 0 ); // Empty the vehicle of any cargo. - while( !v->get_items( part ).empty() ) { - v->remove_item( part, 0 ); - } + v->get_items( part ).clear(); for( int i = 0; i != count; ++i ) { v->add_item( part, obj ); }