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

chore(UI): Unbind suicide, provide defaults for others such as Spellcasting #4503

Conversation

RobbieNeko
Copy link
Collaborator

@RobbieNeko RobbieNeko commented Apr 21, 2024

Purpose of change

Why we even had suicide bound by default, and to 'Q' of all keys, is a mystery to me. A mystery that can now be rectified.
Also, we were missing defaults for spellcasting, debug menu, and opening the diary, so I provided some for those upon receiving feedback.

Describe the solution

  • Unbinds Suicide by default
  • Binds Spellcasting to ] by default (it's right next to the mutations default on the keyboard)
  • Binds Debug menu to ` by default (also known as the grave key, or the default in a wide variety of other games)
  • Binds Diary to the Home key on the keyboard by default
    • I just kind of chose this key randomly. In truth, it could probably be bound to something else. However, it's not like people are going to be constantly using the diary anyway.

Describe alternatives you've considered

  • Continuing to have to advise players against pressing 'Q', then Yes twice

  • Leaving Spellcasting unbound by default

    • Yes, it's technically not really used in vanilla, but with how we have ~3 major magic mods now I'd say it deserves a default binding.

Testing

It doesn't crash when I load in, and it looks like this should work.

Additional context

"Watch how with just one simple trick, we reduced the number of player-suicides in Cata by 99.99%!"

@github-actions github-actions bot added the JSON related to game datas in JSON format. label Apr 21, 2024
chaosvolt
chaosvolt previously approved these changes Apr 21, 2024
Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

I'm still not 100% sure if unbinding quit is ideal but seems okay, I'd like to leavethis for at least one other to give feedback on tho.

@RobbieNeko
Copy link
Collaborator Author

I mean, who uses the suicide keybind anyway? Like, genuinely. It seems pretty niche, and we've got a load of other niche keybinds that are unbound by default

@VissValdyr
Copy link
Contributor

VissValdyr commented Apr 21, 2024

Hey, Lamandus here. Some thing to consider:

Toggle Auto Travel mode is also not bound. A very versatile button, especially for easy travel through forests.

While you did your own spin on it, unbinding suicide is also something DDA did beforehand. Maybe Linking their PR to attribute, wouldn't be a bad idea, if I remember it all correctly.

@scarf005
Copy link
Member

While you did your own spin on it, unbinding suicide is also something DDA did beforehand. Maybe Linking their PR to attribute, wouldn't be a bad idea, if I remember it all correctly.

i think it's not right to attribute PRs not actually done by them

@VissValdyr
Copy link
Contributor

While you did your own spin on it, unbinding suicide is also something DDA did beforehand. Maybe Linking their PR to attribute, wouldn't be a bad idea, if I remember it all correctly.

i think it's not right to attribute PRs not actually done by them

The question is, if the result is the same, (unbinding), can we prove it? I just don't want any trouble

@scarf005
Copy link
Member

The question is, if the result is the same, (unbinding), can we prove it? I just don't want any trouble

if removing a keybind can cause trouble i'm afraid there's nothing else we can edit without trouble

@VissValdyr
Copy link
Contributor

The question is, if the result is the same, (unbinding), can we prove it? I just don't want any trouble

if removing a keybind can cause trouble i'm afraid there's nothing else we can edit without trouble

If you say so. Okey. Still I would consider the toggle auto travel mode as a "useful" keybind

@KheirFerrum
Copy link
Collaborator

The Quit keybind is likely a holdover from the older Roguelikes Cata is based on, and from a time when Cata was much shorter and more closely reminiscent of a Roguelike experience. It probably doesn't fit anymore cause we rarely have truly run-ending situations and the games usually run a bit longer than the standard Roguelike. It's probably fine to remove it, but I'd appreciate not disparaging it without thought.

@RobbieNeko
Copy link
Collaborator Author

The question is, if the result is the same, (unbinding), can we prove it? I just don't want any trouble

if removing a keybind can cause trouble i'm afraid there's nothing else we can edit without trouble

If you say so. Okey. Still I would consider the toggle auto travel mode as a "useful" keybind

Holy hecc Auto Travel is smooth and looks great. I suppose I'll just put that on... comma for now, since that's convenient and not currently bound to anything by default

Thanks Viss, very cool suggestion
Copy link
Contributor

autofix-ci bot commented Apr 21, 2024

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@RobbieNeko
Copy link
Collaborator Author

Huh, the auto-linter in VSCode didn't catch that smol error somehow. Oh well, the keybinds file is a lil unusual.

@scarf005
Copy link
Member

scarf005 commented Apr 21, 2024

Huh, the auto-linter in VSCode didn't catch that smol error somehow. Oh well, the keybinds file is a lil unusual.

image

You need to enable files.trimTrailingWhitespace to automatically trim trailing whitespace on save.

@RobbieNeko
Copy link
Collaborator Author

Ah, good point. Thanks ^-^

@chaosvolt
Copy link
Member

While you did your own spin on it, unbinding suicide is also something DDA did beforehand. Maybe Linking their PR to attribute, wouldn't be a bad idea, if I remember it all correctly.

Yeah, if the PR OP did the change as their own work with no influence from DDA, especially for something that's a no-brainer like this, then that's their problem if they call us out for "stealing" something that's basically convergent evolution.

@RobbieNeko
Copy link
Collaborator Author

Indeed. DDA can't own the concept of unbinding the suicide key by default lmao

chaosvolt
chaosvolt previously approved these changes Apr 21, 2024
Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

I'm still leery of unbinding the quit key admittedly but like I said, looks aight but will wait for others to review too.

Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

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

Idea: maybe we could bind ` to lua console instead as backtick is commonly used for toggling debug terminal?

@Relgar
Copy link
Contributor

Relgar commented Apr 22, 2024

I must be the only player who smashes Shift Q like a maniac after I'm done testing something :x

@VissValdyr
Copy link
Contributor

I must be the only player who smashes Shift Q like a maniac after I'm done testing something :x

Probably. I use debug quit usually

@RobbieNeko
Copy link
Collaborator Author

Idea: maybe we could bind ` to lua console instead as backtick is commonly used for toggling debug terminal?

Considering I didn't even realize we had a LUA console, I'd think that's more of a developer concern than a player one. Plus, then we have to decide where we're putting the debug menu.

@scarf005
Copy link
Member

Idea: maybe we could bind ` to lua console instead as backtick is commonly used for toggling debug terminal?

Considering I didn't even realize we had a LUA console, I'd think that's more of a developer concern than a player one. Plus, then we have to decide where we're putting the debug menu.

how about F11?

@RobbieNeko
Copy link
Collaborator Author

I'll gladly put the LUA Console on F11

@scarf005
Copy link
Member

scarf005 commented Apr 22, 2024

I'll gladly put the LUA Console on F11

oh, i meant debug menu on F11 and ` for lua terminal

@RobbieNeko
Copy link
Collaborator Author

RobbieNeko commented Apr 22, 2024

Ah, okay.
I just think that the debug menu is going to be a lot handier to people to have the slightly more convenient access to, and fulfills much of the purposes one would generally turn to a console for in other games (spawning items, for example). Hence giving it "`" over the LUA console. But I suppose I can defer to your judgement if you think it should be other way around

@chaosvolt
Copy link
Member

Wait, can't you already access the LUA console from the debug menu anyway? That kinda makes debug menu vastly more useful as a binding for this anyway...

Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

Eh, another keybind I'm always gonna end up changing but ¯\(ツ)

@chaosvolt chaosvolt merged commit 017122d into cataclysmbnteam:main Apr 23, 2024
10 checks passed
@RobbieNeko
Copy link
Collaborator Author

Wait, can't you already access the LUA console from the debug menu anyway? That kinda makes debug menu vastly more useful as a binding for this anyway...

It totally is!
image

@chaosvolt
Copy link
Member

Yeah I predict everyone who actually cares about debugging, even LUA modders, are gonna end up changing this binding anyway since they're gonna need the regular debug functions to test stuff as well...

@scarf005
Copy link
Member

to be fair, almost every games have ` bound to consoles

@SilearFlare
Copy link

The thing already had not one but two case sensitive prompts
was that really necesasry

@scarf005
Copy link
Member

The thing already had not one but two case sensitive prompts was that really necesasry

sorry, this PR has been closed a while ago, could you be explicit on which keybind changes?

@chaosvolt
Copy link
Member

They most likely mean the quit command, as that indeed has multiple prompts before it kicks in. I noted that I was a bit leery of approving it, but now I've noticed an actual genuine issue with this: we don't have a bind for this in the escape menu, and debug "quit without saving" is NOT an adequate substitute for this.

If you have a world with more than one character in it and want to get rid of one (for example, one was a test character you had to save/load with and the other is your actual playthrough, because you forgot to create a separate world for testing), you have no option to actually clear that character unless you re-bind the suicide key, unless you want to reset the world and fuck over your actual playthrough save too. Only other things would be more obscure debug methods like setting your HP to zero or otherwise actually getting yourself killed in-game, which is not what we want to encourage players to go do just so they can clear out an unneeded character in a save they're still using.

@RobbieNeko
Copy link
Collaborator Author

It's just a single keybind that the small fraction who would need to delete such characters on any regular basis would need to re-bind. Additionally, having the Suicide key be bound by default for that reason seems like... a bit of a bandaid hiding the real issue of the fact that's not an option in some place like the main menu?

@chaosvolt
Copy link
Member

Seems like I probably should tinker with a PR to add a bind for it to the escape menu at some point...

@SilearFlare
Copy link

SilearFlare commented Jun 23, 2024

Yeah I had to dick around in the kebind menu for a bit to find the actual keybind in the first place, quit didn't show up anything so I thought it had been a DDA situation where they straight up removed the thing, until I explicitly checked the PR and actually tried looking for for "suicide" instead of quit, which was more than a tad unintuitive given the situation
bit of a step forward two steps backwards situation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants