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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Input Settings Menu #100

Merged
merged 14 commits into from
Jul 19, 2022
Merged

Input Settings Menu #100

merged 14 commits into from
Jul 19, 2022

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Jul 16, 2022

It's working!

Opening as a draft because I need to do a self-review, but it's functional. 馃帀

setings-menu-3

@zicklag zicklag linked an issue Jul 16, 2022 that may be closed by this pull request
@zicklag zicklag marked this pull request as ready for review July 17, 2022 02:07
@@ -22,7 +23,7 @@ getrandom = { version = "0.2", features = ["js"] }
bevy_mod_debugdump = { version = "0.4", optional = true }
bevy-inspector-egui = { version = "0.11", optional = true }
bevy-inspector-egui-rapier = { version = "0.4", optional = true, features = ["rapier2d"] }
leafwing_input_manager = { git = "https://github.com/Leafwing-Studios/leafwing-input-manager.git", rev = "e8bd6e0" }
leafwing_input_manager = { git = "https://github.com/zicklag/leafwing-input-manager.git", branch = "backport-leafwing-dev-to-bevy-0.7" }
Copy link
Member Author

@zicklag zicklag Jul 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Because I had to make a fix to leafwing, and they've already migrated to the pending bevy 0.8, there's no way around using my temporary fork for this anymore. I think we'll just have to wait 'till we migrate to Bevy 0.8 to switch this to a more permanent release.

To get my fix in, I had to apply it to their latest dev branch, then backport the dev branch to Bevy 0.7.

@zicklag
Copy link
Member Author

zicklag commented Jul 17, 2022

Just did a more thorough review of my own code: added lots of comments, removed some unneeded code paths, and fixed a minor usability issue with button hovering and focusing. I think this is ready now.

@odecay
Copy link
Collaborator

odecay commented Jul 18, 2022

Gonna try to review this tomorrow.

One thing to consider for a future pr is a "bind all" mode which when entered prompts for input to bind for the first Action on list, then immediately progresses to the next Action after pressing a key/button until all actions are bound to buttons.

@64kramsystem
Copy link
Member

Going to give a review as well tomorrow or the day after, as I'm about to travel 馃槃

- Simplify Controls Metadata and Move to Settings
- Remove camera controls
- Update leafwing input manager to get support for virtual dpads with
  axes in them.
This allows us to say how widgets should be navigated when pressing
directions on the gamepad or keyboard.
This makes sure that you can still use keyboard/gamepad navigation, even
if the mouse is idling over a button.
Copy link
Collaborator

@odecay odecay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I didn't have much meaningful feedback, noticed few small things that looked like spelling errors.

None of this should block this work or be included in this pr but some thoughts came up while looking it over.

I think maybe we will want to think more about what options players have for binding in the menu, I feel like maybe it should allow for selecting an input type and then only showing a map for that type instead of gamepad + keyboard 1/2 at the same time.

This might also relate to how we expect bindings to work in multiplayer both in local multiplayer and networked multiplayer. Which player gets to set their bindings once the menu is triggered? Right now it sets for all players correct? (I haven't actually pulled and tried it out yet so I could very well be misinterpreting)

src/ui/extensions.rs Outdated Show resolved Hide resolved
src/ui/main_menu.rs Outdated Show resolved Hide resolved
Co-authored-by: odecay <electricbuck@gmail.com>
@zicklag
Copy link
Member Author

zicklag commented Jul 19, 2022

noticed few small things that looked like spelling errors.

Good catch, I fixed those now. 馃憤

I feel like maybe it should allow for selecting an input type and then only showing a map for that type instead of gamepad + keyboard 1/2 at the same time.

That's a great idea. We could definitely do that in a next iteration. It would be an easy update.

This might also relate to how we expect bindings to work in multiplayer both in local multiplayer and networked multiplayer. Which player gets to set their bindings once the menu is triggered? Right now it sets for all players correct?

Yep. Every player shares the same gamepad bindings, and there are the two keyboard bindings so that you can play two players with one keyboard.

If we added a way to rotate through which bindings you are actually setting, like you mentioned, then that would make it easier to set different gamepad bindings for player 1 and player 2.

I'd have done separate gamepad bindings for both gamepads if it weren't for the fact that I ran out of room. :) Selecting which binding you are setting first solves a lot of problems.

@odecay
Copy link
Collaborator

odecay commented Jul 19, 2022

noticed few small things that looked like spelling errors.

Good catch, I fixed those now. +1

I feel like maybe it should allow for selecting an input type and then only showing a map for that type instead of gamepad + keyboard 1/2 at the same time.

That's a great idea. We could definitely do that in a next iteration. It would be an easy update.

This might also relate to how we expect bindings to work in multiplayer both in local multiplayer and networked multiplayer. Which player gets to set their bindings once the menu is triggered? Right now it sets for all players correct?

Yep. Every player shares the same gamepad bindings, and there are the two keyboard bindings so that you can play two players with one keyboard.

If we added a way to rotate through which bindings you are actually setting, like you mentioned, then that would make it easier to set different gamepad bindings for player 1 and player 2.

I'd have done separate gamepad bindings for both gamepads if it weren't for the fact that I ran out of room. :) Selecting which binding you are setting first solves a lot of problems.

Yep all totally makes sense. Lets add an issue for next steps for input binding after we get this PR merged in. Feel free to merge whenever you feel its ready.

@zicklag
Copy link
Member Author

zicklag commented Jul 19, 2022

Feel free to merge whenever you feel its ready.

Since nothing I'm doing next needs this I'll wait a day to let @64kramsystem look at it.

@odecay
Copy link
Collaborator

odecay commented Jul 19, 2022

Feel free to merge whenever you feel its ready.

Since nothing I'm doing next needs this I'll wait a day to let @64kramsystem look at it.

What are you planning on working on next btw? I was thinking of starting to work on making the attacks defined via startup + active + recovery frames that correspond to when hitboxes are live. Which would implicate #53 and #54 but I saw you assigned yourself #54. Would you mind if I took that one?

@zicklag
Copy link
Member Author

zicklag commented Jul 19, 2022

Would you mind if I took that one?

Oh, not at all, go ahead. That one looked easy and I was trying to find what the most important things to get out before the next release were, but I'd rather you took it anyway.

What are you planning on working on next btw?

Right now I'm working on a quick experiment to see if an idea I had regarding scripting is feasible or not.

I figured that if we have a solution for scripting, it might have an impact on how we program the players or items, so it'd be good to have some idea of what that might look like earlier rather than later.

It should just take a day or two to see if my idea has any merit, and it's kind of fun experimentation work so I figured I'd take a quick look as sort of a "break".

Then I'll come back to the #83 if nobody else wants it, or I'll just work on the update binding menu.

@odecay
Copy link
Collaborator

odecay commented Jul 19, 2022

Would you mind if I took that one?

Oh, not at all, go ahead. That one looked easy and I was trying to find what the most important things to get out before the next release were, but I'd rather you took it anyway.

What are you planning on working on next btw?

Right now I'm working on a quick experiment to see if an idea I had regarding scripting is feasible or not.

I figured that if we have a solution for scripting, it might have an impact on how we program the players or items, so it'd be good to have some idea of what that might look like earlier rather than later.

It should just take a day or two to see if my idea has any merit, and it's kind of fun experimentation work so I figured I'd take a quick look as sort of a "break".

Then I'll come back to the #83 if nobody else wants it, or I'll just work on the update binding menu.

okay perfect! scripting sounds awesome, followed along with the threads/issues on mun scripting/bevy_mod_scripting, were you thinking something like #107?

@zicklag
Copy link
Member Author

zicklag commented Jul 19, 2022

Replied here to make it more visible: #107 (comment)

@64kramsystem
Copy link
Member

Feel free to merge whenever you feel its ready.

Since nothing I'm doing next needs this I'll wait a day to let @64kramsystem look at it.

Looks good to me (although I'm not very familiar with the areas 馃槵) 馃槃

Copy link
Member

@64kramsystem 64kramsystem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

@odecay odecay merged commit f17b3c1 into fishfolk:master Jul 19, 2022
@odecay
Copy link
Collaborator

odecay commented Jul 19, 2022

Gettn it in then

@erlend-sh erlend-sh added this to the v0.0.3 milestone Jul 19, 2022
@zicklag zicklag deleted the input-settings branch July 19, 2022 22:42
@zicklag zicklag mentioned this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants