From 06cc930e47cfcf5509fb964da1adb38d13a0b5ef Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Tue, 1 Sep 2015 22:39:15 +0100 Subject: [PATCH 1/5] Improve insertion and removal of 'or if' conditionals. Allow inserting an 'or if' immediately after 'if', 'else if', 'or if'. Removing an 'or if' no longer removes the associated block. --- src/tracerestrict.cpp | 2 +- src/tracerestrict_gui.cpp | 30 +++++++++++++++++++----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/tracerestrict.cpp b/src/tracerestrict.cpp index 968a22c8ea..57ab03856a 100644 --- a/src/tracerestrict.cpp +++ b/src/tracerestrict.cpp @@ -807,7 +807,7 @@ CommandCost CmdProgramSignalTraceRestrict(TileIndex tile, DoCommandFlag flags, u case TRDCT_REMOVE_ITEM: { TraceRestrictItem old_item = *TraceRestrictProgram::InstructionAt(items, offset); - if (IsTraceRestrictConditional(old_item)) { + if (IsTraceRestrictConditional(old_item) && GetTraceRestrictCondFlags(old_item) != TRCF_OR) { bool remove_whole_block = false; if (GetTraceRestrictCondFlags(old_item) == 0) { if (GetTraceRestrictType(old_item) == TRIT_COND_ENDIF) { diff --git a/src/tracerestrict_gui.cpp b/src/tracerestrict_gui.cpp index bf9f9652e5..b189cdc249 100644 --- a/src/tracerestrict_gui.cpp +++ b/src/tracerestrict_gui.cpp @@ -115,18 +115,20 @@ struct TraceRestrictDropDownListSet { static const StringID _program_insert_str[] = { STR_TRACE_RESTRICT_CONDITIONAL_IF, STR_TRACE_RESTRICT_CONDITIONAL_ELIF, + STR_TRACE_RESTRICT_CONDITIONAL_ORIF, STR_TRACE_RESTRICT_CONDITIONAL_ELSE, STR_TRACE_RESTRICT_PF_DENY, STR_TRACE_RESTRICT_PF_PENALTY, INVALID_STRING_ID }; -static const uint _program_insert_else_flag = 0x100; ///< flag to indicate that TRCF_ELSE should be set -static const uint32 _program_insert_else_hide_mask = 4; ///< disable bitmask for else +static const uint32 _program_insert_else_hide_mask = 8; ///< disable bitmask for else +static const uint32 _program_insert_or_if_hide_mask = 4; ///< disable bitmask for elif static const uint32 _program_insert_else_if_hide_mask = 2; ///< disable bitmask for elif static const uint _program_insert_val[] = { TRIT_COND_UNDEFINED, // if block - TRIT_COND_UNDEFINED | _program_insert_else_flag, // elif block - TRIT_COND_ENDIF | _program_insert_else_flag, // else block + TRIT_COND_UNDEFINED | (TRCF_ELSE << 16), // elif block + TRIT_COND_UNDEFINED | (TRCF_OR << 16), // orif block + TRIT_COND_ENDIF | (TRCF_ELSE << 16), // else block TRIT_PF_DENY, // deny TRIT_PF_PENALTY, // penalty }; @@ -732,7 +734,7 @@ public: return; } - uint32 disabled = 0; + uint32 disabled = _program_insert_or_if_hide_mask; TraceRestrictItem item = this->GetSelected(); if (GetTraceRestrictType(item) == TRIT_COND_ENDIF || (IsTraceRestrictConditional(item) && GetTraceRestrictCondFlags(item) != 0)) { @@ -744,6 +746,15 @@ public: // can't insert else/end if here disabled |= _program_insert_else_hide_mask | _program_insert_else_if_hide_mask; } + if (this->selected_instruction > 1) { + TraceRestrictItem prev_item = this->GetItem(this->GetProgram(), this->selected_instruction - 1); + if (IsTraceRestrictConditional(prev_item) && GetTraceRestrictType(prev_item) != TRIT_COND_ENDIF) { + // previous item is either: an if, or an else/or if + + // else if has same validation rules as or if, use it instead of creating another test function + if (ElseIfInsertionDryRun(false)) disabled &= ~_program_insert_or_if_hide_mask; + } + } this->ShowDropDownListWithValue(&_program_insert, 0, true, TR_WIDGET_INSERT, disabled, 0, 0); break; @@ -914,13 +925,10 @@ public: case TR_WIDGET_INSERT: { TraceRestrictItem insert_item = 0; - bool have_else = false; - if (value & _program_insert_else_flag) { - value &= ~_program_insert_else_flag; - have_else = true; - } + TraceRestrictCondFlags cond_flags = static_cast(value >> 16); + value &= 0xFFFF; SetTraceRestrictTypeAndNormalise(insert_item, static_cast(value)); - if (have_else) SetTraceRestrictCondFlags(insert_item, TRCF_ELSE); // this needs to happen after calling SetTraceRestrictTypeAndNormalise + SetTraceRestrictCondFlags(insert_item, cond_flags); // this needs to happen after calling SetTraceRestrictTypeAndNormalise this->expecting_inserted_item = insert_item; TraceRestrictDoCommandP(this->tile, this->track, TRDCT_INSERT_ITEM, this->selected_instruction - 1, insert_item, STR_TRACE_RESTRICT_ERROR_CAN_T_INSERT_ITEM); From 06a1d7c5c9b541ce2d7bd8b510080643f5a996f6 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 2 Sep 2015 01:39:33 +0100 Subject: [PATCH 2/5] Add reserve through action to trace restrict programs. This only changes the behaviour of PBS reservations which would otherwise terminate at a PBS signal. If the signal restriction sets the reserve through state, the reservation continues through the signal, and the signal is set to green. --- src/lang/english.txt | 2 + src/pathfinder/yapf/yapf_rail.cpp | 2 +- src/pbs.cpp | 75 ++++++++++++++++++++++++++++++- src/pbs.h | 1 + src/tracerestrict.cpp | 9 ++++ src/tracerestrict.h | 5 +++ src/tracerestrict_gui.cpp | 34 ++++++++++++++ src/train_cmd.cpp | 2 +- 8 files changed, 126 insertions(+), 4 deletions(-) diff --git a/src/lang/english.txt b/src/lang/english.txt index a80d411140..49543dca3a 100644 --- a/src/lang/english.txt +++ b/src/lang/english.txt @@ -2413,6 +2413,8 @@ STR_TRACE_RESTRICT_END :End STR_TRACE_RESTRICT_PF_DENY :Deny STR_TRACE_RESTRICT_PF_ALLOW :Allow STR_TRACE_RESTRICT_PF_ALLOW_LONG :Allow (cancel previous Deny) +STR_TRACE_RESTRICT_RESERVE_THROUGH :Reserve through +STR_TRACE_RESTRICT_RESERVE_THROUGH_CANCEL :Cancel reserve through STR_TRACE_RESTRICT_PF_PENALTY :Penalty STR_TRACE_RESTRICT_PF_VALUE_SMALL :small STR_TRACE_RESTRICT_PF_VALUE_MEDIUM :medium diff --git a/src/pathfinder/yapf/yapf_rail.cpp b/src/pathfinder/yapf/yapf_rail.cpp index 5324e4fc87..3e8daf0c91 100644 --- a/src/pathfinder/yapf/yapf_rail.cpp +++ b/src/pathfinder/yapf/yapf_rail.cpp @@ -97,7 +97,7 @@ private: m_res_fail_td = td; } } else { - if (!TryReserveRailTrack(tile, TrackdirToTrack(td))) { + if (!TryReserveRailTrackdir(tile, td)) { /* Tile couldn't be reserved, undo. */ m_res_fail_tile = tile; m_res_fail_td = td; diff --git a/src/pbs.cpp b/src/pbs.cpp index 133293909f..16d0358b04 100644 --- a/src/pbs.cpp +++ b/src/pbs.cpp @@ -14,6 +14,7 @@ #include "vehicle_func.h" #include "newgrf_station.h" #include "pathfinder/follow_track.hpp" +#include "tracerestrict.h" #include "safeguards.h" @@ -71,6 +72,24 @@ void SetRailStationPlatformReservation(TileIndex start, DiagDirection dir, bool } while (IsCompatibleTrainStationTile(tile, start)); } +/** + * Try to reserve a specific track on a tile + * This also sets PBS signals to green if reserving through the facing track direction + * @param tile the tile + * @param t the track + * @param trigger_stations whether to call station randomisation trigger + * @return \c true if reservation was successful, i.e. the track was + * free and didn't cross any other reserved tracks. + */ +bool TryReserveRailTrackdir(TileIndex tile, Trackdir td, bool trigger_stations) +{ + bool success = TryReserveRailTrack(tile, TrackdirToTrack(td), trigger_stations); + if (success && HasPbsSignalOnTrackdir(tile, td)) { + SetSignalStateByTrackdir(tile, td, SIGNAL_STATE_GREEN); + } + return success; +} + /** * Try to reserve a specific track on a tile * @param tile the tile @@ -370,6 +389,46 @@ Train *GetTrainForReservation(TileIndex tile, Track track) return NULL; } +/** + * This is called to retrieve the previous signal, as required + * This is not run all the time as it is somewhat expensive and most restrictions will not test for the previous signal + */ +static TileIndex IsSafeWaitingPositionTraceRestrictPreviousSignalCallback(const Train *v, const void *) +{ + // scan forwards from vehicle position, for the case that train is waiting at/approaching PBS signal + + TileIndex tile = v->tile; + Trackdir trackdir = v->GetVehicleTrackdir(); + + CFollowTrackRail ft(v); + + for (;;) { + if (IsTileType(tile, MP_RAILWAY) && HasSignalOnTrackdir(tile, trackdir)) { + if (HasPbsSignalOnTrackdir(tile, trackdir)) { + // found PBS signal + return tile; + } else { + // wrong type of signal + return INVALID_TILE; + } + } + + // advance to next tile + if (!ft.Follow(tile, trackdir)) { + // ran out of track + return INVALID_TILE; + } + + if (KillFirstBit(ft.m_new_td_bits) != TRACKDIR_BIT_NONE) { + // reached a junction tile + return INVALID_TILE; + } + + tile = ft.m_new_tile; + trackdir = FindFirstTrackdir(ft.m_new_td_bits); + } +} + /** * Determine whether a certain track on a tile is a safe position to end a path. * @@ -405,8 +464,20 @@ bool IsSafeWaitingPosition(const Train *v, TileIndex tile, Trackdir trackdir, bo if (ft.m_new_td_bits != TRACKDIR_BIT_NONE && KillFirstBit(ft.m_new_td_bits) == TRACKDIR_BIT_NONE) { Trackdir td = FindFirstTrackdir(ft.m_new_td_bits); - /* PBS signal on next trackdir? Safe position. */ - if (HasPbsSignalOnTrackdir(ft.m_new_tile, td)) return true; + /* PBS signal on next trackdir? Conditionally safe position. */ + if (HasPbsSignalOnTrackdir(ft.m_new_tile, td)) { + if (IsRestrictedSignal(ft.m_new_tile)) { + const TraceRestrictProgram *prog = GetExistingTraceRestrictProgram(ft.m_new_tile, TrackdirToTrack(td)); + if (prog) { + TraceRestrictProgramResult out; + prog->Execute(v, TraceRestrictProgramInput(tile, trackdir, &IsSafeWaitingPositionTraceRestrictPreviousSignalCallback, nullptr), out); + if (out.flags & TRPRF_RESERVE_THROUGH) { + return false; + } + } + } + return true; + } /* One-way PBS signal against us? Safe if end-of-line is allowed. */ if (IsTileType(ft.m_new_tile, MP_RAILWAY) && HasSignalOnTrackdir(ft.m_new_tile, ReverseTrackdir(td)) && GetSignalType(ft.m_new_tile, TrackdirToTrack(td)) == SIGTYPE_PBS_ONEWAY) { diff --git a/src/pbs.h b/src/pbs.h index a02d4d06e1..5a0bd96651 100644 --- a/src/pbs.h +++ b/src/pbs.h @@ -22,6 +22,7 @@ TrackBits GetReservedTrackbits(TileIndex t); void SetRailStationPlatformReservation(TileIndex start, DiagDirection dir, bool b); bool TryReserveRailTrack(TileIndex tile, Track t, bool trigger_stations = true); +bool TryReserveRailTrackdir(TileIndex tile, Trackdir td, bool trigger_stations = true); void UnreserveRailTrack(TileIndex tile, Track t); /** This struct contains information about the end of a reserved path. */ diff --git a/src/tracerestrict.cpp b/src/tracerestrict.cpp index 57ab03856a..84710ce164 100644 --- a/src/tracerestrict.cpp +++ b/src/tracerestrict.cpp @@ -382,6 +382,14 @@ void TraceRestrictProgram::Execute(const Train* v, const TraceRestrictProgramInp } break; + case TRIT_RESERVE_THROUGH: + if (GetTraceRestrictValue(item)) { + out.flags &= ~TRPRF_RESERVE_THROUGH; + } else { + out.flags |= TRPRF_RESERVE_THROUGH; + } + break; + default: NOT_REACHED(); } @@ -502,6 +510,7 @@ void SetTraceRestrictValueDefault(TraceRestrictItem &item, TraceRestrictValueTyp case TRVT_DENY: case TRVT_SPEED: case TRVT_TILE_INDEX: + case TRVT_RESERVE_THROUGH: SetTraceRestrictValue(item, 0); SetTraceRestrictAuxField(item, 0); break; diff --git a/src/tracerestrict.h b/src/tracerestrict.h index 6913234bc7..63a80b9d21 100644 --- a/src/tracerestrict.h +++ b/src/tracerestrict.h @@ -97,6 +97,7 @@ enum TraceRestrictItemType { TRIT_NULL = 0, ///< Null-type, not in programs and not valid for execution, mainly used with TraceRestrictNullTypeSpecialValue for start/end TRIT_PF_DENY = 1, ///< Pathfinder deny/allow TRIT_PF_PENALTY = 2, ///< Add to pathfinder penalty + TRIT_RESERVE_THROUGH = 3, ///< Reserve through PBS signal TRIT_COND_BEGIN = 8, ///< Start of conditional item types, note that this has the save value as TRIT_COND_ENDIF TRIT_COND_ENDIF = 8, ///< This is an endif block or an else block @@ -192,6 +193,7 @@ enum TraceRestrictPathfinderPenaltyPresetIndex { */ enum TraceRestrictProgramResultFlags { TRPRF_DENY = 1 << 0, ///< Pathfinder deny is set + TRPRF_RESERVE_THROUGH = 1 << 1, ///< Reserve through is set }; DECLARE_ENUM_AS_BIT_SET(TraceRestrictProgramResultFlags) @@ -392,6 +394,7 @@ enum TraceRestrictValueType { TRVT_DIRECTION = 7, ///< takes a TraceRestrictDirectionTypeSpecialValue TRVT_TILE_INDEX = 8, ///< takes a TileIndex in the next item slot TRVT_PF_PENALTY = 9, ///< takes a pathfinder penalty value or preset index, as per the auxiliary field as type: TraceRestrictPathfinderPenaltyAuxField + TRVT_RESERVE_THROUGH = 10,///< takes a value 0 = reserve through, 1 = cancel previous reserve through }; /** @@ -463,6 +466,8 @@ static inline TraceRestrictTypePropertySet GetTraceRestrictTypeProperties(TraceR out.value_type = TRVT_PF_PENALTY; } else if (GetTraceRestrictType(item) == TRIT_PF_DENY) { out.value_type = TRVT_DENY; + } else if (GetTraceRestrictType(item) == TRIT_RESERVE_THROUGH) { + out.value_type = TRVT_RESERVE_THROUGH; } else { out.value_type = TRVT_NONE; } diff --git a/src/tracerestrict_gui.cpp b/src/tracerestrict_gui.cpp index b189cdc249..0737f540a9 100644 --- a/src/tracerestrict_gui.cpp +++ b/src/tracerestrict_gui.cpp @@ -119,6 +119,7 @@ static const StringID _program_insert_str[] = { STR_TRACE_RESTRICT_CONDITIONAL_ELSE, STR_TRACE_RESTRICT_PF_DENY, STR_TRACE_RESTRICT_PF_PENALTY, + STR_TRACE_RESTRICT_RESERVE_THROUGH, INVALID_STRING_ID }; static const uint32 _program_insert_else_hide_mask = 8; ///< disable bitmask for else @@ -131,6 +132,7 @@ static const uint _program_insert_val[] = { TRIT_COND_ENDIF | (TRCF_ELSE << 16), // else block TRIT_PF_DENY, // deny TRIT_PF_PENALTY, // penalty + TRIT_RESERVE_THROUGH, // reserve through }; /** insert drop down list strings and values */ @@ -153,6 +155,21 @@ static const TraceRestrictDropDownListSet _deny_value = { _deny_value_str, _deny_value_val, }; +static const StringID _reserve_through_value_str[] = { + STR_TRACE_RESTRICT_RESERVE_THROUGH, + STR_TRACE_RESTRICT_RESERVE_THROUGH_CANCEL, + INVALID_STRING_ID +}; +static const uint _reserve_through_value_val[] = { + 0, + 1, +}; + +/** value drop down list for deny types strings and values */ +static const TraceRestrictDropDownListSet _reserve_through_value = { + _reserve_through_value_str, _reserve_through_value_val, +}; + static const StringID _direction_value_str[] = { STR_TRACE_RESTRICT_DIRECTION_FRONT, STR_TRACE_RESTRICT_DIRECTION_BACK, @@ -211,11 +228,13 @@ static const TraceRestrictDropDownListSet *GetTypeDropDownListSet(TraceRestrictI static const StringID str_action[] = { STR_TRACE_RESTRICT_PF_DENY, STR_TRACE_RESTRICT_PF_PENALTY, + STR_TRACE_RESTRICT_RESERVE_THROUGH, INVALID_STRING_ID, }; static const uint val_action[] = { TRIT_PF_DENY, TRIT_PF_PENALTY, + TRIT_RESERVE_THROUGH, }; static const TraceRestrictDropDownListSet set_action = { str_action, val_action, @@ -642,6 +661,10 @@ static void DrawInstructionString(const TraceRestrictProgram *prog, TraceRestric } break; + case TRIT_RESERVE_THROUGH: + instruction_string = GetTraceRestrictValue(item) ? STR_TRACE_RESTRICT_RESERVE_THROUGH_CANCEL : STR_TRACE_RESTRICT_RESERVE_THROUGH; + break; + default: NOT_REACHED(); break; @@ -843,6 +866,10 @@ public: this->ShowDropDownListWithValue(&_pf_penalty_dropdown, GetPathfinderPenaltyDropdownIndex(item), false, TR_WIDGET_VALUE_DROPDOWN, 0, 0, 0); break; + case TRVT_RESERVE_THROUGH: + this->ShowDropDownListWithValue(&_reserve_through_value, GetTraceRestrictValue(item), false, TR_WIDGET_VALUE_DROPDOWN, 0, 0, 0); + break; + default: break; } @@ -1581,6 +1608,13 @@ private: } break; + case TRVT_RESERVE_THROUGH: + right_sel->SetDisplayedPlane(DPR_VALUE_DROPDOWN); + this->EnableWidget(TR_WIDGET_VALUE_DROPDOWN); + this->GetWidget(TR_WIDGET_VALUE_DROPDOWN)->widget_data = + GetTraceRestrictValue(item) ? STR_TRACE_RESTRICT_RESERVE_THROUGH_CANCEL : STR_TRACE_RESTRICT_RESERVE_THROUGH; + break; + default: break; } diff --git a/src/train_cmd.cpp b/src/train_cmd.cpp index 8f20973b35..7023922a69 100644 --- a/src/train_cmd.cpp +++ b/src/train_cmd.cpp @@ -2386,7 +2386,7 @@ static PBSTileInfo ExtendTrainReservation(const Train *v, TrackBits *new_tracks, return PBSTileInfo(tile, cur_td, true); } - if (!TryReserveRailTrack(tile, TrackdirToTrack(cur_td))) break; + if (!TryReserveRailTrackdir(tile, cur_td)) break; } if (ft.m_err == CFollowTrackRail::EC_OWNER || ft.m_err == CFollowTrackRail::EC_NO_WAY) { From e2435c7169c427af0df1fbf539b874b4905c2834 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 2 Sep 2015 18:35:56 +0100 Subject: [PATCH 3/5] Only run trace restrict programs if they contain relevant actions. Only run in pathfinder case if deny or penalty are present. Only run in is safe waiting tile case is reserve through is present. Presence is determined at program validation time and cached in the program structure. Validator now checks for unknown non-conditional instructions. --- src/lang/english.txt | 1 + src/pathfinder/yapf/yapf_costrail.hpp | 2 +- src/pbs.cpp | 2 +- src/tracerestrict.cpp | 23 ++++++++++++++++++++--- src/tracerestrict.h | 23 +++++++++++++++++------ src/tracerestrict_gui.cpp | 3 ++- 6 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/lang/english.txt b/src/lang/english.txt index 49543dca3a..13b2daa5af 100644 --- a/src/lang/english.txt +++ b/src/lang/english.txt @@ -2462,6 +2462,7 @@ STR_TRACE_RESTRICT_ERROR_VALIDATE_END_CONDSTACK :Validation fail STR_TRACE_RESTRICT_ERROR_VALIDATE_NO_IF :Validation failed: else/endif without opening if STR_TRACE_RESTRICT_ERROR_VALIDATE_DUP_ELSE :Validation failed: duplicate else STR_TRACE_RESTRICT_ERROR_VALIDATE_ELIF_NO_IF :Validation failed: else if without opening if +STR_TRACE_RESTRICT_ERROR_VALIDATE_UNKNOWN_INSTRUCTION :Validation failed: unknown instruction STR_TRACE_RESTRICT_ERROR_SOURCE_SAME_AS_TARGET :Source and target signals are the same STR_TRACE_RESTRICT_ERROR_CAN_T_RESET_SIGNAL :{WHITE}Can't reset signal STR_TRACE_RESTRICT_ERROR_CAN_T_COPY_PROGRAM :{WHITE}Can't copy program diff --git a/src/pathfinder/yapf/yapf_costrail.hpp b/src/pathfinder/yapf/yapf_costrail.hpp index f4ff174e43..b010fe4d8a 100644 --- a/src/pathfinder/yapf/yapf_costrail.hpp +++ b/src/pathfinder/yapf/yapf_costrail.hpp @@ -264,7 +264,7 @@ private: inline bool ExecuteTraceRestrict(Node& n, TileIndex tile, Trackdir trackdir, int& cost, TraceRestrictProgramResult &out) { const TraceRestrictProgram *prog = GetExistingTraceRestrictProgram(tile, TrackdirToTrack(trackdir)); - if (prog) { + if (prog && prog->actions_used_flags & TRPAUF_PF) { prog->Execute(Yapf().GetVehicle(), TraceRestrictProgramInput(tile, trackdir, &TraceRestrictPreviousSignalCallback, &n), out); if (out.flags & TRPRF_DENY) { n.m_segment->m_end_segment_reason |= ESRB_DEAD_END; diff --git a/src/pbs.cpp b/src/pbs.cpp index 16d0358b04..78a8337cb6 100644 --- a/src/pbs.cpp +++ b/src/pbs.cpp @@ -468,7 +468,7 @@ bool IsSafeWaitingPosition(const Train *v, TileIndex tile, Trackdir trackdir, bo if (HasPbsSignalOnTrackdir(ft.m_new_tile, td)) { if (IsRestrictedSignal(ft.m_new_tile)) { const TraceRestrictProgram *prog = GetExistingTraceRestrictProgram(ft.m_new_tile, TrackdirToTrack(td)); - if (prog) { + if (prog && prog->actions_used_flags & TRPAUF_RESERVE_THROUGH) { TraceRestrictProgramResult out; prog->Execute(v, TraceRestrictProgramInput(tile, trackdir, &IsSafeWaitingPositionTraceRestrictPreviousSignalCallback, nullptr), out); if (out.flags & TRPRF_RESERVE_THROUGH) { diff --git a/src/tracerestrict.cpp b/src/tracerestrict.cpp index 84710ce164..ff86f5c80a 100644 --- a/src/tracerestrict.cpp +++ b/src/tracerestrict.cpp @@ -413,12 +413,14 @@ void TraceRestrictProgram::DecrementRefCount() { /** * Validate a instruction list * Returns successful result if program seems OK - * This only validates that conditional nesting is correct, at present + * This only validates that conditional nesting is correct, + * and that all non-conditionals have a known type, at present */ -CommandCost TraceRestrictProgram::Validate(const std::vector &items) { +CommandCost TraceRestrictProgram::Validate(const std::vector &items, TraceRestrictProgramActionsUsedFlags &actions_used_flags) { // static to avoid needing to re-alloc/resize on each execution static std::vector condstack; condstack.clear(); + actions_used_flags = static_cast(0); size_t size = items.size(); for (size_t i = 0; i < size; i++) { @@ -462,6 +464,19 @@ CommandCost TraceRestrictProgram::Validate(const std::vector return_cmd_error(STR_TRACE_RESTRICT_ERROR_OFFSET_TOO_LARGE); // instruction ran off end } } + switch (GetTraceRestrictType(item)) { + case TRIT_PF_DENY: + case TRIT_PF_PENALTY: + actions_used_flags |= TRPAUF_PF; + break; + + case TRIT_RESERVE_THROUGH: + actions_used_flags |= TRPAUF_RESERVE_THROUGH; + break; + + default: + return_cmd_error(STR_TRACE_RESTRICT_ERROR_VALIDATE_UNKNOWN_INSTRUCTION); + } } } if(!condstack.empty()) { @@ -887,7 +902,8 @@ CommandCost CmdProgramSignalTraceRestrict(TileIndex tile, DoCommandFlag flags, u break; } - CommandCost validation_result = TraceRestrictProgram::Validate(items); + TraceRestrictProgramActionsUsedFlags actions_used_flags; + CommandCost validation_result = TraceRestrictProgram::Validate(items, actions_used_flags); if (validation_result.Failed()) { return validation_result; } @@ -897,6 +913,7 @@ CommandCost CmdProgramSignalTraceRestrict(TileIndex tile, DoCommandFlag flags, u // move in modified program prog->items.swap(items); + prog->actions_used_flags = actions_used_flags; if (prog->items.size() == 0 && prog->refcount == 1) { // program is empty, and this tile is the only reference to it diff --git a/src/tracerestrict.h b/src/tracerestrict.h index 63a80b9d21..1704dc4c79 100644 --- a/src/tracerestrict.h +++ b/src/tracerestrict.h @@ -197,6 +197,15 @@ enum TraceRestrictProgramResultFlags { }; DECLARE_ENUM_AS_BIT_SET(TraceRestrictProgramResultFlags) +/** + * Enumeration for TraceRestrictProgram::actions_used_flags + */ +enum TraceRestrictProgramActionsUsedFlags { + TRPAUF_PF = 1 << 0, ///< Pathfinder deny or penalty are present + TRPAUF_RESERVE_THROUGH = 1 << 1, ///< Reserve through action is present +}; +DECLARE_ENUM_AS_BIT_SET(TraceRestrictProgramActionsUsedFlags) + /** * Execution input of a TraceRestrictProgram */ @@ -230,9 +239,10 @@ struct TraceRestrictProgramResult { struct TraceRestrictProgram : TraceRestrictProgramPool::PoolItem<&_tracerestrictprogram_pool> { std::vector items; uint32 refcount; + TraceRestrictProgramActionsUsedFlags actions_used_flags; TraceRestrictProgram() - : refcount(0) { } + : refcount(0), actions_used_flags(static_cast(0)) { } void Execute(const Train *v, const TraceRestrictProgramInput &input, TraceRestrictProgramResult &out) const; @@ -243,7 +253,7 @@ struct TraceRestrictProgram : TraceRestrictProgramPool::PoolItem<&_tracerestrict void DecrementRefCount(); - static CommandCost Validate(const std::vector &items); + static CommandCost Validate(const std::vector &items, TraceRestrictProgramActionsUsedFlags &actions_used_flags); static size_t InstructionOffsetToArrayOffset(const std::vector &items, size_t offset); @@ -285,10 +295,11 @@ struct TraceRestrictProgram : TraceRestrictProgramPool::PoolItem<&_tracerestrict return items.begin() + TraceRestrictProgram::InstructionOffsetToArrayOffset(items, instruction_offset); } - /** - * Call validation function on current program instruction list - */ - CommandCost Validate() const { return TraceRestrictProgram::Validate(items); } + /** Call validation function on current program instruction list and set actions_used_flags */ + CommandCost Validate() + { + return TraceRestrictProgram::Validate(items, actions_used_flags); + } }; /** Get TraceRestrictItem type field */ diff --git a/src/tracerestrict_gui.cpp b/src/tracerestrict_gui.cpp index 0737f540a9..5125c053c3 100644 --- a/src/tracerestrict_gui.cpp +++ b/src/tracerestrict_gui.cpp @@ -1680,7 +1680,8 @@ private: items.insert(items.begin() + array_offset, item); } - return TraceRestrictProgram::Validate(items).Succeeded(); + TraceRestrictProgramActionsUsedFlags actions_used_flags; + return TraceRestrictProgram::Validate(items, actions_used_flags).Succeeded(); } /** From d9acfc4599089ef82de03715344940899e7202ae Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 2 Sep 2015 19:13:30 +0100 Subject: [PATCH 4/5] Fix validation of dual item instructions. --- src/tracerestrict.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/tracerestrict.cpp b/src/tracerestrict.cpp index ff86f5c80a..85c0fd0631 100644 --- a/src/tracerestrict.cpp +++ b/src/tracerestrict.cpp @@ -427,6 +427,14 @@ CommandCost TraceRestrictProgram::Validate(const std::vector TraceRestrictItem item = items[i]; TraceRestrictItemType type = GetTraceRestrictType(item); + // check multi-word instructions + if (IsTraceRestrictDoubleItem(item)) { + i++; + if (i >= size) { + return_cmd_error(STR_TRACE_RESTRICT_ERROR_OFFSET_TOO_LARGE); // instruction ran off end + } + } + if (IsTraceRestrictConditional(item)) { TraceRestrictCondFlags condflags = GetTraceRestrictCondFlags(item); @@ -457,13 +465,6 @@ CommandCost TraceRestrictProgram::Validate(const std::vector HandleCondition(condstack, condflags, true); } } else { - // check multi-word instructions - if (IsTraceRestrictDoubleItem(item)) { - i++; - if (i >= size) { - return_cmd_error(STR_TRACE_RESTRICT_ERROR_OFFSET_TOO_LARGE); // instruction ran off end - } - } switch (GetTraceRestrictType(item)) { case TRIT_PF_DENY: case TRIT_PF_PENALTY: From 73b69c5594bbe8c0f8e26f34fbc66781ddeabbc3 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 2 Sep 2015 20:51:30 +0100 Subject: [PATCH 5/5] Validate type of all instructions, log validation failures at load. The validator now checks that the type of conditional instructions is known. On a validation failure, the load code now outputs a corrupt savegame message, with the validation error message and a program dump, instead of using an assertion. --- src/saveload/tracerestrict_sl.cpp | 18 ++++++++++++++++-- src/tracerestrict.cpp | 19 ++++++++++++++++++- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/saveload/tracerestrict_sl.cpp b/src/saveload/tracerestrict_sl.cpp index 93d43cf3ea..8e983a78b3 100644 --- a/src/saveload/tracerestrict_sl.cpp +++ b/src/saveload/tracerestrict_sl.cpp @@ -11,9 +11,10 @@ #include "../stdafx.h" #include "../tracerestrict.h" +#include "../strings_func.h" +#include "../string_func.h" #include "saveload.h" #include -#include "saveload.h" static const SaveLoad _trace_restrict_mapping_desc[] = { SLE_VAR(TraceRestrictMappingItem, program_id, SLE_UINT32), @@ -66,7 +67,20 @@ static void Load_TRRP() SlObject(&stub, _trace_restrict_program_stub_desc); prog->items.resize(stub.length); SlArray(&(prog->items[0]), stub.length, SLE_UINT32); - assert(prog->Validate().Succeeded()); + CommandCost validation_result = prog->Validate(); + if (validation_result.Failed()) { + char str[4096]; + char *strend = str + seprintf(str, lastof(str), "Trace restrict program %d: %s\nProgram dump:", + index, GetStringPtr(validation_result.GetErrorMessage())); + for (unsigned int i = 0; i < prog->items.size(); i++) { + if (i % 3) { + strend += seprintf(strend, lastof(str), " %08X", prog->items[i]); + } else { + strend += seprintf(strend, lastof(str), "\n%4u: %08X", i, prog->items[i]); + } + } + SlErrorCorrupt(str); + } } } diff --git a/src/tracerestrict.cpp b/src/tracerestrict.cpp index 85c0fd0631..c69da07ce8 100644 --- a/src/tracerestrict.cpp +++ b/src/tracerestrict.cpp @@ -414,7 +414,7 @@ void TraceRestrictProgram::DecrementRefCount() { * Validate a instruction list * Returns successful result if program seems OK * This only validates that conditional nesting is correct, - * and that all non-conditionals have a known type, at present + * and that all instructions have a known type, at present */ CommandCost TraceRestrictProgram::Validate(const std::vector &items, TraceRestrictProgramActionsUsedFlags &actions_used_flags) { // static to avoid needing to re-alloc/resize on each execution @@ -464,6 +464,23 @@ CommandCost TraceRestrictProgram::Validate(const std::vector } HandleCondition(condstack, condflags, true); } + + switch (GetTraceRestrictType(item)) { + case TRIT_COND_ENDIF: + case TRIT_COND_UNDEFINED: + case TRIT_COND_TRAIN_LENGTH: + case TRIT_COND_MAX_SPEED: + case TRIT_COND_CURRENT_ORDER: + case TRIT_COND_NEXT_ORDER: + case TRIT_COND_LAST_STATION: + case TRIT_COND_CARGO: + case TRIT_COND_ENTRY_DIRECTION: + case TRIT_COND_PBS_ENTRY_SIGNAL: + break; + + default: + return_cmd_error(STR_TRACE_RESTRICT_ERROR_VALIDATE_UNKNOWN_INSTRUCTION); + } } else { switch (GetTraceRestrictType(item)) { case TRIT_PF_DENY: