From 8d144d26b16d2fed2309a63e111dde2eee36f91f Mon Sep 17 00:00:00 2001 From: AngelicosPhosphoros Date: Fri, 10 Jun 2022 23:27:37 +0300 Subject: [PATCH] Refactor power grid traverse code (#1599) I was randomly reading code and found this function confusing. Refactored for readability. * Split graph traversion and whatever caller wants to do with items. * Added comments in places where I failed to understand fast. * Removed `amount` updating from traversing and few hacks on call-sites caused by this. * Removed redundant "only vehicles" traversion method. * Removed raw pointers from public interface. --- src/distribution_grid.h | 33 +++++++- src/vehicle.cpp | 169 +++++++++++++++++++++------------------- src/vehicle.h | 16 ---- 3 files changed, 120 insertions(+), 98 deletions(-) diff --git a/src/distribution_grid.h b/src/distribution_grid.h index 10dfb3297037..a69f05358e91 100644 --- a/src/distribution_grid.h +++ b/src/distribution_grid.h @@ -179,15 +179,40 @@ class distribution_grid_tracker void on_options_changed(); }; +class vehicle; + namespace distribution_graph { +enum class traverse_visitor_result { + stop, + continue_further, +}; -/** -* Traverses the graph of connected vehicles and grids. +/** Traverses the graph of connected vehicles and grids. +* +* Runs Breadth-First over all vehicles and grids calling passed actions on each of them +* until any visitor action return traverse_visitor_result::stop. +* Connected vehicles checked by all POWER_TRANSFER part and grids by vehicle connectors. +* +* @param start Reference to the start node of the graph. Assumed to be already visited. +* @param veh_action Visitor function which accepts vehicle& or const & then returns traverse_visitor_result. +* @param grid_action Visitor function which accepts distribution_grid& or const & then returns traverse_visitor_result. */ template -int traverse( StartPoint *start, int amount, - VehFunc veh_action, GridFunc grid_action ); +void traverse( StartPoint &start, + VehFunc veh_action, GridFunc grid_action ); + +/* Useful if we want to act only in one type. */ +inline constexpr traverse_visitor_result noop_visitor_grid( const distribution_grid & ) +{ + return traverse_visitor_result::continue_further; +} + +/* Useful if we want to act only in one type. */ +inline constexpr traverse_visitor_result noop_visitor_veh( const vehicle & ) +{ + return traverse_visitor_result::continue_further; +} } // namespace distribution_graph diff --git a/src/vehicle.cpp b/src/vehicle.cpp index d039f13d5a27..401f272acae4 100644 --- a/src/vehicle.cpp +++ b/src/vehicle.cpp @@ -3183,18 +3183,17 @@ int vehicle::fuel_left( const itype_id &ftype, bool recurse ) const } ); if( recurse && ftype == fuel_type_battery ) { - auto fuel_counting_visitor = [&]( vehicle const * veh, int amount ) { - return amount + veh->fuel_left( ftype, false ); + using tvr = distribution_graph::traverse_visitor_result; + auto fuel_counting_visitor = [&fl, &ftype]( vehicle const & veh ) { + fl += veh.fuel_left( ftype, false ); + return tvr::continue_further; }; - auto power_counting_visitor = [&]( distribution_grid const * grid, int amount ) { - return amount + grid->get_resource( false ); + auto power_counting_visitor = [&fl]( distribution_grid const & grid ) { + fl += grid.get_resource( false ); + return tvr::continue_further; }; - // HAX: add 1 to the initial amount so traversal doesn't immediately stop just - // 'cause we have 0 fuel left in the current vehicle. Subtract the 1 immediately - // after traversal. - fl = distribution_graph::traverse( this, fl + 1, - fuel_counting_visitor, power_counting_visitor ) - 1; + distribution_graph::traverse( *this, fuel_counting_visitor, power_counting_visitor ); } //muscle engines have infinite fuel @@ -4759,27 +4758,18 @@ vehicle *vehicle::find_vehicle( const tripoint &where ) void vehicle::enumerate_vehicles( std::map &connected_vehicles, const std::set &vehicle_list ) { - auto enumerate_visitor = [&connected_vehicles]( vehicle * veh, int amount ) { + auto enumerate_visitor = [&connected_vehicles]( vehicle & veh ) { // Only emplaces if element is not present already. - connected_vehicles.emplace( veh, false ); - return amount; + connected_vehicles.emplace( &veh, false ); + return distribution_graph::traverse_visitor_result::continue_further; }; for( vehicle *veh : vehicle_list ) { // This autovivifies, and also overwrites the value if already present. connected_vehicles[veh] = true; - traverse_vehicle_graph( veh, 1, enumerate_visitor ); + distribution_graph::traverse( *veh, enumerate_visitor, distribution_graph::noop_visitor_grid ); } } -template -int vehicle::traverse_vehicle_graph( Vehicle *start_veh, int amount, Func action ) -{ - const auto do_nothing = []( const distribution_grid *, int amt ) { - return amt; - }; - return distribution_graph::traverse( start_veh, amount, action, do_nothing ); -} - // TODO: It looks out of place in vehicle.cpp namespace distribution_graph { @@ -4820,66 +4810,87 @@ struct vehicle_or_grid { }; template -int traverse( StartPoint *start, int amount, - VehFunc veh_action, GridFunc grid_action ) +void traverse( StartPoint &start, + VehFunc veh_action, GridFunc grid_action ) { + using tvr = traverse_visitor_result; constexpr bool IsConst = std::is_const::value; struct hash { const std::hash char_hash = std::hash(); const std::hash ptr_hash = std::hash(); auto operator()( const vehicle_or_grid &elem ) const { return char_hash( static_cast( elem.type ) ) ^ - ( ptr_hash( reinterpret_cast( elem.veh ) | reinterpret_cast( elem.grid ) ) ); + ptr_hash( + // Because only one of pointers is not nullptr, binary OR would get value of set pointer. + reinterpret_cast( elem.veh ) | reinterpret_cast( elem.grid ) + ); } }; + // Actually, they are visited elements with unvisited neighbours. + // Not all connected elements are here. std::queue> connected_elements; + // For fast checking if we should visit some neighbour. std::unordered_set, hash> visited_elements; - connected_elements.emplace( start ); - visited_elements.insert( start ); + connected_elements.emplace( &start ); + visited_elements.insert( &start ); auto &grid_tracker = get_distribution_grid_tracker(); + auto enqueue = [&connected_elements, &visited_elements]( vehicle_or_grid newly_visited ) { + connected_elements.push( newly_visited ); + visited_elements.insert( newly_visited ); + }; + auto was_already_visited = [&visited_elements]( vehicle_or_grid to_visit ) { + return visited_elements.count( to_visit ) != 0; + }; + auto process_vehicle = [&]( const tripoint_abs_ms & target_pos ) { auto *veh = vehicle::find_vehicle( target_pos ); if( veh == nullptr ) { debugmsg( "lost vehicle at %s", target_pos.to_string() ); + return tvr::continue_further; } - // Add this connected vehicle to the queue of vehicles to search next, - // but only if we haven't seen this one before. - if( veh != nullptr && visited_elements.count( veh ) == 0 ) { - connected_elements.emplace( veh ); - visited_elements.insert( veh ); - amount = veh_action( veh, amount ); - g->u.add_msg_if_player( m_debug, "After remote veh %p, %d power", - static_cast( veh ), amount ); + if( was_already_visited( veh ) ) { + return tvr::continue_further; + } + + const tvr result = veh_action( *veh ); + g->u.add_msg_if_player( m_debug, "After remote veh %p", + static_cast( veh ) ); - return amount < 1; + // We do not need to check neighbours if we stop. + if( result == tvr::continue_further ) { + enqueue( veh ); } - return false; + return result; }; auto process_grid = [&]( const tripoint_abs_ms & target_pos ) { - auto *grid = &grid_tracker.grid_at( target_pos ); - if( !*grid ) { + auto &grid = grid_tracker.grid_at( target_pos ); + if( !grid ) { debugmsg( "lost grid at %s", target_pos.to_string() ); + return tvr::continue_further; } - if( *grid && visited_elements.count( grid ) == 0 ) { - connected_elements.emplace( grid ); - visited_elements.insert( grid ); - amount = grid_action( grid, amount ); - g->u.add_msg_if_player( m_debug, "After remote grid %p, %d power", - static_cast( grid ), amount ); + if( was_already_visited( &grid ) ) { + return tvr::continue_further; + } - return amount < 1; + const tvr result = grid_action( grid ); + g->u.add_msg_if_player( m_debug, "After remote grid %p", + static_cast( &grid ) ); + + // We do not need to check neighbours if we stop. + if( result == tvr::continue_further ) { + enqueue( &grid ); } - return false; + return result; }; - while( amount > 0 && !connected_elements.empty() ) { + while( !connected_elements.empty() ) { auto current = connected_elements.front(); connected_elements.pop(); @@ -4893,12 +4904,12 @@ int traverse( StartPoint *start, int amount, const tripoint_abs_ms target_pos( current_veh.cpart( p ).target.second ); if( current_veh.cpart( p ).has_flag( vehicle_part::targets_grid ) ) { - if( process_grid( target_pos ) ) { - break; + if( process_grid( target_pos ) == tvr::stop ) { + return; } } else { - if( process_vehicle( target_pos ) ) { - break; + if( process_vehicle( target_pos ) == tvr::stop ) { + return; } } } @@ -4912,14 +4923,13 @@ int traverse( StartPoint *start, int amount, } for( const tripoint_abs_ms &target_pos : connector->connected_vehicles ) { - if( process_vehicle( target_pos ) ) { - break; + if( process_vehicle( target_pos ) == tvr::stop ) { + return; } } } } } - return amount; } } // namespace distribution_graph @@ -4951,18 +4961,20 @@ int vehicle::charge_battery( int amount, bool include_other_vehicles ) } } - auto charge_veh = []( vehicle * veh, int amount ) { - g->u.add_msg_if_player( m_debug, "CHv: %d", amount ); - return veh->charge_battery( amount, false ); - }; - auto charge_grid = []( distribution_grid * grid, int amount ) { - g->u.add_msg_if_player( m_debug, "CHg: %d", amount ); - return grid->mod_resource( amount, false ); - }; - if( amount > 0 && include_other_vehicles ) { // still a bit of charge we could send out... - amount = distribution_graph::traverse( this, amount, charge_veh, charge_grid ); + using tvr = distribution_graph::traverse_visitor_result; + auto charge_veh = [&amount]( vehicle & veh ) { + g->u.add_msg_if_player( m_debug, "CHv: %d", amount ); + amount = veh.charge_battery( amount, false ); + return amount > 0 ? tvr::continue_further : tvr::stop; + }; + auto charge_grid = [&amount]( distribution_grid & grid ) { + g->u.add_msg_if_player( m_debug, "CHg: %d", amount ); + amount = grid.mod_resource( amount, false ); + return amount > 0 ? tvr::continue_further : tvr::stop; + }; + distribution_graph::traverse( *this, charge_veh, charge_grid ); } @@ -4995,17 +5007,20 @@ int vehicle::discharge_battery( int amount, bool recurse ) } } - auto discharge_vehicle = []( vehicle * veh, int amount ) { - g->u.add_msg_if_player( m_debug, "CHv: %d", amount ); - return veh->discharge_battery( amount, false ); - }; - auto discharge_grid = []( distribution_grid * grid, int amount ) { - g->u.add_msg_if_player( m_debug, "CHg: %d", amount ); - return -grid->mod_resource( -amount, false ); - }; if( amount > 0 && recurse ) { // need more power! - amount = distribution_graph::traverse( this, amount, discharge_vehicle, discharge_grid ); + using tvr = distribution_graph::traverse_visitor_result; + auto discharge_vehicle = [&amount]( vehicle & veh ) { + g->u.add_msg_if_player( m_debug, "CHv: %d", amount ); + amount = veh.discharge_battery( amount, false ); + return amount > 0 ? tvr::continue_further : tvr::stop; + }; + auto discharge_grid = [&amount]( distribution_grid & grid ) { + g->u.add_msg_if_player( m_debug, "CHg: %d", amount ); + amount = -grid.mod_resource( -amount, false ); + return amount > 0 ? tvr::continue_further : tvr::stop; + }; + distribution_graph::traverse( *this, discharge_vehicle, discharge_grid ); } return amount; // non-zero if we weren't able to fulfill demand. @@ -5389,10 +5404,8 @@ void vehicle::gain_moves() // Force off-map vehicles to load by visiting them every time we gain moves. // Shouldn't be too expensive if there aren't fifty trillion vehicles in the graph... // ...and if there are, it's the player's fault for putting them there. - auto nil_visitor = []( vehicle *, int amount ) { - return amount; - }; - traverse_vehicle_graph( this, 1, nil_visitor ); + distribution_graph::traverse( *this, distribution_graph::noop_visitor_veh, + distribution_graph::noop_visitor_grid ); if( check_environmental_effects ) { check_environmental_effects = do_environmental_effects(); diff --git a/src/vehicle.h b/src/vehicle.h index 9c3b26462a9b..c205cf2328c2 100644 --- a/src/vehicle.h +++ b/src/vehicle.h @@ -748,22 +748,6 @@ class vehicle return find_vehicle( where.raw() ); } - private: - /** - * Traverses the graph of connected vehicles, starting from start_veh, and continuing - * along all vehicles connected by some kind of POWER_TRANSFER part. - * @param start_veh The vehicle to start traversing from. NB: the start_vehicle is - * assumed to have been already visited! - * @param amount An amount of power to traverse with. This is passed back to the visitor, - * and reset to the visitor's return value at each step. - * @param action A function(vehicle* veh, int amount, int loss) returning int. The function - * may do whatever it desires, and may be a lambda (including a capturing lambda). - * NB: returning 0 from a visitor will stop traversal immediately! - * @return The last visitor's return value. - */ - template - static int traverse_vehicle_graph( Vehicle *start_veh, int amount, Func action ); - public: vehicle( const vproto_id &type_id, int init_veh_fuel = -1, int init_veh_status = -1 ); vehicle(); vehicle( const vehicle & ) = delete;