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

Update the dependencies to bevy 0.13 and bevy_egui 0.25 #463

Closed
wants to merge 46 commits into from

Conversation

Shute052
Copy link
Collaborator

@Shute052 Shute052 commented Feb 3, 2024

Status

The contents of this complex PR have been split into PRs #465, #466, and #470.

Objective

Update the dependencies to bevy 0.13 and bevy_egui 0.25 (unreleased).

Solution

Updates for bevy 0.13 (passed all tests) and bevy_egui 0.25 (passed all tests).

bevy_egui now has an PR #236 to update to bevy 0.13.
Unfortuanly, it is not yet updated to the new released bevy.
To address this, I temporarily use my forked version from the PR repo, fixing the problem.

TODO

  • Remove the logger after the bug resolved
  • Block on unreleased bevy_egui 0.25
  • Document

Relevant PRs

Migration Guide

  • Both KeyCode-based logical keybindings and ScanCode-based physical keybindings are no longer supported; please migrate to:
    • KeyCode is now representing physical keybindings.
    • InputKind::KeyLocation have been removed; please use InputKind::PhysicalKey instead.
    • ScanCode and QwertyScanCode have been removed; please use KeyCode instead:
      • All letter keys now follow the format KeyCode::Key<Letter>, e.g., ScanCode::K is now KeyCode::KeyK.
      • All number keys over letters now follow the format KeyCode::Digit<Number>, e.g., ScanCode::Key1 is now KeyCode::Digit1.
      • All arrow keys now follow the format KeyCode::Arrow<Direction>, e.g., ScanCode::Up is now KeyCode::ArrowUp.

Internal Changes

  • Input has been renamed to ButtonInput.
  • Replace assert! with assert_eq!.
  • Simplify code logic.
  • Fix typo in axislike.rs.

examples/twin_stick_controller.rs

  • Ray3d::intersect_plane() now take a Plane2d rather than a Vec3 as the normal of the plane after the merge of Bevy PR #10856
  • add_state has been renamed to init_state after the merge of Bevy PR #11043

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 3, 2024

After reviewing the Bevy PR #10702, it seems like ScanCode has been merged into KeyCode.

Could we consider merging ScanCode into KeyCode following the PR, and subsequently replace all instances of InputKind::Keyboard with InputKind::KeyLocation? Also, is it advisable to remove all code, examples, and documentation related to QwertyScanCodes and physical keybindings?

Encountered issues while updating InputMocking due to the breaking changes in the PR. Now we cannot get the logical Key from KeyCode.

It seems necessary to replace all occurrences of KeyCode with bevy::input::keyboard::Key for logical key bindings, and replace all instances of ScanCode with KeyCode for physical key bindings?

Both are failed, Key isn't derived the Copy trait, which is required by InputKind to create a InputKind::Keyboard(Key) variant.

@alice-i-cecile alice-i-cecile added the dependencies Update dependencies label Feb 3, 2024
@alice-i-cecile
Copy link
Contributor

@TimJentzsch, I'd appreciate your guidance around what to do with ScanCode.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 3, 2024

It seems that the usage of logical key bindings has undergone a breaking change, and now using InputKind::Keyboard(Key) is not as straightforward. The issue arises from the fact that Key now stores the actual inputted character for letter buttons. For instance, KeyCode::W now needs to be written as Key::Character(SmolStr(Repr::Static("W"))), which can be cumbersome.

Consider the following example:

let input_map = InputMap::new([
    (RunAction::Forward, Key::Character(SmolStr(Repr::Static("W")))),
    (RunAction::Backward, Key::Character(SmolStr(Repr::Static("S")))),
    (RunAction::Left, Key::Character(SmolStr(Repr::Static("A")))),
    (RunAction::Right, Key::Character(SmolStr(Repr::Static("D")))),
]);

While this setup functions normally on a US keyboard, it poses challenges for players using different layouts. For instance, French players might require bindings for the "ZQSD" keys, and Kyrgyz players might need bindings for the "Цфыв" keys.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 3, 2024

ReceivedCharacter also doesn‘t derive the Copy.

If we still require logical key bindings, we should find a way to translate a logical Key into its corresponding KeyCode on QWERTY keyboard.

@alice-i-cecile
Copy link
Contributor

Yeah, my feeling is that we're going to need to drop support for ScanCode in this version, then work upstream in both Bevy and winit to try and build a useful abstraction.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 3, 2024

Used a temporary solution, sending a placeholder like 'Mocking' as a logical key, fixed the compilation erros and passed all tests

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 4, 2024

Maybe I found a way to re-implement the logical key bindings, but it seems really breaking and ugly:
Remove Copy trait for InputKind, add InputKind::Keyboard(Key) for logical bindings, and replace all the previous KeyCode with Key.

For example:

KeyCode::Key<Letter> and KeyCode::Digit<Number> into Key::Character("<Letter or Number>"), e.g., Key::Character("a or 7")

This change is kind of a mess and could easily lead to some unintended chaos. Definitely needs a makeover.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 4, 2024

Maybe I found a way to re-implement the logical key bindings, but it seems really breaking and ugly: Remove Copy trait for InputKind, add InputKind::Keyboard(Key) for logical bindings, and replace all the previous KeyCode with Key.

For example:

KeyCode::Key<Letter> and KeyCode::Digit<Number> into Key::Character("<Letter or Number>"), e.g., Key::Character("a or 7")

This change is kind of a mess and could easily lead to some unintended chaos. Definitely needs a makeover.

This is really terrible after the test.

The situation is:

  • If you just press any letter keys, e.g., W key and then you will get a KeyboardInput(logical_key: "w"), an expected lowercase w letter.
  • However, if CapsLock is activated or Shift is pressing, like normal text input, you will get a KeyboardInput(logical_key: "W"), an UPPERCASE W LETTER!

@TimJentzsch
Copy link
Collaborator

@TimJentzsch, I'd appreciate your guidance around what to do with ScanCode.

I think it's safe to remove, to my understanding the new KeyCode provided by winit is pretty much the same thing, just with different naming.

As for logical keybindings: I think it makes sense that they are a string now.
These probably only make sense when you want the character being typed to be the same as you display on the screen.
If you implement a controls setting in your game, you probably want the user to type the keys they want to use for the binding.
Then you could perhaps safe the keycode for the thing to check for on press, but display the logical key in the UI.

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 4, 2024

Most functionality of new logical key bindings system has been completed, but only one problem:

ActionState::just_released(Key) will be reported in the next frame when reporting ActionState::pressed(Key) even we are holding it

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 5, 2024

Note that the new button_pressed() and input_value() computation for both MouseMotion and MouseWheel may seem confusing.
Refer to the Decompiled Code for insights.
These changes aim to assist the compiler in optimizing the code towards just a more efficient f64 accumulation, reducing executable size.
In my test with 10k random directions and events, the new version showed a roughly 1-3% improvement.

@alice-i-cecile
Copy link
Contributor

FYI, plan for tomorrow is to cut a release off of main for Bevy 0.12, and then come to this PR, do a final review pass, and then cut a second release for 0.13 :)

@Seldom-SE
Copy link

With

[dependencies]
bevy = { version = "0.13.0", default-features = false }

[dependencies.leafwing-input-manager]
git = "https://github.com/Shute052/leafwing-input-manager"
rev = "2b069dd"
default-features = false

I get

error[E0432]: unresolved import `bevy::asset`
  --> /home/seldom/.cargo/git/checkouts/leafwing-input-manager-a0e9919cdd1c7500/2b069dd/src/input_map.rs:10:11
   |
10 | use bevy::asset::Asset;
   |           ^^^^^ could not find `asset` in `bevy`

error: cannot determine resolution for the derive macro `Asset`
  --> /home/seldom/.cargo/git/checkouts/leafwing-input-manager-a0e9919cdd1c7500/2b069dd/src/input_map.rs:75:55
   |
75 |     Resource, Component, Debug, Clone, PartialEq, Eq, Asset, Reflect, Serialize, Deserialize,
   |                                                       ^^^^^
   |
   = note: import resolution is stuck, try simplifying macro imports

Minimal repro: https://github.com/Seldom-SE/testetst/tree/bebb52cbfc4d5cddad7bffe36a24eef38919126c

@Shute052
Copy link
Collaborator Author

Shute052 commented Feb 18, 2024

Minimal repro: https://github.com/Seldom-SE/testetst/tree/bebb52cbfc4d5cddad7bffe36a24eef38919126c

The issue was identified in the configuration, where bevy specifies:

#[cfg(feature = "bevy_asset")]
pub mod asset {
    //! Functions for loading and storing assets and resources for Apps.
    pub use bevy_asset::*;
}

And in your configuration, the default features have been disabled and the bevy_asset feature flag doesn't enabled specifically.

The problem can also be reproduced with the latest main branch or even after merging PR #421 (which addresses a bug related to the absence of the egui flag) with Bevy version 0.12.1.

@Shute052
Copy link
Collaborator Author

Another solution could be to make the Asset derive for InputMap optional, catering to those who prefer not to enable the bevy_asset feature.

@alice-i-cecile
Copy link
Contributor

Another solution could be to make the Asset derive for InputMap optional, catering to those who prefer not to enable the bevy_asset feature.

We should do that, and feature gate it.

@alice-i-cecile alice-i-cecile mentioned this pull request Feb 19, 2024
@alice-i-cecile
Copy link
Contributor

Oh, great idea with the table!

@Shute052
Copy link
Collaborator Author

Perhaps we could splitting the version table and the asset feature gate into a separate PR for the 0.12 release of this crate?

@alice-i-cecile
Copy link
Contributor

Perhaps we could splitting the version table and the asset feature gate into a separate PR for the 0.12 release of this crate?

Yep, I'd be happy with that. If you do, add a changelog entry and bump the version to 0.12.1 please.

@Shute052
Copy link
Collaborator Author

The contents of this complex PR have been split into PRs #465, #466, and #470.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants