From 256574a5c7362a079e279995bb7d3eea37f6beeb Mon Sep 17 00:00:00 2001 From: Niko Fellner Date: Tue, 7 Oct 2025 18:33:47 +0200 Subject: [PATCH 1/2] Fix issue #4133: FinalizeAllowedVehicles incorrectly excludes vehicles with negative transits The FinalizeAllowedVehicles() function was using abs() on transit values to check against vehicle capacity, which incorrectly excluded vehicles when: - CARGO_LOAD dimensions use negative constants for cumulative load tracking - Reload dimensions use negative values at landfills/depots This fix modifies the capacity check to only apply to positive transit values, since negative transits are used for algorithmic purposes (reload semantics, load tracking) rather than actual cargo capacity requirements. Changes: - Lines 1414-1421: Pre-processing loop now only considers positive transits for max_node_transit calculation - Lines 1444-1448: Main capacity check now skips negative transits entirely This resolves the regression introduced in commit 847cfc9b93 (Dec 1, 2023) affecting OR-Tools versions 9.9.3963 through 9.14.6206. Fixes: google/or-tools#4133 Testing: test_ortools_version_standalone_codex.py (multi-class VRP with load tracking) --- ortools/constraint_solver/routing.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/ortools/constraint_solver/routing.cc b/ortools/constraint_solver/routing.cc index ebacbcc1280..373bd87cc32 100644 --- a/ortools/constraint_solver/routing.cc +++ b/ortools/constraint_solver/routing.cc @@ -1411,9 +1411,13 @@ void RoutingModel::FinalizeAllowedVehicles() { for (int node = 0; node < Size(); ++node) { if (IsStart(node)) continue; for (int callback_index : dimension->class_evaluators_) { - max_node_transit = std::max( - max_node_transit, - std::abs(UnaryTransitCallbackOrNull(callback_index)(node))); + // Only consider positive transits for capacity checks. + // Negative transits are used for reload/load tracking and should not + // trigger vehicle exclusions based on capacity. + const int64_t transit = UnaryTransitCallbackOrNull(callback_index)(node); + if (transit > 0) { + max_node_transit = std::max(max_node_transit, transit); + } } } } @@ -1437,7 +1441,11 @@ void RoutingModel::FinalizeAllowedVehicles() { // The vehicle is already forbidden for this node. continue; } - if (std::abs(transit_evaluator(node)) <= capacity) continue; + // Fix for issue #4133: Skip capacity check for negative transits. + // Negative transits are used for reload dimensions and load tracking, + // not actual cargo requirements. + const int64_t transit = transit_evaluator(node); + if (transit <= 0 || transit <= capacity) continue; // 'node' can't be served by 'vehicle', so we remove the 'vehicle' // from the node's set of allowed_vehicles_. From 6cdf0abb0c04df0c549253d541653ca667d67f7e Mon Sep 17 00:00:00 2001 From: Niko Fellner Date: Mon, 13 Oct 2025 16:46:40 +0200 Subject: [PATCH 2/2] Fix issue #4133: Correct negative transit handling in FinalizeAllowedVehicles The FinalizeAllowedVehicles() optimization incorrectly excluded vehicles from nodes with negative unary transits. The bug checked abs(transit) against capacity alone, but negative transits are feasible when abs(transit) <= capacity + slack_max (vehicle can absorb the negative transit via both capacity headroom and dimension slack). This caused heterogeneous VRP problems with reload/load-tracking dimensions to incorrectly exclude smaller vehicles, forcing suboptimal solutions with dropped stops. The fix properly bounds negative transits by combining capacity and slack_max, while positive transits continue to check capacity alone. Fixes #4133 --- ortools/routing/routing.cc | 44 ++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/ortools/routing/routing.cc b/ortools/routing/routing.cc index 76945892091..a6fa19f76fa 100644 --- a/ortools/routing/routing.cc +++ b/ortools/routing/routing.cc @@ -1401,21 +1401,27 @@ void RoutingModel::AddRouteConstraint( void RoutingModel::FinalizeAllowedVehicles() { const std::vector unary_dimensions = GetUnaryDimensions(); - // Pre-process the node transit values to find the maximum transit for each - // unary dimension to avoid unnecessary computations below. - std::vector dimension_max_node_transit(unary_dimensions.size(), 0); - for (int i = 0; i < unary_dimensions.size(); ++i) { - int64_t& max_node_transit = dimension_max_node_transit[i]; - const RoutingDimension* dimension = unary_dimensions[i]; + // Pre-process the node transit values to find, for each unary dimension, the + // maximum positive transit and the most negative transit. These bounds let us + // skip per-node checks when a vehicle's capacity and slack dominate them. + const int num_dimensions = unary_dimensions.size(); + std::vector dimension_max_positive_transit(num_dimensions, 0); + std::vector dimension_min_transit(num_dimensions, 0); + std::vector dimension_slack_max(num_dimensions, 0); + for (int i = 0; i < num_dimensions; ++i) { + int64_t& max_positive_transit = dimension_max_positive_transit[i]; + int64_t& min_transit = dimension_min_transit[i]; + const RoutingDimension* const dimension = unary_dimensions[i]; + dimension_slack_max[i] = dimension->SlackVar(0)->Max(); for (int node = 0; node < Size(); ++node) { if (IsStart(node)) continue; for (int callback_index : dimension->class_evaluators_) { - // Only consider positive transits for capacity checks. - // Negative transits are used for reload/load tracking and should not - // trigger vehicle exclusions based on capacity. const int64_t transit = UnaryTransitCallbackOrNull(callback_index)(node); - if (transit > 0) { - max_node_transit = std::max(max_node_transit, transit); + if (transit > max_positive_transit) { + max_positive_transit = transit; + } + if (transit < min_transit) { + min_transit = transit; } } } @@ -1431,7 +1437,13 @@ void RoutingModel::FinalizeAllowedVehicles() { dim->GetUnaryTransitEvaluator(vehicle); DCHECK(transit_evaluator != nullptr); const int64_t capacity = dim->vehicle_capacities()[vehicle]; - if (capacity >= dimension_max_node_transit[i]) continue; + const int64_t slack_max = dimension_slack_max[i]; + const int64_t min_allowed_transit = + CapSub(0, CapAdd(capacity, slack_max)); + if (capacity >= dimension_max_positive_transit[i] && + dimension_min_transit[i] >= min_allowed_transit) { + continue; + } for (int node = 0; node < Size(); ++node) { if (IsStart(node)) continue; @@ -1440,11 +1452,11 @@ void RoutingModel::FinalizeAllowedVehicles() { // The vehicle is already forbidden for this node. continue; } - // Fix for issue #4133: Skip capacity check for negative transits. - // Negative transits are used for reload dimensions and load tracking, - // not actual cargo requirements. + // Positive transits must fit within the vehicle capacity. Negative + // transits can only decrease the cumul as far as the capacity supplemented + // by the dimension slack allows. const int64_t transit = transit_evaluator(node); - if (transit <= 0 || transit <= capacity) continue; + if (transit <= capacity && transit >= min_allowed_transit) continue; // 'node' can't be served by 'vehicle', so we remove the 'vehicle' // from the node's set of allowed_vehicles_.