-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add two travel-related overmap keybinds for a better UX #64174
Add two travel-related overmap keybinds for a better UX #64174
Conversation
0fecfd9
to
bcb7469
Compare
Add two new keybinds GO_TO_DESTINATION and CENTER_ON_DESTINATION that allow the player to: - Move the cursor to the current travel destination. - Go to the current travel destination without the cursor needing to be on it. I was getting annoyed about constantly having to open the overmap and pan over to my destination over and over again when traveling somewhere to resume traveling after enemies showed up
…rd_code because it's unnecessary when setting keybinds for lower case letters Co-authored-by: Jianxiang Wang (王健翔) <[email protected]>
f4820b5
to
2180185
Compare
} | ||
return "QUIT"; | ||
} | ||
return nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why return a nullptr
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason (unfortunately). Looking back at the code I think it would be cleaner to return true/false in this function and set the action
variable to "QUIT" conditionally from the calling code. Although we've still got a function that returns a value and has a side-effect which isn't ideal... Let me type up a quick PR before I forget to refactor this.
EDIT: #64353
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant constructing a std::string
from nullptr
is going to crash the program here.
Edit: #64355
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh interesting, it wasn't crashing on my end when i tested it. But I'm not surprised that it could. Thanks for keeping an eye out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you press N to the prompt you'll reach here and access nullptr
memory.
This is a little cleaner CleverRaven#64174 (comment)
This is a little cleaner #64174 (comment)
Summary
Interface "Add two travel-related overmap keybinds for a better UX"
Add two new keybinds GO_TO_DESTINATION and CENTER_ON_DESTINATION that allow the player to:
Purpose of change
I was getting annoyed about constantly having to open the overmap and pan over to my destination over and over again when traveling somewhere to resume traveling after enemies showed up.
Describe the solution
Describe alternatives you've considered
For next steps, I'd like to make the GO_TO_DESTINATION keybind work directly from the game world as well as the overmap. Although that seems like a bigger change.
Let me know if you've got a better idea for the default values for the keybinds.
Testing
Additional context
Overmap screen with new keybinds being displayed:
GO_TO_DESTINATION key pressed when the cursor is NOT on the destination:
GO_TO_DESTINATION or CHOOSE_DESTINATION key pressed when the cursor is already on the destination: