Dallinger / Griduniverse

Welcome to the Griduniverse.
7 stars 3 forks source link

Adding continuous motion #107

Closed vlall closed 7 years ago

vlall commented 7 years ago

Issue #105 asks for a default behavior where avatar motion continues during 'keydown' arrow strokes.

TODO:

  1. What was the lock used for in the mouse-binding? I removed it because It prevents a keydown call because it waits for the keyup @jessesnyder https://github.com/Dallinger/Griduniverse/commit/21cfa616a209a6ecf70164ca3a5c0e074a2e0739

  2. Test this on bots

suchow commented 7 years ago

@vlall I think I originally added the lock mechanism as a way to prevent the situation where holding down the key triggers multiple move events.

suchow commented 7 years ago

I tested this by moving around with a single player:

  1. When I make a short keypress in any direction, the players moves 1 block in the requested direction.
  2. When I hold down a key, the player doesn't really move much at first but then moves a ton in the requested direction, even after I release the key. If I hold down the key for 10 seconds, the player gets stuck at the edge and can never move away from it.
  3. If I turn key repeat off on my keyboard settings, the motion returns to how it was before this PR.
  4. If I turn the key repeat to its slowest level on my keyboard settings, the player moves two spots and then stop each time I hold down the arrow key.
  5. When I set the key repeat rate to a medium level on my keyboard settings, the player moves until I release the key.
  6. Rate of motion is insensitive to changes motion_speed_limit, meaning that the player does not move at the maximum rate when the key is held down.

I think this can (mostly) all be explained by the mechanism you're using to produce continual motion, which is to rely on the keyboard firing multiple keypress events when a key is held down. The problem there is twofold: first, it means that the rate of motion varies across players according to their keyboard settings, and second, if the key repeat rate is really high, it sends a ton of web requests.

Instead, I'd recommend the following: when an arrow key is pressed, begin firing movement events in the requested direction in a loop at the maximum rate; then, when the arrow key is released, break out of that loop and stop firing movement events. That way, the rate of motion is independent of keyboard settings and is tethered to motion_speed_limit.

vlall commented 7 years ago

My latest commit tries to do as you suggested @MatthewWilkes, but I'm having some issues. Events are fired in rapid succession, so having a setInterval or a setTimeout hasn't worked for me. It's as if they're being executed successively from within my setInterval call.

suchow commented 7 years ago

@jessesnyder This is close, though for whatever reason there's now a lag that builds up as the key is held down. In particular, when the player is still and then I press a key, the motion always responds immediately. However, if I hold down the key for a while and then change direction, there is a lag before the player turns. This lag doesn't appear if I make a series of short movements.

jessesnyder commented 7 years ago

@suchow Yes, this seems to happen once the speed is beyond a certain threshold, which is why I'd previously hard-coded a speed that behaved well, rather than tying it to motion_speed_limit. I'll try to figure out why that's happening.

jessesnyder commented 7 years ago

@suchow @vlall Nothing too mysterious going on with the "lag" (motion continuing after key is released): the rendering of the grid, including the square representing the active player, is controlled by the grid state which is sent over the websocket from the server. So, from the time you release the key, you wind up watching changes to the grid that happen during the time your movements are sent to the backend and returned in the form of grid state. When the rate of movement is sufficiently slow this isn't noticeable, but once it's fast enough the time delay is perceivable.

Something I noticed while looking into this: the Player.move() implementation on the client side does nothing. I wondered about switching to rendering the active player (ego) on the client side, and then sending the player position instead of movement to the server, but this would just cause other delay issues. For example, when you consumed a piece of food, you'd appear to pass over right over it, and then a moment later it would disappear.

suchow commented 7 years ago

@jessesnyder @vlall Thanks for looking into this! Two notes:

  1. I'm doubtful that the WebSocket round trip explains the lag I'm observing for two reasons. First, that sort of lag ought to apply equally to every movement-based action, but instead, I'm seeing the lag only when I hold down the key for a while and then switch directions. In particular, I don't observe the lag the first time I move after standing still, where the WebSocket round trip still applies. Second, the lag increases as I hold down the arrow key longer before switching directions, even though that doesn't affect the rate of messages being sent to the server.

  2. The architecture is supposed to compensate for this server lag. It used to, and the fact that it doesn't anymore suggests there's been a regression sometime over the past couple months. In particular, all the movement-related logic is run twice, in parallel, once on the server and once on the client. The client is essentially making predictions about what the server will send next as state information via the socket connection. It's interesting that Player.move() doesn't do anything anymore, because it's supposed to update the client player immediately, killing lag. Then, in cases where the server state and client state conflict, the server state is considered canonical and client state is overwritten.

I'll take a look through the Git history to figure out when the regression happened and then fix it. I think that'll help something, but predict the lag observed above won't go away.

A mystery!

jessesnyder commented 7 years ago

@suchow I cede you've found flaws in my argument. :-) I keep puzzling over this to the point I'm afraid I won't sleep well. I think I reached my premature conclusion largely based on observation that movement messages continue to be processed (or at least logged) in handle_move() on the server after message sending appears to end on the client. Player movement on the client appears to correspond more or less exactly to executions of handle_move(), but it's perhaps dangerous to trust logging IO as perfectly synchronous (buffering?). Fascinating stuff.

You may also be right about the client side Player.move(). The position attribute of the active player is updated from the key presses (I logged this while trying to understand what was going on). My conclusion was based on the fact that when I removed the player update from the state data, the player no longer moved at all, whereas when I did the reverse (removed the client-side call to Player.move() and retained the updated from grid state data), the player moved exactly as it does with both forms of update active.

Looking forward to learning more.

suchow commented 7 years ago

@jessesnyder @vlall Okay, used git bisect to find the commit where it disappeared:

https://github.com/Dallinger/Griduniverse/commit/679d3be823c6db157a451386e2d03b4c2756d1ad#diff-ae993eae94fd0e5cc051ac7901a56f1eL22980

I've reinstated it in this PR:

https://github.com/Dallinger/Griduniverse/pull/122

jessesnyder commented 7 years ago

Now configures the chat backend to deliver messages with no delay (depends on Dallinger PR https://github.com/Dallinger/Dallinger/pull/689)

suchow commented 7 years ago

@jessesnyder The latest chat backend change seems okay, but maybe the default here should be something greater than zero to avoid the "constant context switching" issue that we don't fully understand? Maybe 0.001 (1 ms)?

jessesnyder commented 7 years ago

@suchow The default is 0.1 (same value we were using before the change), but I agree that even in the GU context a non-zero value feels safer.