Auroriax / PuzzleScriptPlus

Puzzlescript Plus: Open Source HTML5 Puzzle Game Engine (with lots of extra functionality)
https://auroriax.github.io/PuzzleScript/
35 stars 5 forks source link

mobile + mouse + undo menu crash (getTilesTraversingPoints infinite loop) #88

Closed pancelor closed 1 year ago

pancelor commented 1 year ago

Repro steps to get PS+ into an infinite loop inside getTilesTraversingPoints:

  1. open a game with mouse controls in desktop firefox (e.g. export this minimal example and open the exported HTML file)
  2. open the devtools, set the browser to pretend to be a mobile device
  3. reload the page, the mobile undo/restart/quit pullout menu should appear
  4. pull out the pullout menu and to play the game, toggling the pullout menu a few times and interacting with the game while the pullout menu is open. after a few clicks the game will freeze up the browser

A picture is worth 1000 words: clicking at the green circle sends input to both the pullout menu and also the game, and results in an infinite loop:

bug


35 is related, and the root cause is similar:

function setMouseCoord(e){
 var coords = canvas.relMouseCoords(e);
 if (isNaN(coords.x) || isNaN(coords.y)) {
  console.warn("[SetMouseCoord] Did not recieve valid mouse coords from event. Ignoring it (since I'm assuming this is a faked keypress that was generated on mobile).")
 }
    mousePixelX=coords.x-xoffset; // line A
 mousePixelY=coords.y-yoffset;
 mouseCoordX=Math.floor(mousePixelX/cellwidth);
 mouseCoordY=Math.floor(mousePixelY/cellheight);
}

...
// in mouseAction:
  x1 = x2;  // line C
  y1 = y2;
  x2 = mousePixelX; // line B
  y2 = mousePixelY;

...
// in mouseAction:
   var tileLists = getTilesTraversingPoints(x1, y1, x2, y2); // line D

When coords.x is NaN and the console.warn fires, coords.x is nan, so mousePixelX is nan (line A), so x2 is nan (line B), then on a later click(?) x1 is nan (line C). Then something about getTilesTraversingPoints(NaN,NaN,42,42) (line D) causes a infinite loop. probably because while(cellX1 !== cellX2 is always true because cellX1 is nan? (I didn't look into this too closely). (note: my explanation here is not quite right, because I added some console.logs and never noticed x2 being nan inside getTilesTraversingPoints -- only x1 and y1 were nan)


I think that fixing this should probably involve making the pullout menu "consume" input events and not pass them down into the game? Some sort of refactor that makes it impossible for inputs to affect both the pullout menu and the game canvas at the same time.

I have a quick fix in the interim; I'll make a PR shortly

pancelor commented 1 year ago

(if the history looks odd it's because I accidentally referenced the original repo's issue number instead of this repo; I force pushed an amended version of the commit. hopefully that doesn't cause problems)

Auroriax commented 1 year ago

Aah, I thought I found all of the infinite loop edge cases, but this is one case that I overlooked. Thanks for the report! I agree that a proper "consume" fix would be better, but I don't know when I'll have time to look into this, so I'll happily take your fix in the meanwhile.