Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

game_loop and winit_input_helper do not appear to be compatible (invaders example) #270

Closed
Zageron opened this issue Apr 8, 2022 · 5 comments · Fixed by #272
Closed
Labels
upstream bug Bug appears to originate in an upstream dependency

Comments

@Zageron
Copy link
Contributor

Zageron commented Apr 8, 2022

I was using your invaders example as a reference and ran into an issue.
This isn't something you can fix, but thought I'd let you know.

g.game.input.update(&event) never returns true, which means the state can never be guaranteed to be cleared.

Ex. This will never run the keyboard events, and if I remove the if guard the keyboard state remains between events and I get multiple false positives.

The only state you care about is the example is key_pressed and window_resized, for actions that don't care about repeat triggers. Add a print statement to see what I mean.

game_loop(
    event_loop,
    window,
    game,
    FPS,
    0.1,
    |g| {
        g.game.update();
    },
    |g| {
        // Drawing
        g.game.world.draw(g.game.pixels.get_frame());
        if let Err(e) = g.game.pixels.render() {
            error!("pixels.render() failed: {}", e);
            g.exit();
        }

        #[cfg(not(target_arch = "wasm32"))]
        {
            // Sleep the main thread to limit drawing to the fixed time step.
            // See: https://github.com/parasyte/pixels/issues/174
            let dt = TIME_STEP.as_secs_f64() - Time::now().sub(&g.current_instant());
            if dt > 0.0 {
                std::thread::sleep(Duration::from_secs_f64(dt));
            }
        }
    },
    |g, event| {
        if g.game.input.update(&event) {
            println!("{:?}", event);
            // Close events
            if g.game.input.key_pressed(VirtualKeyCode::Escape) || g.game.input.quit() {
                g.exit();
            }

            if g.game.input.key_released(VirtualKeyCode::Space) {
                g.game.head.play_song();
                println!("Playing song");
            }

            // Resize the window
            if let Some(size) = g.game.input.window_resized() {
                g.game.pixels.resize_surface(size.width, size.height);
            }
        }
    },
);
@parasyte
Copy link
Owner

parasyte commented Apr 8, 2022

I remember having some issues when I integrated game-loop, and this might have been the biggest one. It is a good observation, though!

I'm trying to recall exactly what those issues were while working on #252. I think the reason I ended up with the code as it is today (not checking the result of g.game.input.update() is because, as you discovered, it will never return true). The reason for this is because game-loop consumes the MainEventsCleared event and never sends it to us. Meanwhile, winit_input_helper uses this same event to return true. (But otherwise, it functions correctly. E.g. its CurrentInput is stepped every frame by processing the NewEvents event.)

It's just serendipitous that winit_input_helper doesn't actually do anything except return true for that event. What I ended up doing is using winit_input_helper to accumulate input events and poll it (and the gamepad, if any) immediately for every event that it processes. This synchronizes our local view of the controls state. Admittedly, winit_input_handler is not really doing much, here. And I would rather poll the gamepad independently of passing events to winit_input_handler.

If I were to refactor this code again, I would remove winit_input_helper, since its utility is questionable with game-loop managing the event loop. We can accumulate input events without it. I also recommend keeping an abstract concept of controls that is independent of keyboard inputs. Least wise so you can mix gamepad or touchscreen inputs.

That said, I am now beginning to wonder if game-loop' consuming the MainEventsCleared event might be a bug. There is no other way to finalize accumulation of input events than doing so in the update closure. And that can be called multiple times per frame. So it's not ideal, either. Patching it to forward this event to us would make winit_input_helper work as expected, and would also allow us to do our own input finalization prior to running the update and render closures.

@parasyte parasyte added the upstream bug Bug appears to originate in an upstream dependency label Apr 8, 2022
@parasyte
Copy link
Owner

parasyte commented Apr 8, 2022

Looks like doing your own input accumulation is better in the short term, anyway. The current behavior of winit_input_helper's key_pressed method isn't as documented: rukai/winit_input_helper#17

@Zageron
Copy link
Contributor Author

Zageron commented Apr 8, 2022

Wow thanks for the insanely thorough investigation and quick fix attempt. :o

@Zageron
Copy link
Contributor Author

Zageron commented Apr 8, 2022

I tested our your branch as a dependency and it resolved my issues, thank you! :)
Once I get more progress done on my MVP I'll likely take into consideration your recommendation to refactor out winit_input_helper and write my own control logic.

@tuzz
Copy link

tuzz commented Apr 14, 2022

Hi, just thought I'd chip in and say I've released version 0.9.0 of game-loop that, once integrated, should resolve this problem. Cheers.

parasyte added a commit that referenced this issue Apr 25, 2022
parasyte added a commit that referenced this issue Apr 25, 2022
* Update dependencies

- Closes #270

* Unify controls in Invaders example

The fire button on gamepads was allowing trapid fire when holding the
button. Keyboard controls required the fire key to be released between
each shot fired. This commit fixes the difference by making the gamepad
fire button act like the keyboard fire key.
JMS55 pushed a commit to JMS55/pixels that referenced this issue Jul 18, 2022
* Update dependencies

- Closes parasyte#270

* Unify controls in Invaders example

The fire button on gamepads was allowing trapid fire when holding the
button. Keyboard controls required the fire key to be released between
each shot fired. This commit fixes the difference by making the gamepad
fire button act like the keyboard fire key.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream bug Bug appears to originate in an upstream dependency
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants