Demo crashes when entering the tunnel

[ll-nick]

I just saw the demo crash when Pac-Man tried entering the tunnel during the ChaseGhost behavior. It would probably not have crashed with #21 but that's something we should investigate nevertheless.

[orzechow]

I could reproduce an exception thrown by AvoidGhostBehavior by forcing PacMan into the tunnel by overriding the command manually 🕹️ 😁

diff --git a/demo/include/utils/pacman_wrapper.hpp b/demo/include/utils/pacman_wrapper.hpp
index 9380637..0522eaf 100644
--- a/demo/include/utils/pacman_wrapper.hpp
+++ b/demo/include/utils/pacman_wrapper.hpp
@@ -30,7 +30,7 @@ public:
     void printKeybindings();
 
 private:
-    void handleUserInput();
+    std::optional<SDL_Scancode> handleUserInput();
     void renderPath(const demo::Positions& path);
 
     int scaleFactor_;
diff --git a/demo/src/pacman_wrapper.cpp b/demo/src/pacman_wrapper.cpp
index 3428afc..665d847 100644
--- a/demo/src/pacman_wrapper.cpp
+++ b/demo/src/pacman_wrapper.cpp
@@ -54,7 +54,9 @@ PacmanWrapper::PacmanWrapper()
     SDL_SetRenderDrawBlendMode(renderer_.get(), SDL_BLENDMODE_BLEND);
 }
 
-void PacmanWrapper::handleUserInput() {
+std::optional<SDL_Scancode> PacmanWrapper::handleUserInput() {
+    std::optional<SDL_Scancode> commandOverride;
+
     SDL_Event event;
     while (SDL_PollEvent(&event) != 0) {
         if (event.type == SDL_QUIT) {
@@ -75,15 +77,26 @@ void PacmanWrapper::handleUserInput() {
                 renderPath_ = !renderPath_;
                 break;
             }
+            if (event.key.keysym.scancode == SDL_SCANCODE_UP || event.key.keysym.scancode == SDL_SCANCODE_DOWN ||
+                event.key.keysym.scancode == SDL_SCANCODE_LEFT || event.key.keysym.scancode == SDL_SCANCODE_RIGHT) {
+                commandOverride = event.key.keysym.scancode;
+                break;
+            }
         }
     }
+
+    return commandOverride;
 }
 
 void PacmanWrapper::progressGame(const demo::Command& command,
                                  const demo::EnvironmentModel::ConstPtr& environmentModel) {
-    handleUserInput();
+    std::optional<SDL_Scancode> commandOverride = handleUserInput();
 
-    game_.input(command.scancode());
+    if (commandOverride) {
+        game_.input(commandOverride.value());
+    } else {
+        game_.input(command.scancode());
+    }
 
     if (pause_) {
         return;

[orzechow]

The exception is thrown by an out of bounds access to the Maze (Maze::isWall)
https://github.com/KIT-MRT/EnTT-Pacman/blob/master/src/util/grid.hpp#L61
with position = {x:19, y:9} and mazeState_.size = {x:19, y:22}

Stacktrace:

Grid<Tile>::operator[](Pos) const (/usr/local/include/pacman/util/grid.hpp:61)
utils::Maze::at(demo::Position const&) const (/workspaces/demo/include/utils/maze.hpp:40)
utils::Maze::isWall(demo::Position const&) const (/workspaces/demo/include/utils/maze.hpp:66)
demo::EnvironmentModel::isWall(demo::Position const&) const (/workspaces/demo/include/demo/environment_model.hpp:109)
demo::AvoidGhostBehavior::getCommand(std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) (/workspaces/demo/src/avoid_ghost_behavior.cpp:15)
arbitration_graphs::Arbitrator<…>::Option::getCommand(std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) const (/usr/include/arbitration_graphs/arbitrator.hpp:70)
arbitration_graphs::Arbitrator<…>::getAndVerifyCommand(std::shared_ptr<arbitration_graphs::Arbitrator<demo::Command, demo::Command, demo::Verifier, demo::VerificationResult>::Option> const&, std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) const (/usr/include/arbitration_graphs/internal/arbitrator_impl.hpp:54)
arbitration_graphs::Arbitrator<…>::getAndVerifyCommandFromApplicable(std::vector<std::shared_ptr<arbitration_graphs::Arbitrator<demo::Command, demo::Command, demo::Verifier, demo::VerificationResult>::Option>, std::allocator<std::shared_ptr<arbitration_graphs::Arbitrator<demo::Command, demo::Command, demo::Verifier, demo::VerificationResult>::Option>>> const&, std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) (/usr/include/arbitration_graphs/internal/arbitrator_impl.hpp:115)
arbitration_graphs::Arbitrator<…>::getCommand(std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) (/usr/include/arbitration_graphs/arbitrator.hpp:131)
demo::PacmanAgent::getCommand(std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<double, std::ratio<1l, 1l>>> const&) (/workspaces/demo/include/demo/pacman_agent.hpp:67)
main (/workspaces/demo/src/main.cpp:29)

Looks like positionConsideringTunnel() doesn't do a correct job here…
So, positionConsideringTunnel() returns the unwrapped position, if the wrapped position points to a wall.

@ll-nick is this intentionally?
AFAIK all uses of positionConsideringTunnel() assume that the position is always wrapped, no matter the new position type. That's why this part crashes for AvoidGhost and ChaseGhost if the position is not wrapped.

  auto nextPosition = environmentModel_->positionConsideringTunnel(pacmanPosition + move.deltaPosition);

→ if (environmentModel_->isWall(nextPosition)) {
      continue;
  }

[ll-nick]

Nice investigative work 🕵️

Present-me thinks that the implementation of past-me makes absolutely no sense.

You're right, the wrapped position should be returned no matter the type. The question is: Why is the wrapped position a wall? Did that make sense in the situation you described?

And one other question: Should we implement the manual override into the demo? Might be handy in other situations too. 😀

[orzechow]

After a brief thought: clamping the position for non-tunnel positions does the job.

See

• dd1903a for the new, failing unit test
• 54d36d1 for the fix

[orzechow]

You're right, the wrapped position should be returned no matter the type. The question is: Why is the wrapped position a wall? Did that make sense in the situation you described?

That's a good question!

And yes, it makes sense:

ChaseGhost/AvoidGhost

• takes the (unwrapped) pacmanPosition = {x:19, y:10} (a tunnel) and
• tries all possibleMoves() → pacmanPosition + move.deltaPosition
• thus, one of the moves it checks, is going UP after moving through the tunnel ({x:19, y:9})

[ll-nick]

Ah yes, that makes sense. Thanks for the fix, I'll take a look now 👌