From ddd9370eb839f839edc8c40b06c3689a8a04948d Mon Sep 17 00:00:00 2001 From: Eric Pierce Date: Fri, 21 Feb 2020 15:06:28 -0700 Subject: [PATCH 1/3] Make electric motors go in reverse better Due to the downscaling of reverse power/speed for vehicle engines, it can happen that a vehicle is so heavy, it can move forward but not backward. Because battery-powered electric motors can turn in either direction with equal speed and torque, vehicles with electric motors should be exempt from the power-reduction for reverse speeds. This commit adds a new `vehicle::max_reverse_velocity` function as the counterpart to `vehicle::max_velocity`, where an exception is made for battery-powered motors. Here I suppose is where the maximum reverse speed multiplier for horses, bicycles, and jet turbines would eventually be made; for now it just checks for battery-powered motors. Note, how *much* the motor is contributing to overall torque is not calculated; the simple presence of any electric motor in the drivetrain (no matter how small) ought to negate the reverse-power penalty. Fixed #38165 --- src/vehicle.cpp | 12 ++++++++++++ src/vehicle.h | 2 ++ src/vehicle_move.cpp | 8 ++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/vehicle.cpp b/src/vehicle.cpp index 60265f60707bb..b979ed62a3fec 100644 --- a/src/vehicle.cpp +++ b/src/vehicle.cpp @@ -3691,6 +3691,18 @@ int vehicle::max_velocity( const bool fueled ) const return is_watercraft() ? max_water_velocity( fueled ) : max_ground_velocity( fueled ); } +int vehicle::max_reverse_velocity( const bool fueled ) const +{ + int max_vel = max_velocity( fueled ); + if( has_engine_type( fuel_type_battery, true ) ) { + // Electric motors can go in reverse as well as forward + return -max_vel; + } else { + // All other motive powers do poorly in reverse + return -max_vel / 4; + } +} + // the same physics as max_ground_velocity, but with a smaller engine power int vehicle::safe_ground_velocity( const bool fueled ) const { diff --git a/src/vehicle.h b/src/vehicle.h index 1120f924b5109..3b7a665f14f02 100644 --- a/src/vehicle.h +++ b/src/vehicle.h @@ -1191,6 +1191,8 @@ class vehicle int max_water_velocity( bool fueled = true ) const; // Get maximum velocity for the current movement mode int max_velocity( bool fueled = true ) const; + // Get maximum reverse velocity for the current movement mode + int max_reverse_velocity( bool fueled = true ) const; // Get safe ground velocity gained by combined power of all engines. // If fueled == true, then only the engines which the vehicle has fuel for are included diff --git a/src/vehicle_move.cpp b/src/vehicle_move.cpp index ef111cb60b704..e3cc3404576e6 100644 --- a/src/vehicle_move.cpp +++ b/src/vehicle_move.cpp @@ -147,8 +147,8 @@ void vehicle::thrust( int thd ) //pos or neg if accelerator or brake int vel_inc = ( ( thrusting ) ? accel : brk ) * thd; - if( thd == -1 && thrusting ) { - //accelerate 60% if going backward + // Reverse is only 60% acceleration, unless an electric motor is in use + if( thd == -1 && thrusting && !has_engine_type( fuel_type_battery, true ) ) { vel_inc = .6 * vel_inc; } @@ -213,7 +213,7 @@ void vehicle::thrust( int thd ) stop(); } else { // Increase velocity up to max_vel or min_vel, but not above. - const int min_vel = -max_vel / 4; + const int min_vel = max_reverse_velocity(); if( vel_inc > 0 ) { // Don't allow braking by accelerating (could happen with damaged engines) velocity = std::max( velocity, std::min( velocity + vel_inc, max_vel ) ); @@ -245,7 +245,7 @@ void vehicle::cruise_thrust( int amount ) } int safe_vel = safe_velocity(); int max_vel = max_velocity(); - int max_rev_vel = -max_vel / 4; + int max_rev_vel = max_reverse_velocity(); //if the safe velocity is between the cruise velocity and its next value, set to safe velocity if( ( cruise_velocity < safe_vel && safe_vel < ( cruise_velocity + amount ) ) || From 96bceaa1c385fe96e7ee1c4d7b6efe3dbbdd3be4 Mon Sep 17 00:00:00 2001 From: Eric Pierce Date: Fri, 21 Feb 2020 20:35:14 -0700 Subject: [PATCH 2/3] Refactor vehicle_power_test.cpp for readability This is mainly preliminary work for adding new test cases to cover the changes I have made to maximum reverse speed; this vehicle power test seemed like a suitable place to put them. I had trouble understanding what this test was doing; refactoring it into BDD-style expressions helped me grok it while making it a whole lot easier for others to read in the future. --- tests/vehicle_power_test.cpp | 169 +++++++++++++++++++++++------------ 1 file changed, 110 insertions(+), 59 deletions(-) diff --git a/tests/vehicle_power_test.cpp b/tests/vehicle_power_test.cpp index a78330e064f82..c8207909f9c43 100644 --- a/tests/vehicle_power_test.cpp +++ b/tests/vehicle_power_test.cpp @@ -17,74 +17,125 @@ static const itype_id fuel_type_battery( "battery" ); static const itype_id fuel_type_plut_cell( "plut_cell" ); -TEST_CASE( "vehicle_power" ) +static void remove_all_vehicles() { - GIVEN( "Reactor and solar panels" ) { - for( const tripoint &p : g->m.points_in_rectangle( tripoint_zero, - tripoint( MAPSIZE * SEEX, MAPSIZE * SEEY, 0 ) ) ) { - g->m.furn_set( p, furn_id( "f_null" ) ); - g->m.ter_set( p, ter_id( "t_pavement" ) ); - g->m.trap_set( p, trap_id( "tr_null" ) ); - g->m.i_clear( p ); - } + VehicleList vehs = g->m.get_vehicles(); + vehicle *veh_ptr; + for( auto &vehs_v : vehs ) { + veh_ptr = vehs_v.v; + g->m.destroy_vehicle( veh_ptr ); + } +} - g->m.invalidate_map_cache( 0 ); - g->m.build_map_cache( 0, true ); - - CHECK( !g->u.in_vehicle ); - const tripoint test_origin( 15, 15, 0 ); - g->u.setpos( test_origin ); - const tripoint vehicle_origin = tripoint( 10, 10, 0 ); - VehicleList vehs = g->m.get_vehicles(); - vehicle *veh_ptr; - for( auto &vehs_v : vehs ) { - veh_ptr = vehs_v.v; - g->m.destroy_vehicle( veh_ptr ); - } - g->refresh_all(); - REQUIRE( g->m.get_vehicles().empty() ); - veh_ptr = g->m.add_vehicle( vproto_id( "reactor_test" ), vehicle_origin, 0, 0, 0 ); +TEST_CASE( "vehicle power with reactor and solar panels", "[vehicle][power]" ) +{ + // Set up a sandbox world + for( const tripoint &p : g->m.points_in_rectangle( tripoint_zero, + tripoint( MAPSIZE * SEEX, MAPSIZE * SEEY, 0 ) ) ) { + g->m.furn_set( p, furn_id( "f_null" ) ); + g->m.ter_set( p, ter_id( "t_pavement" ) ); + g->m.trap_set( p, trap_id( "tr_null" ) ); + g->m.i_clear( p ); + } + + g->m.invalidate_map_cache( 0 ); + g->m.build_map_cache( 0, true ); + + REQUIRE( !g->u.in_vehicle ); + + const tripoint player_origin( 15, 15, 0 ); + vehicle *veh_ptr; + + g->u.setpos( player_origin ); + g->refresh_all(); + + remove_all_vehicles(); + REQUIRE( g->m.get_vehicles().empty() ); + + SECTION( "vehicle with reactor" ) { + const tripoint reactor_origin = tripoint( 10, 10, 0 ); + veh_ptr = g->m.add_vehicle( vproto_id( "reactor_test" ), reactor_origin, 0, 0, 0 ); REQUIRE( veh_ptr != nullptr ); g->refresh_all(); + REQUIRE( !veh_ptr->reactors.empty() ); vehicle_part &reactor = veh_ptr->parts[ veh_ptr->reactors.front() ]; - reactor.ammo_unset(); - veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); - REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); - reactor.ammo_set( fuel_type_plut_cell, 1 ); - REQUIRE( reactor.ammo_remaining() == 1 ); - veh_ptr->power_parts(); - CHECK( reactor.ammo_remaining() == 0 ); - CHECK( veh_ptr->fuel_left( fuel_type_battery ) == 100 ); - g->m.destroy_vehicle( veh_ptr ); - g->refresh_all(); - REQUIRE( g->m.get_vehicles().empty() ); + + GIVEN( "the reactor is empty" ) { + reactor.ammo_unset(); + veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); + REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); + + WHEN( "the reactor is loaded with plutonium fuel" ) { + reactor.ammo_set( fuel_type_plut_cell, 1 ); + REQUIRE( reactor.ammo_remaining() == 1 ); + + AND_WHEN( "the reactor is used to power and charge the battery" ) { + veh_ptr->power_parts(); + THEN( "the reactor should be empty, and the battery should be charged" ) { + CHECK( reactor.ammo_remaining() == 0 ); + CHECK( veh_ptr->fuel_left( fuel_type_battery ) == 100 ); + } + } + } + } + } + + SECTION( "vehicle with solar panels" ) { const tripoint solar_origin = tripoint( 5, 5, 0 ); veh_ptr = g->m.add_vehicle( vproto_id( "solar_panel_test" ), solar_origin, 0, 0, 0 ); REQUIRE( veh_ptr != nullptr ); g->refresh_all(); - calendar::turn = calendar::turn_zero + calendar::season_length() + 1_days; - const time_point start_time = sunrise( calendar::turn ) + 3_hours; - veh_ptr->update_time( start_time ); - veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); - REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); - g->weather.weather_override = WEATHER_SUNNY; - veh_ptr->update_time( start_time + 30_minutes ); - int approx_battery1 = veh_ptr->fuel_left( fuel_type_battery ) / 100; - const int exp_min = 10; - const int exp_max = 15; - CHECK( approx_battery1 >= exp_min ); - CHECK( approx_battery1 <= exp_max ); - veh_ptr->update_time( start_time + 2 * 30_minutes ); - int approx_battery2 = veh_ptr->fuel_left( fuel_type_battery ) / 100; - CHECK( approx_battery2 >= approx_battery1 + exp_min ); - CHECK( approx_battery2 <= approx_battery1 + exp_max ); - const time_point at_night = sunset( calendar::turn ) + 3_hours; - g->weather.weather_override = WEATHER_CLEAR; - veh_ptr->update_time( at_night ); - veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); - REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); - veh_ptr->update_time( at_night + 30_minutes ); - CHECK( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); + + GIVEN( "it is 3 hours after sunrise, with sunny weather" ) { + calendar::turn = calendar::turn_zero + calendar::season_length() + 1_days; + const time_point start_time = sunrise( calendar::turn ) + 3_hours; + veh_ptr->update_time( start_time ); + g->weather.weather_override = WEATHER_SUNNY; + + AND_GIVEN( "the battery has no charge" ) { + veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); + REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); + + WHEN( "30 minutes elapse" ) { + veh_ptr->update_time( start_time + 30_minutes ); + + THEN( "the battery should be partially charged" ) { + int charge = veh_ptr->fuel_left( fuel_type_battery ) / 100; + CHECK( 10 <= charge ); + CHECK( charge <= 15 ); + + AND_WHEN( "another 30 minutes elapse" ) { + veh_ptr->update_time( start_time + 2 * 30_minutes ); + + THEN( "the battery should be further charged" ) { + charge = veh_ptr->fuel_left( fuel_type_battery ) / 100; + CHECK( 20 <= charge ); + CHECK( charge <= 30 ); + } + } + } + } + } + } + + GIVEN( "it is 3 hours after sunset, with clear weather" ) { + const time_point at_night = sunset( calendar::turn ) + 3_hours; + g->weather.weather_override = WEATHER_CLEAR; + veh_ptr->update_time( at_night ); + + AND_GIVEN( "the battery has no charge" ) { + veh_ptr->discharge_battery( veh_ptr->fuel_left( fuel_type_battery ) ); + REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); + + WHEN( "60 minutes elapse" ) { + veh_ptr->update_time( at_night + 2 * 30_minutes ); + + THEN( "the battery should still have no charge" ) { + CHECK( veh_ptr->fuel_left( fuel_type_battery ) == 0 ); + } + } + } + } } } From 0a03d769dff3fcab81fa1d8bb0c8754d17992851 Mon Sep 17 00:00:00 2001 From: Eric Pierce Date: Sat, 22 Feb 2020 08:29:53 -0700 Subject: [PATCH 3/3] Add vehicle reverse velocity tests To minimally test the new `max_reverse_velocity` function, this commit adds two test vehicles (scooter and electric scooter), and checks that: - Combustion engine reverse velocity is 1/4 of forward - Electric engine reverse velocity is equivalent to forward Some additional refactoring of the helper functions is also done. --- data/json/vehicles/test.json | 27 +++++++++ tests/vehicle_power_test.cpp | 105 ++++++++++++++++++++++++++++------- 2 files changed, 113 insertions(+), 19 deletions(-) diff --git a/data/json/vehicles/test.json b/data/json/vehicles/test.json index b5a6640832808..d57d0f25a2e6f 100644 --- a/data/json/vehicles/test.json +++ b/data/json/vehicles/test.json @@ -182,5 +182,32 @@ { "x": 1, "y": 3, "parts": [ "frame_vertical", "solar_panel" ] }, { "x": 0, "y": -1, "parts": [ "frame_vertical", "seat", "battery_car" ] } ] + }, + { + "id": "scooter_test", + "type": "vehicle", + "name": "TEST Scooter", + "blueprint": [ "o>o" ], + "parts": [ + { "x": 0, "y": 0, "parts": [ "frame_handle", "headlight", "saddle", "controls", "controls_electronic" ] }, + { "x": 0, "y": 0, "parts": [ "horn_car", "motorcycle_kickstand", "engine_1cyl", "alternator_motorbike" ] }, + { "x": 0, "y": 0, "parts": [ "battery_motorbike", { "part": "tank_small", "fuel": "gasoline" } ] }, + { "x": 1, "y": 0, "parts": [ "xlframe_vertical", "wheel_mount_light_steerable", "wheel_small" ] }, + { "x": -1, "y": 0, "parts": [ "xlframe_vertical", "wheel_mount_light", "wheel_small" ] }, + { "x": -1, "y": 0, "part": "muffler" } + ] + }, + { + "id": "scooter_electric_test", + "type": "vehicle", + "name": "TEST Electric Scooter", + "blueprint": [ "o>o" ], + "parts": [ + { "x": 0, "y": 0, "parts": [ "frame_handle", "headlight", "saddle", "controls" ] }, + { "x": 0, "y": 0, "parts": [ "controls_electronic", "horn_car", "motorcycle_kickstand" ] }, + { "x": 0, "y": 0, "parts": [ "medium_storage_battery", "engine_electric_small" ] }, + { "x": 1, "y": 0, "parts": [ "xlframe_vertical", "wheel_mount_light_steerable", "wheel_small" ] }, + { "x": -1, "y": 0, "parts": [ "xlframe_vertical", "wheel_mount_light", "wheel_small" ] } + ] } ] diff --git a/tests/vehicle_power_test.cpp b/tests/vehicle_power_test.cpp index c8207909f9c43..3a1cb865c2a30 100644 --- a/tests/vehicle_power_test.cpp +++ b/tests/vehicle_power_test.cpp @@ -16,45 +16,53 @@ static const itype_id fuel_type_battery( "battery" ); static const itype_id fuel_type_plut_cell( "plut_cell" ); +static const efftype_id effect_blind( "blind" ); -static void remove_all_vehicles() +static void reset_player() { - VehicleList vehs = g->m.get_vehicles(); - vehicle *veh_ptr; - for( auto &vehs_v : vehs ) { - veh_ptr = vehs_v.v; - g->m.destroy_vehicle( veh_ptr ); - } + // Move player somewhere safe + REQUIRE( !g->u.in_vehicle ); + g->u.setpos( tripoint_zero ); + // Blind the player to avoid needless drawing-related overhead + g->u.add_effect( effect_blind, 1_turns, num_bp, true ); } -TEST_CASE( "vehicle power with reactor and solar panels", "[vehicle][power]" ) +// Build a map of size MAPSIZE_X x MAPSIZE_Y around tripoint_zero with a given +// terrain, and no furniture, traps, or items. +static void build_test_map( const ter_id &terrain ) { - // Set up a sandbox world for( const tripoint &p : g->m.points_in_rectangle( tripoint_zero, tripoint( MAPSIZE * SEEX, MAPSIZE * SEEY, 0 ) ) ) { g->m.furn_set( p, furn_id( "f_null" ) ); - g->m.ter_set( p, ter_id( "t_pavement" ) ); + g->m.ter_set( p, terrain ); g->m.trap_set( p, trap_id( "tr_null" ) ); g->m.i_clear( p ); } g->m.invalidate_map_cache( 0 ); g->m.build_map_cache( 0, true ); +} - REQUIRE( !g->u.in_vehicle ); - - const tripoint player_origin( 15, 15, 0 ); +static void remove_all_vehicles() +{ + VehicleList vehs = g->m.get_vehicles(); vehicle *veh_ptr; + for( auto &vehs_v : vehs ) { + veh_ptr = vehs_v.v; + g->m.destroy_vehicle( veh_ptr ); + } + REQUIRE( g->m.get_vehicles().empty() ); +} - g->u.setpos( player_origin ); - g->refresh_all(); - +TEST_CASE( "vehicle power with reactor and solar panels", "[vehicle][power]" ) +{ + reset_player(); + build_test_map( ter_id( "t_pavement" ) ); remove_all_vehicles(); - REQUIRE( g->m.get_vehicles().empty() ); SECTION( "vehicle with reactor" ) { const tripoint reactor_origin = tripoint( 10, 10, 0 ); - veh_ptr = g->m.add_vehicle( vproto_id( "reactor_test" ), reactor_origin, 0, 0, 0 ); + vehicle *veh_ptr = g->m.add_vehicle( vproto_id( "reactor_test" ), reactor_origin, 0, 0, 0 ); REQUIRE( veh_ptr != nullptr ); g->refresh_all(); @@ -83,7 +91,7 @@ TEST_CASE( "vehicle power with reactor and solar panels", "[vehicle][power]" ) SECTION( "vehicle with solar panels" ) { const tripoint solar_origin = tripoint( 5, 5, 0 ); - veh_ptr = g->m.add_vehicle( vproto_id( "solar_panel_test" ), solar_origin, 0, 0, 0 ); + vehicle *veh_ptr = g->m.add_vehicle( vproto_id( "solar_panel_test" ), solar_origin, 0, 0, 0 ); REQUIRE( veh_ptr != nullptr ); g->refresh_all(); @@ -139,3 +147,62 @@ TEST_CASE( "vehicle power with reactor and solar panels", "[vehicle][power]" ) } } } + +TEST_CASE( "maximum reverse velocity", "[vehicle][power][reverse]" ) +{ + reset_player(); + build_test_map( ter_id( "t_pavement" ) ); + remove_all_vehicles(); + + GIVEN( "a scooter with combustion engine and charged battery" ) { + const tripoint origin = tripoint( 10, 0, 0 ); + vehicle *veh_ptr = g->m.add_vehicle( vproto_id( "scooter_test" ), origin, 0, 0, 0 ); + REQUIRE( veh_ptr != nullptr ); + g->refresh_all(); + veh_ptr->charge_battery( 500 ); + REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 500 ); + + WHEN( "the engine is started" ) { + veh_ptr->start_engines(); + + THEN( "it can go in both forward and reverse" ) { + int max_fwd = veh_ptr->max_velocity( false ); + int max_rev = veh_ptr->max_reverse_velocity( false ); + + CHECK( max_rev < 0 ); + CHECK( max_fwd > 0 ); + + AND_THEN( "its maximum reverse velocity is 1/4 of the maximum forward velocity" ) { + CHECK( std::abs( max_fwd / max_rev ) == 4 ); + } + } + + } + } + + GIVEN( "a scooter with an electric motor and charged battery" ) { + const tripoint origin = tripoint( 15, 0, 0 ); + vehicle *veh_ptr = g->m.add_vehicle( vproto_id( "scooter_electric_test" ), origin, 0, 0, 0 ); + REQUIRE( veh_ptr != nullptr ); + g->refresh_all(); + veh_ptr->charge_battery( 5000 ); + REQUIRE( veh_ptr->fuel_left( fuel_type_battery ) == 5000 ); + + WHEN( "the engine is started" ) { + veh_ptr->start_engines(); + + THEN( "it can go in both forward and reverse" ) { + int max_fwd = veh_ptr->max_velocity( false ); + int max_rev = veh_ptr->max_reverse_velocity( false ); + + CHECK( max_rev < 0 ); + CHECK( max_fwd > 0 ); + + AND_THEN( "its maximum reverse velocity is equal to maximum forward velocity" ) { + CHECK( std::abs( max_rev ) == std::abs( max_fwd ) ); + } + } + } + } +} +