From 01ae0e45d75c30619af76ae2438dc1b92ef61e08 Mon Sep 17 00:00:00 2001 From: Karl Ostmo Date: Sun, 10 Mar 2024 19:48:26 -0700 Subject: [PATCH] react immediately to wakeups (#1736) Fixes #1598 ## Demo Illustrates immediate (same-tick) reaction to a change of a `watch`ed cell: scripts/play.sh -i data/scenarios/Testing/1598-detect-entity-change.yaml --autoplay --speed 1 ## Background Robots can be **scheduled** to wakeup from a `watch` by the `wakeWatchingRobots` function. Robots may be **actually awakened** by the `wakeUpRobotsDoneSleeping` function. Previously, `wakeWatchingRobots` would only ever schedule wakeups at `currentTick + 1`. Then, once the next tick is reached, in `wakeUpRobotsDoneSleeping` only robots that had been scheduled to wake on precisely the **current tick** could actually be awakened. But this made it impossible for one robot to cause another robot, which had been sleeping, to actually wake up within the same tick that scheduling of the wakeup is performed. The importance of this comes into play for system robots that are `watch`ing a cell and intended to react instantaneously to player actions (usually by running code in an `instant` block). During each "tick", every active robot gets exactly one "turn" to execute code. Given the following ID assignments: | Robot ID | Robot name | | --- | --- | | `0` | `base` | | `1` | `systemBot` | the `systemBot` will always execute *after* the `base` robot in a given tick. If the `systemBot` is `watch`ing a given cell, a modification of that cell by the `base` will schedule a wakeup of the `systemBot`. If the scheduled wakeup tick is `currentTick + 1`, then the `base` will have **another execution turn** before the `systemBot` gets an opportunity to react to the `base`'s action at the current tick, causing the `systemBot` to overlook actions that may be relevant to a goal condition. ## Solution In contast, if we schedule the `systemBot` wakeup for `currentTick` instead of `currentTick + 1`, then it should have an opportunity to wake, run, and react to the `base`'s action before the `base`'s next turn. But in the status quo, the selection of which robots to run in a given tick is made only once, before iteration over those robots begins. We need instead to update the robots-to-run pool dynamically, after each iteration on an individual robot. The most appropriate data structure for the iteration pool is probably a [Monotone priority queue](https://en.wikipedia.org/wiki/Monotone_priority_queue), but we approximate this with an `IntSet` of `RID`s and the [`minView`](https://hackage.haskell.org/package/containers-0.7/docs/Data-IntSet.html#v:minView) function. Being able to alter the list of active robots in the middle of a given tick's robot iteration has required a change to the `runRobotIDs` function. Instead of using a `forM_` to iterate over a static list of `RIDs`, we have a custom iteration function `iterateRobots` to handle the dynamic set. ## Performance Benchmarks were performed locally with `stack` and GHC 9.6. I had tested the replacement of the `forM_` loop with `iterateRobots` in isolation, and it had zero effect on benchmarks. But some other trivial changes tested in isolation, such as the addition of docstrings, yielded a +6% increase in benchmarks. So there seems to be some instability in the ability of the compiler to perform certain optimizations. With this PR, increases in benchmark times ranging from 7% to 11% were observed. The obvious culprit is that `wakeUpRobotsDoneSleeping` is now called N times per tick, rather than once per tick, where N is the number of robots that are initially (or become) active during that tick. ### Mitigating with an "awakened" set In the common case (i.e. when there are *no* watching robots), the `wakeUpRobotsDoneSleeping` that is now executed `N` times per tick (instead of once per tick) incurs a `Map` lookup, which is `O(log M)` in the number of distinct future wakeup times across all robots. However, those extra `N` invocations only exist to serve the purpose of `watch`ing robots that may need to wake up at the current tick. It may be more efficient to have a dedicated `Set` in the `GameState` for such robots that gets populated parallel to this insertion: https://github.com/swarm-game/swarm/blob/ad5c58917e058306d33e86f3b85e7825d64e54f4/src/swarm-engine/Swarm/Game/State/Robot.hs#L387 Then if this set remains `null`, we can avoid paying for an additional `O(N * log M)` operations entailed by the `Map` lookups into `internalWaitingRobots`. #### Result Indeed, storing awakened bots in a new `currentTickWakeableBots` set restored the benchmarks nearly to the baseline. Instead of the regressions in the 10-20% range observed before, now only a few benchmarks had at most 3-4% increases. ### CI regressions In CI, (`snake`, `powerset`, and `word-search`) exceeded their timeout thresholds in GHC 9.2 and GHC 9.4. No regressions were observed with GHC 9.6. To accommodate the lowest common denominator, I bumped the thresholds for those three scenarios to get a passing build. I don't believe it is worth further effort or investigation to optimize for GHC 9.4, since such efforts will be moot for GHC 9.6 and onward. Test invocation: cabal test swarm-integration --test-show-details streaming --test-options '--color always --pattern "word-search"' | **Scenario** | GHC 9.4.8 | GHC 9.4.8 | GHC 9.6.4 | GHC 9.6.4 | | --- | --- | --- | --- | --- | | | Before | **After** | Before | **After** | `snake` | 1.84s | **5.38s** | 1.62s | **1.67s** | | `powerset` | 1.66s | **5.09s** | 1.56s | **1.66s** | | `word-search` | 0.56s | **1.91s** | 0.44s | **0.48s** | ### Potential improvements #### `TickNumber` as `Map` key `waitingRobots` is of type `Map TickNumber [RID]`. Instead of `Map`, we could have a `newtype` that encapsulates a `IntMap` to make lookups by `TickNumber` more efficient. --- .../Challenges/Sokoban/_foresight/solution.sw | 2 +- .../Challenges/_combo-lock/solution.sw | 1 - data/scenarios/Testing/00-ORDER.txt | 2 + .../Testing/1322-wait-with-instant.yaml | 72 ++++++++++++++ .../Testing/1598-detect-entity-change.yaml | 94 +++++++++++++++++++ .../_1598-detect-entity-change/setup.sw | 17 ++++ scripts/benchmark-against-ancestor.sh | 25 +++++ scripts/benchmark-against-parent.sh | 19 +--- src/swarm-engine/Swarm/Game/State/Robot.hs | 69 ++++++++++---- src/swarm-engine/Swarm/Game/Step.hs | 82 +++++++++++++--- src/swarm-engine/Swarm/Game/Step/Util.hs | 5 +- .../Swarm/Game/Step/Util/Command.hs | 9 +- test/integration/Main.hs | 8 +- 13 files changed, 346 insertions(+), 59 deletions(-) create mode 100644 data/scenarios/Testing/1322-wait-with-instant.yaml create mode 100644 data/scenarios/Testing/1598-detect-entity-change.yaml create mode 100644 data/scenarios/Testing/_1598-detect-entity-change/setup.sw create mode 100755 scripts/benchmark-against-ancestor.sh diff --git a/data/scenarios/Challenges/Sokoban/_foresight/solution.sw b/data/scenarios/Challenges/Sokoban/_foresight/solution.sw index 9f167392d..fd00deb37 100644 --- a/data/scenarios/Challenges/Sokoban/_foresight/solution.sw +++ b/data/scenarios/Challenges/Sokoban/_foresight/solution.sw @@ -139,7 +139,7 @@ def firstLeg = pushUntilBarrier; wait 4; - move; + moveUntilBlocked; doN 5 (turn left; moveUntilBlocked); turn right; diff --git a/data/scenarios/Challenges/_combo-lock/solution.sw b/data/scenarios/Challenges/_combo-lock/solution.sw index a51af436a..3483ff327 100644 --- a/data/scenarios/Challenges/_combo-lock/solution.sw +++ b/data/scenarios/Challenges/_combo-lock/solution.sw @@ -4,7 +4,6 @@ def moveToLock = end; def cycleCombos = \n. - wait 1; entityNorth <- scan north; let hasGate = case entityNorth (\_. false) (\x. x == "gate") in if hasGate { diff --git a/data/scenarios/Testing/00-ORDER.txt b/data/scenarios/Testing/00-ORDER.txt index 0ca5d39a6..31aa77126 100644 --- a/data/scenarios/Testing/00-ORDER.txt +++ b/data/scenarios/Testing/00-ORDER.txt @@ -46,6 +46,8 @@ Achievements 1341-command-count.yaml 1355-combustion.yaml 1379-single-world-portal-reorientation.yaml +1322-wait-with-instant.yaml +1598-detect-entity-change.yaml 1399-backup-command.yaml 1430-built-robot-ownership.yaml 1536-custom-unwalkable-entities.yaml diff --git a/data/scenarios/Testing/1322-wait-with-instant.yaml b/data/scenarios/Testing/1322-wait-with-instant.yaml new file mode 100644 index 000000000..17309e73c --- /dev/null +++ b/data/scenarios/Testing/1322-wait-with-instant.yaml @@ -0,0 +1,72 @@ +version: 1 +name: Using wait with instant +author: Karl Ostmo +description: | + Observe timing of (instant $ wait 1) + interspersed with other commands +creative: false +seed: 0 +objectives: + - goal: + - | + Hare must win by three cells + condition: | + h <- robotnamed "hare"; + hareloc <- as h {whereami}; + + t <- robotnamed "tortoise"; + tortoiseloc <- as t {whereami}; + + let xDiff = fst hareloc - fst tortoiseloc in + + return $ fst hareloc == 0 && xDiff == 3; +solution: | + noop; +robots: + - name: base + dir: [1, 0] + display: + invisible: true + devices: + - hourglass + - logger + - name: tortoise + system: true + display: + invisible: false + attr: green + dir: [1, 0] + program: | + move; move; + move; move; + move; move; + - name: hare + system: true + display: + invisible: false + attr: snow + dir: [1, 0] + program: | + instant ( + move; move; + wait 1; + move; move; + wait 1; + move; move; + ); +world: + dsl: | + {blank} + upperleft: [-6, 2] + offset: false + palette: + '.': [grass, erase] + 'd': [dirt, erase] + 'B': [grass, erase, base] + 'T': [grass, erase, tortoise] + 'H': [grass, erase, hare] + map: | + B.....d. + T.....d. + H.....d. + ......d. diff --git a/data/scenarios/Testing/1598-detect-entity-change.yaml b/data/scenarios/Testing/1598-detect-entity-change.yaml new file mode 100644 index 000000000..604a48b8c --- /dev/null +++ b/data/scenarios/Testing/1598-detect-entity-change.yaml @@ -0,0 +1,94 @@ +version: 1 +name: Entity change detection +author: Karl Ostmo +description: | + Ensure that a change to an entity can be observed + by a system robot within a single tick. + + In this scenario, the base will first `swap` the + existing `dial (R)`{=entity} with a `dial (G)`{=entity}, + then immediately `swap` again with a `dial (B)`{=entity}. + + The system robot should be able to detect the presence + of the `dial (G)`{=entity} before it is `swap`ped a second time. +creative: false +seed: 0 +objectives: + - goal: + - | + Turn the light green + condition: | + as base {has "flower"}; + prerequisite: + not: blue_light + - id: blue_light + teaser: No blue light + optional: true + goal: + - | + Turn the light blue + condition: | + r <- robotnamed "lockbot"; + as r {ishere "dial (B)"}; +robots: + - name: base + dir: [1, 0] + display: + invisible: true + devices: + - hourglass + - fast grabber + - logger + - treads + inventory: + - [1, "dial (R)"] + - [1, "dial (G)"] + - [1, "dial (B)"] + - name: lockbot + system: true + display: + invisible: true + dir: [1, 0] + program: | + run "scenarios/Testing/_1598-detect-entity-change/setup.sw" + inventory: + - [1, flower] +solution: | + wait 2; + move; + move; + swap "dial (G)"; + swap "dial (B)"; +entities: + - name: "dial (R)" + display: + char: '•' + attr: red + description: + - A red dial + properties: [known, pickable] + - name: "dial (G)" + display: + char: '•' + attr: green + description: + - A green dial + properties: [known, pickable] + - name: "dial (B)" + display: + char: '•' + attr: blue + description: + - A blue dial + properties: [known, pickable] +world: + dsl: | + {blank} + upperleft: [-1, -1] + offset: false + palette: + '.': [grass, erase] + 'B': [grass, erase, base] + 'c': [grass, dial (R), lockbot] + map: | + B.c diff --git a/data/scenarios/Testing/_1598-detect-entity-change/setup.sw b/data/scenarios/Testing/_1598-detect-entity-change/setup.sw new file mode 100644 index 000000000..21b1aa3f6 --- /dev/null +++ b/data/scenarios/Testing/_1598-detect-entity-change/setup.sw @@ -0,0 +1,17 @@ +def doUntilCorrect = + herenow <- ishere "dial (G)"; + if herenow { + give base "flower"; + } { + watch down; + wait 1000; + doUntilCorrect; + } + end; + +def go = + instant $ + doUntilCorrect; + end; + +go; diff --git a/scripts/benchmark-against-ancestor.sh b/scripts/benchmark-against-ancestor.sh new file mode 100755 index 000000000..12fb8a6b8 --- /dev/null +++ b/scripts/benchmark-against-ancestor.sh @@ -0,0 +1,25 @@ +#!/bin/bash -xe + +# Requires that the working tree be clean. + +REFERENCE_COMMIT=${1:-HEAD~} + +SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) +cd $SCRIPT_DIR/.. + +if git diff --quiet --exit-code +then + echo "Working tree is clean. Starting benchmarks..." +else + echo "Working tree is dirty! Quitting." + exit 1 +fi + +BASELINE_OUTPUT=baseline.csv + +git checkout $REFERENCE_COMMIT + +scripts/run-benchmarks.sh "--csv $BASELINE_OUTPUT" + +git switch - +scripts/run-benchmarks.sh "--baseline $BASELINE_OUTPUT --fail-if-slower 3" \ No newline at end of file diff --git a/scripts/benchmark-against-parent.sh b/scripts/benchmark-against-parent.sh index d530d1155..2a9a63a3a 100755 --- a/scripts/benchmark-against-parent.sh +++ b/scripts/benchmark-against-parent.sh @@ -1,23 +1,6 @@ #!/bin/bash -xe -# Requires that the working tree be clean. - SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) cd $SCRIPT_DIR/.. -if git diff --quiet --exit-code -then - echo "Working tree is clean. Starting benchmarks..." -else - echo "Working tree is dirty! Quitting." - exit 1 -fi - -BASELINE_OUTPUT=baseline.csv - -git checkout HEAD~ - -scripts/run-benchmarks.sh "--csv $BASELINE_OUTPUT" - -git switch - -scripts/run-benchmarks.sh "--baseline $BASELINE_OUTPUT --fail-if-slower 3" \ No newline at end of file +scripts/benchmark-against-ancestor.sh HEAD~ \ No newline at end of file diff --git a/src/swarm-engine/Swarm/Game/State/Robot.hs b/src/swarm-engine/Swarm/Game/State/Robot.hs index 590e2ea7c..d066b3d8e 100644 --- a/src/swarm-engine/Swarm/Game/State/Robot.hs +++ b/src/swarm-engine/Swarm/Game/State/Robot.hs @@ -25,6 +25,7 @@ module Swarm.Game.State.Robot ( robotsWatching, activeRobots, waitingRobots, + currentTickWakeableBots, viewCenterRule, viewCenter, focusedRobotID, @@ -60,6 +61,7 @@ import Data.IntMap qualified as IM import Data.IntSet (IntSet) import Data.IntSet qualified as IS import Data.IntSet.Lens (setOf) +import Data.List (partition) import Data.List.NonEmpty qualified as NE import Data.Map (Map) import Data.Map qualified as M @@ -120,11 +122,12 @@ data Robots = Robots -- Waiting robots for a given time are a list because it is cheaper to -- prepend to a list than insert into a 'Set'. _waitingRobots :: Map TickNumber [RID] + , _currentTickWakeableBots :: [RID] , _robotsByLocation :: Map SubworldName (Map Location IntSet) , -- This member exists as an optimization so -- that we do not have to iterate over all "waiting" robots, -- since there may be many. - _robotsWatching :: Map (Cosmic Location) (S.Set RID) + _robotsWatching :: Map (Cosmic Location) IntSet , _robotNaming :: RobotNaming , _viewCenterRule :: ViewCenterRule , _viewCenter :: Cosmic Location @@ -154,6 +157,9 @@ activeRobots = internalActiveRobots waitingRobots :: Getter Robots (Map TickNumber [RID]) waitingRobots = internalWaitingRobots +-- | Get a list of all the robots that are \"watching\" by location. +currentTickWakeableBots :: Lens' Robots [RID] + -- | The names of all robots that currently exist in the game, indexed by -- location (which we need both for /e.g./ the @salvage@ command as -- well as for actually drawing the world). Unfortunately there is @@ -166,7 +172,7 @@ waitingRobots = internalWaitingRobots robotsByLocation :: Lens' Robots (Map SubworldName (Map Location IntSet)) -- | Get a list of all the robots that are \"watching\" by location. -robotsWatching :: Lens' Robots (Map (Cosmic Location) (S.Set RID)) +robotsWatching :: Lens' Robots (Map (Cosmic Location) IntSet) -- | State and data for assigning identifiers to robots robotNaming :: Lens' Robots RobotNaming @@ -196,6 +202,7 @@ initRobots gsc = { _robotMap = IM.empty , _activeRobots = IS.empty , _waitingRobots = M.empty + , _currentTickWakeableBots = mempty , _robotsByLocation = M.empty , _robotsWatching = mempty , _robotNaming = @@ -294,6 +301,15 @@ activateRobot rid = internalActiveRobots %= IS.insert rid -- | Removes robots whose wake up time matches the current game ticks count -- from the 'waitingRobots' queue and put them back in the 'activeRobots' set -- if they still exist in the keys of 'robotMap'. +-- +-- = Mutations +-- +-- This function modifies: +-- +-- * 'wakeLog' +-- * 'robotsWatching' +-- * 'internalWaitingRobots' +-- * 'internalActiveRobots' (aka 'activeRobots') wakeUpRobotsDoneSleeping :: (Has (State Robots) sig m) => TickNumber -> m () wakeUpRobotsDoneSleeping time = do mrids <- internalWaitingRobots . at time <<.= Nothing @@ -301,29 +317,34 @@ wakeUpRobotsDoneSleeping time = do Nothing -> return () Just rids -> do robots <- use robotMap - let aliveRids = filter (`IM.member` robots) rids - internalActiveRobots %= IS.union (IS.fromList aliveRids) + let robotIdSet = IM.keysSet robots + wakeableRIDsSet = IS.fromList rids + + -- Limit ourselves to the robots that have not expired in their sleep + newlyAlive = IS.intersection robotIdSet wakeableRIDsSet + + internalActiveRobots %= IS.union newlyAlive -- These robots' wake times may have been moved "forward" -- by 'wakeWatchingRobots'. - clearWatchingRobots rids + clearWatchingRobots wakeableRIDsSet -- | Clear the "watch" state of all of the -- awakened robots clearWatchingRobots :: (Has (State Robots) sig m) => - [RID] -> + IntSet -> m () clearWatchingRobots rids = do - robotsWatching %= M.map (`S.difference` S.fromList rids) + robotsWatching %= M.map (`IS.difference` rids) -- | Iterates through all of the currently @wait@-ing robots, -- and moves forward the wake time of the ones that are @watch@-ing this location. -- -- NOTE: Clearing 'TickNumber' map entries from 'internalWaitingRobots' -- upon wakeup is handled by 'wakeUpRobotsDoneSleeping' -wakeWatchingRobots :: (Has (State Robots) sig m) => TickNumber -> Cosmic Location -> m () -wakeWatchingRobots currentTick loc = do +wakeWatchingRobots :: (Has (State Robots) sig m) => RID -> TickNumber -> Cosmic Location -> m () +wakeWatchingRobots myID currentTick loc = do waitingMap <- use waitingRobots rMap <- use robotMap watchingMap <- use robotsWatching @@ -335,7 +356,7 @@ wakeWatchingRobots currentTick loc = do botsWatchingThisLoc :: [Robot] botsWatchingThisLoc = mapMaybe (`IM.lookup` rMap) $ - S.toList $ + IS.toList $ M.findWithDefault mempty loc watchingMap -- Step 2: Get the target wake time for each of these robots @@ -356,10 +377,23 @@ wakeWatchingRobots currentTick loc = do -- when their tick comes up in "wakeUpRobotsDoneSleeping". f (k, botsToRemove) = M.adjust (filter (`S.notMember` botsToRemove)) k - -- Step 4: Re-add the watching bots to be awakened at the next tick: + -- Step 4: Re-add the watching bots to be awakened ASAP: wakeableBotIds = map fst wakeTimes - newWakeTime = addTicks 1 currentTick - newInsertions = M.singleton newWakeTime wakeableBotIds + + -- It is crucial that only robots with a larger RID than the current robot + -- be scheduled for the *same* tick, since within a given tick we iterate over + -- robots in increasing order of RID. + -- See note in 'iterateRobots'. + (currTickWakeable, nextTickWakeable) = partition (> myID) wakeableBotIds + wakeTimeGroups = + [ (currentTick, currTickWakeable) + , (addTicks 1 currentTick, nextTickWakeable) + ] + newInsertions = M.filter (not . null) $ M.fromList wakeTimeGroups + + -- Contract: This must be emptied immediately + -- in 'iterateRobots' + currentTickWakeableBots .= currTickWakeable -- NOTE: There are two "sources of truth" for the waiting state of robots: -- 1. In the GameState via "internalWaitingRobots" @@ -369,10 +403,11 @@ wakeWatchingRobots currentTick loc = do internalWaitingRobots .= M.unionWith (<>) filteredWaiting newInsertions -- 2. Update the machine of each robot - forM_ wakeableBotIds $ \rid -> - robotMap . at rid . _Just . machine %= \case - Waiting _ c -> Waiting newWakeTime c - x -> x + forM_ wakeTimeGroups $ \(newWakeTime, wakeableBots) -> + forM_ wakeableBots $ \rid -> + robotMap . at rid . _Just . machine %= \case + Waiting _ c -> Waiting newWakeTime c + x -> x deleteRobot :: (Has (State Robots) sig m) => RID -> m () deleteRobot rn = do diff --git a/src/swarm-engine/Swarm/Game/Step.hs b/src/swarm-engine/Swarm/Game/Step.hs index b645696aa..ed3c1f462 100644 --- a/src/swarm-engine/Swarm/Game/Step.hs +++ b/src/swarm-engine/Swarm/Game/Step.hs @@ -84,7 +84,7 @@ import Prelude hiding (Applicative (..), lookup) -- -- Note that the game may be in 'RobotStep' mode and not finish -- the tick. Use the return value to check whether a full tick happened. -gameTick :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => m Bool +gameTick :: HasGameStepState sig m => m Bool gameTick = do time <- use $ temporal . ticks zoomRobots $ wakeUpRobotsDoneSleeping time @@ -133,14 +133,14 @@ gameTick = do -- | Finish a game tick in progress and set the game to 'WorldTick' mode afterwards. -- -- Use this function if you need to unpause the game. -finishGameTick :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => m () +finishGameTick :: HasGameStepState sig m => m () finishGameTick = use (temporal . gameStep) >>= \case WorldTick -> pure () RobotStep SBefore -> temporal . gameStep .= WorldTick RobotStep _ -> void gameTick >> finishGameTick --- Insert the robot back to robot map. +-- | Insert the robot back to robot map. -- Will selfdestruct or put the robot to sleep if it has that set. insertBackRobot :: Has (State GameState) sig m => RID -> Robot -> m () insertBackRobot rn rob = do @@ -158,16 +158,72 @@ insertBackRobot rn rob = do Nothing -> unless (isActive rob) (sleepForever rn) --- Run a set of robots - this is used to run robots before/after the focused one. -runRobotIDs :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => IS.IntSet -> m () -runRobotIDs robotNames = forM_ (IS.toList robotNames) $ \rn -> do - mr <- uses (robotInfo . robotMap) (IM.lookup rn) - forM_ mr (stepOneRobot rn) +-- | GameState with support for IO and Time effect +type HasGameStepState sig m = (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) + +-- | Run a set of robots - this is used to run robots before/after the focused one. +-- +-- Note that during the iteration over the supplied robot IDs, it is possible +-- that a robot that may have been present in 'robotMap' at the outset +-- of the iteration to be removed before the iteration comes upon it. +-- This is why we must perform a 'robotMap' lookup at each iteration, rather +-- than looking up elements from 'robotMap' in bulk up front with something like +-- 'restrictKeys'. +-- +-- = Invariants +-- +-- * Every tick, every active robot shall have exactly one opportunity to run. +-- * The sequence in which robots are chosen to run is by increasing order of 'RID'. +runRobotIDs :: HasGameStepState sig m => IS.IntSet -> m () +runRobotIDs robotNames = do + time <- use $ temporal . ticks + flip (iterateRobots time) robotNames $ \rn -> do + mr <- uses (robotInfo . robotMap) (IM.lookup rn) + forM_ mr (stepOneRobot rn) where + stepOneRobot :: HasGameStepState sig m => RID -> Robot -> m () stepOneRobot rn rob = tickRobot rob >>= insertBackRobot rn --- This is a helper function to do one robot step or run robots before/after. -singleStep :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => SingleStep -> RID -> IS.IntSet -> m Bool +-- | +-- Runs the given robots in increasing order of 'RID'. +-- +-- Running a given robot _may_ cause another robot +-- with a higher 'RID' to be inserted into the runnable set. +-- +-- Note that the behavior we desire is described precisely by a +-- . +-- +-- A priority queue allows O(1) access to the lowest priority item. However, +-- /splitting/ the min item from rest of the queue is still an O(log N) operation, +-- and therefore is not any better than the 'minView' function from 'IntSet'. +-- +-- Tail-recursive. +iterateRobots :: HasGameStepState sig m => TickNumber -> (RID -> m ()) -> IS.IntSet -> m () +iterateRobots time f runnableBots = + forM_ (IS.minView runnableBots) $ \(thisRobotId, remainingBotIDs) -> do + f thisRobotId + + -- We may have awakened new robots in the current robot's iteration, + -- so we add them to the list + poolAugmentation <- do + -- NOTE: We could use 'IS.split thisRobotId activeRIDsThisTick' + -- to ensure that we only insert RIDs greater than 'thisRobotId' + -- into the queue. + -- However, we already ensure in 'wakeWatchingRobots' that only + -- robots with a larger RID are scheduled for the current tick; + -- robots with smaller RIDs will be scheduled for the next tick. + robotsToAdd <- use $ robotInfo . currentTickWakeableBots + if null robotsToAdd + then return id + else do + zoomRobots $ wakeUpRobotsDoneSleeping time + robotInfo . currentTickWakeableBots .= [] + return $ IS.union $ IS.fromList robotsToAdd + + iterateRobots time f $ poolAugmentation remainingBotIDs + +-- | This is a helper function to do one robot step or run robots before/after. +singleStep :: HasGameStepState sig m => SingleStep -> RID -> IS.IntSet -> m Bool singleStep ss focRID robotSet = do let (preFoc, focusedActive, postFoc) = IS.splitMember focRID robotSet case ss of @@ -424,7 +480,7 @@ traceLogShow = void . traceLog Logged Info . from . show -- | Run a robot for one tick, which may consist of up to -- 'robotStepsPerTick' CESK machine steps and at most one tangible -- command execution, whichever comes first. -tickRobot :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => Robot -> m Robot +tickRobot :: HasGameStepState sig m => Robot -> m Robot tickRobot r = do steps <- use $ temporal . robotStepsPerTick tickRobotRec (r & activityCounts . tickStepBudget .~ steps) @@ -433,7 +489,7 @@ tickRobot r = do -- robot is actively running and still has steps left, and if so -- runs it for one step, then calls itself recursively to continue -- stepping the robot. -tickRobotRec :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => Robot -> m Robot +tickRobotRec :: HasGameStepState sig m => Robot -> m Robot tickRobotRec r = do time <- use $ temporal . ticks case wantsToStep time r && (r ^. runningAtomic || r ^. activityCounts . tickStepBudget > 0) of @@ -442,7 +498,7 @@ tickRobotRec r = do -- | Single-step a robot by decrementing its 'tickStepBudget' counter and -- running its CESK machine for one step. -stepRobot :: (Has (State GameState) sig m, Has (Lift IO) sig m, Has Effect.Time sig m) => Robot -> m Robot +stepRobot :: HasGameStepState sig m => Robot -> m Robot stepRobot r = do (r', cesk') <- runState (r & activityCounts . tickStepBudget -~ 1) (stepCESK (r ^. machine)) -- sendIO $ appendFile "out.txt" (prettyString cesk' ++ "\n") diff --git a/src/swarm-engine/Swarm/Game/Step/Util.hs b/src/swarm-engine/Swarm/Game/Step/Util.hs index e33fb4b32..aba1d0f33 100644 --- a/src/swarm-engine/Swarm/Game/Step/Util.hs +++ b/src/swarm-engine/Swarm/Game/Step/Util.hs @@ -60,7 +60,7 @@ lookInDirection d = do -- | Modify the entity (if any) at a given location. updateEntityAt :: - (Has (State GameState) sig m) => + (Has (State Robot) sig m, Has (State GameState) sig m) => Cosmic Location -> (Maybe Entity -> Maybe Entity) -> m () @@ -71,7 +71,8 @@ updateEntityAt cLoc@(Cosmic subworldName loc) upd = do forM_ (WM.getModification =<< someChange) $ \modType -> do currentTick <- use $ temporal . ticks - zoomRobots $ wakeWatchingRobots currentTick cLoc + myID <- use robotID + zoomRobots $ wakeWatchingRobots myID currentTick cLoc SRT.entityModified modType cLoc pcr <- use $ pathCaching . pathCachingRobots diff --git a/src/swarm-engine/Swarm/Game/Step/Util/Command.hs b/src/swarm-engine/Swarm/Game/Step/Util/Command.hs index 7e267af87..8c9bbae9f 100644 --- a/src/swarm-engine/Swarm/Game/Step/Util/Command.hs +++ b/src/swarm-engine/Swarm/Game/Step/Util/Command.hs @@ -20,6 +20,7 @@ import Control.Effect.Lens import Control.Effect.Lift import Control.Lens as Lens hiding (Const, distrib, from, parts, use, uses, view, (%=), (+=), (.=), (<+=), (<>=)) import Control.Monad (forM_, unless, when) +import Data.IntSet qualified as IS import Data.Map qualified as M import Data.Sequence qualified as Seq import Data.Set (Set) @@ -89,10 +90,10 @@ purgeFarAwayWatches = do let isNearby = isNearbyOrExempt privileged myLoc f loc = if not $ isNearby loc - then S.delete rid + then IS.delete rid else id - robotInfo . robotsWatching %= M.filter (not . null) . M.mapWithKey f + robotInfo . robotsWatching %= M.filter (not . IS.null) . M.mapWithKey f verbedGrabbingCmd :: GrabbingCmd -> Text verbedGrabbingCmd = \case @@ -272,7 +273,7 @@ addWatchedLocation :: m () addWatchedLocation loc = do rid <- use robotID - robotInfo . robotsWatching %= M.insertWith (<>) loc (S.singleton rid) + robotInfo . robotsWatching %= M.insertWith (<>) loc (IS.singleton rid) -- | Give some entities from a parent robot (the robot represented by -- the ambient @State Robot@ effect) to a child robot (represented @@ -371,7 +372,7 @@ createLogEntry source sev msg = do -- | replace some entity in the world with another entity updateWorld :: - (Has (State GameState) sig m, Has (Throw Exn) sig m) => + HasRobotStepState sig m => Const -> WorldUpdate Entity -> m () diff --git a/test/integration/Main.hs b/test/integration/Main.hs index 1951a4649..9702276a6 100644 --- a/test/integration/Main.hs +++ b/test/integration/Main.hs @@ -226,7 +226,7 @@ testScenarioSolutions rs ui = ] , testGroup "Fun" - [ testSolution (Sec 10) "Fun/snake" + [ testSolution (Sec 20) "Fun/snake" ] , testGroup "Challenges" @@ -234,7 +234,7 @@ testScenarioSolutions rs ui = , testSolution Default "Challenges/teleport" , testSolution Default "Challenges/maypole" , testSolution (Sec 5) "Challenges/2048" - , testSolution (Sec 3) "Challenges/word-search" + , testSolution (Sec 6) "Challenges/word-search" , testSolution (Sec 10) "Challenges/bridge-building" , testSolution (Sec 5) "Challenges/ice-cream" , testSolution (Sec 10) "Challenges/combo-lock" @@ -261,7 +261,7 @@ testScenarioSolutions rs ui = "Ranching" [ testSolution Default "Challenges/Ranching/capture" , testSolution (Sec 60) "Challenges/Ranching/beekeeping" - , testSolution (Sec 10) "Challenges/Ranching/powerset" + , testSolution (Sec 20) "Challenges/Ranching/powerset" , testSolution (Sec 10) "Challenges/Ranching/fishing" , testSolution (Sec 30) "Challenges/Ranching/gated-paddock" ] @@ -363,6 +363,8 @@ testScenarioSolutions rs ui = , testSolution Default "Testing/144-subworlds/subworld-located-robots" , testSolution Default "Testing/1355-combustion" , testSolution Default "Testing/1379-single-world-portal-reorientation" + , testSolution Default "Testing/1322-wait-with-instant" + , testSolution Default "Testing/1598-detect-entity-change" , testSolution Default "Testing/1399-backup-command" , testSolution Default "Testing/1536-custom-unwalkable-entities" , testSolution Default "Testing/1721-custom-walkable-entities"