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

Add blind throwing #26949

Merged
merged 1 commit into from Dec 14, 2018

Conversation

Projects
None yet
10 participants
@ralreegorganon
Copy link
Contributor

commented Dec 5, 2018

Summary

SUMMARY: Features "Add blind throwing"

Purpose of change

Add a new variation on throwing, a "blind throw" which can be used to throw around obstacles (e.g. corners) in exchange for an additional move cost and dispersion penalty.

Describe the solution

The blind throw is initiated by first peeking [X] and then pressing the hotkey to blind throw [t]. You are then prompted to select which item to throw using the normal menu for this selection. Note that the moves spent peeking at the target location and wielding the throwable item (if necessary) are spent while in the original location.

Then, you are presented with a blind throwing target selection menu, which takes into account the dispersion penalty that will be applied. Note the Blind throwing rock in the targeting menu and the terrible chance to hit...but I am targeting something that was untargetable around the corner in the previous screenshot.

blind-throw-skill-0-close

For comparison, here's the normal targeting menu and chance to hit if I just moved into the space and attempted to throw the rock normally.

normal-throw-skill-0-close

After selecting the target, the player is then returned to their original position while the projectile is thrown from the blind throw position with its appropriate penalties.

I've flagged this as [WIP] [CR] as I'm looking for feedback on some specific things, in addition to the usual code review:

  1. Does blind throw make sense as the nomenclature for this? Is there a better alternative? I'm not particularly attached to it, but it's what made the most sense to me.

Feedback so far seems to be that this is ok.

  1. I picked q as the keybinding for this--we're somewhat running out of space on the keyboard, but it seems worth giving a default binding to, q was open, and its proximity to Q (suicide) makes a certain sort of morbid sense to me because a character with low throwing skill attempting to blind throw an explosive around a corner might as well just hit Q. Is there a better key? Should it not be bound by default?

Based on feedback, I've moved this into a hotkey triggered from the existing peek action. I'd have preferred to take t since it would be consistent with throwing normally, but t within the LOOK keybindings is currently taken by the travel to location and I'm reluctant to change existing hotkeys, so instead I took T. After review, I've swapped this so t is now blind throw when peeking, and T is now the travel key.

  1. I went with the same move cost as peeking and quadrupled the normal throwing dispersion as the initial cost/penalty for this. I touch on my rationale some in the comments here, but I'd be interested in hearing other thoughts.

My refactor from the original implementation to now trigging this from within the existing peek action means that it always is going to burn at least the move cost for peeking, but can burn additional time if necessary. I left the quadrupling of normal throwing dispersion in here as my baseline penalty.

@Night-Pryanik

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

What happens if player chooses to blind throw from the position he is standing now? Will penalties to aiming be applied in this case?

Does blind throw make sense as the nomenclature for this?

Can't say for others, but for me it's good.

I picked q as the keybinding for this

Honestly I don't like this much distance (on the keyboard) between regular throw and blind throw. If anything, I'd prefer using Ctrl+t or Alt+t, but unfortunately the game doesn't support using modifiers like these, and certainly that's a wish for some other PR.

I went with the same move cost as peeking and quadrupled the normal throwing dispersion as the initial cost/penalty for this.

Sound like OK for me, but I guess more tests are needed.

@OzoneH3

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

Peek throwing doesn't feel like it would require it's own keybinding since it's very situational.
Maybe allow it to be activate able from the peek screen instead.

@Hatfish

This comment has been minimized.

Copy link

commented Dec 5, 2018

Could it be made a toggleable option from the throw screen?

@tinukedaya

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

Peek throwing doesn't feel like it would require it's own keybinding since it's very situational.
Maybe allow it to be activate able from the peek screen instead.

I like this! Throwing from the peek view would be the best.

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Peek throwing doesn't feel like it would require it's own keybinding since it's very situational.
Maybe allow it to be activate able from the peek screen instead.

This was my original line of thinking until I saw the peek implementation, which actually just prompts for a peek location, charges the moves, moves the player to that location, calls the same look_around method used throughout the application, and then when that returns, just puts the player back.

Because

  • we want to ensure that the time spent peeking, wielding the item to throw, and actually throwing is done in the original location rather than from the peeking location
  • we don't need the look_around UI, we need the throwing UI

I wasn't quite sure how to trigger the throw in a way that meets the above criteria without hacking up look_around. That being said, I see that the zone management has already hacked it up, so I'll look into what can be done there.

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Because

  • we want to ensure that the time spent peeking, wielding the item to throw, and actually throwing is done in the original location rather than from the peeking location
  • we don't need the look_around UI, we need the throwing UI

I wasn't quite sure how to trigger the throw in a way that meets the above criteria without hacking up look_around. That being said, I see that the zone management has already hacked it up, so I'll look into what can be done there.

I took a look at this and think I have a path forward that isn't too distasteful which gives us blind throwing as an action triggered from within the peeking action while maintaining my requirements above, so I'll implement it tonight.

@HotSake

This comment has been minimized.

Copy link

commented Dec 5, 2018

Since it's closely related, any chance blind firing can be implemented at the same time?

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Since it's closely related, any chance blind firing can be implemented at the same time?

I'd prefer to keep this limited in scope and do something like that in a follow up, but yes, I have also considered blind firing and will try to ensure that the approach I take doesn't preclude it.

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch 2 times, most recently from 021b42e to e9529a3 Dec 6, 2018

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

I've updated this as described above to now be triggered from with the X peek action. I updated OP based on the changes/summary of feedback, but I'll repeat it here too:

My original CR items:

  1. Does blind throw make sense as the nomenclature for this? Is there a better alternative? I'm not particularly attached to it, but it's what made the most sense to me.

Feedback so far seems to be that this is ok.

  1. I picked q as the keybinding for this--we're somewhat running out of space on the keyboard, but it seems worth giving a default binding to, q was open, and its proximity to Q (suicide) makes a certain sort of morbid sense to me because a character with low throwing skill attempting to blind throw an explosive around a corner might as well just hit Q. Is there a better key? Should it not be bound by default?

Based on feedback, I've moved this into a hotkey triggered from the existing peek action. I'd have preferred to take t since it would be consistent with throwing normally, but t within the LOOK keybindings is currently taken by the travel to location and I'm reluctant to change existing hotkeys, so instead I took T.

  1. I went with the same move cost as peeking and quadrupled the normal throwing dispersion as the initial cost/penalty for this. I touch on my rationale some in the comments here, but I'd be interested in hearing other thoughts.

My refactor from the original implementation to now trigging this from within the existing peek action means that it always is going to burn at least the move cost for peeking, but can burn additional time if necessary. I left the quadrupling of normal throwing dispersion in here as my baseline penalty.

@ralreegorganon ralreegorganon changed the title [WIP] [CR] Add blind throwing Add blind throwing Dec 6, 2018

@OzoneH3

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

A nice addition would be to ask if you would like to pull the pin before throwing a grenade.

If the thing you want to throw is already in your hand you should not spend the two turns first going back out of peek and then back into blind throwing. This leaves you with only one turn remaining after throwing an active emp grenade or none if it wasn't wielded first.

Show resolved Hide resolved src/game.cpp Outdated
}

cata::optional<tripoint> game::look_around( catacurses::window w_info, tripoint &center,
const tripoint start_point, bool has_first_point, bool select_zone )
const tripoint start_point, bool has_first_point, bool select_zone,
cata::optional<peek_action_opt> &peek_action )

This comment has been minimized.

Copy link
@BevapDin

BevapDin Dec 6, 2018

Contributor

Please don't make output reference parameters. The return value is for the output of the function, parameters are for the input. Make a new type containing the position and the action and return that (wrapped into optional).

This comment has been minimized.

Copy link
@ralreegorganon

ralreegorganon Dec 6, 2018

Author Contributor

I updated this to return a new type and axed the output reference parameter as suggested. However, I made the return type non-optional, but the members of that new type (position and action) both are--it seemed more obtuse to work with if the return type was also wrapped in optional, since it should always be returning that object, but different properties may or may not be set depending on the path through the function. Does that make sense?

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch 2 times, most recently from 00c76ec to 25b5474 Dec 6, 2018

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

A nice addition would be to ask if you would like to pull the pin before throwing a grenade.

If the thing you want to throw is already in your hand you should not spend the two turns first going back out of peek and then back into blind throwing. This leaves you with only one turn remaining after throwing an active emp grenade or none if it wasn't wielded first.

This seems like a useful addition, but I think would basically be an enhancement to the throwing action in general. #24561 discusses it. Ideally, that work is done independently and this just benefits from it.

Right now, I think if you want to blind toss a grenade around a corner your best bet is to wield the grenade, activate the grenade, peek, and then blind throw, and as you mention that's going to cost you the two turns that peeking takes.

As it stands right now, because I moved the blind throw to be chained off the peek, the only way I see to make it not burn the two turns on an active grenade would be to put the move point deduction (u.moves -= 200;) after the player gets to look around (and potentially blind throw) rather than before (or potentially split it, some before, some after.)

This would have the side effect of making it so that creatures would move after the player finished taking a look/throwing, rather than before.

I don't know that it'd be wrong, but I think it would be unexpected given current behavior where I know that when I exit the peek, everything is in the state I just saw. If my peek revealed a monster right around the corner, I know I can start running immediately and the distance between us will be what I just saw. If the move point deduction comes after the peek, then I may exit the peek only to be attacked because the monster advanced around the corner and attacked me while I was exiting the peek.

...and you'd still have some potential blast radius problems if you're trying to toss an active grenade and run away, the difference being that you'd spend the time "exiting" the peek in the same spot you threw it from rather than spending the time "entering" the peek with an active grenade in your hand.

Thoughts? My inclination is just to wait for #24561 to be tackled...

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch from 25b5474 to 5794a09 Dec 6, 2018

@dissociativity

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

I'm totally okay with this so long as it's not necessarily something one would use for say, throwing weapons at monsters, but rather, making sure an explosive or molotov arrives roughly where you want it as opposed to direct hits.

@dissociativity

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

Mind you, this could be useful to expand on suppressive fire, blind-firing so that we at least have some semblance of the ability to use cover against missile weapons and NPCs with them, hopefully with NPCs able to do the same.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/pks-rebalancing-unofficial-patch/16060/103

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

this could be useful to expand on suppressive fire

The main issue with suppressive fire is people's reaction to it, unless there are enemies that take cover when fired at, it's not going to do anything.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

While testing this, I hit 't' (lower case) more times than I can count, resulting in standing around spending turns with an armed grenade in my hands.

I'd generally agree that avoiding changing keybinds around is a good idea, but this is a very high stakes mistake to make.

Also the menu could use a prompt that throwing is now an option in the peek menu, hardly anyone is going to know about this otherwise.

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch from 5794a09 to ad4d104 Dec 7, 2018

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

While testing this, I hit 't' (lower case) more times than I can count, resulting in standing around spending turns with an armed grenade in my hands.

I'd generally agree that avoiding changing keybinds around is a good idea, but this is a very high stakes mistake to make.

Also the menu could use a prompt that throwing is now an option in the peek menu, hardly anyone is going to know about this otherwise.

Agreed on both counts. I've swapped this to t and moved the travel to location on to T, and added another line for the prompt to blind throw only when peeking:

image

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch from ad4d104 to c4a3d78 Dec 7, 2018

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

IDK if this has anything to do with trying out the previous version of the PR, but I ended up in a shouldn't-happen scenario of having throw and travel both bound to 't'.
blind_throw_keymap

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2018

IDK if this has anything to do with trying out the previous version of the PR, but I ended up in a shouldn't-happen scenario of having throw and travel both bound to 't'.

Confirmed.

I bet it's because we have our user-keybindings (./config/keybindings.json) which the game created when it didn't exist the first time, which specified that travel was t. Then, in my update here, I defined that travel was T and blind throw was t (./data/raw/keybindings.json). When the game runs, it sees that it doesn't have a keybinding for throw_blind in the user-keybindings and adds it from the system ones, and so now we have two entries for t, but we don't change the user's custom keybindings if the underlying system default has changed, which is basically what having t for travel has now become.

Any idea if we've got a mechanism for migrating keybindings, or another alternative approach that might be reasonable?

I did make the observation that if I change the order in which the actions are registered within the look_around, e.g. put https://github.com/CleverRaven/Cataclysm-DDA/pull/26949/files#diff-18513665750ef5adf42b5ec29e14162eR7328 before https://github.com/CleverRaven/Cataclysm-DDA/pull/26949/files#diff-18513665750ef5adf42b5ec29e14162eR7321, then it seems to resolve the throw instead of the travel. I don't know that's a great solution, but might be a workaround?

@HotSake

This comment has been minimized.

Copy link

commented Dec 9, 2018

Sounds like the code that is adding a keybind from defaults should necessarily check if that key is already bound, and leave it unbound if so. But that's for a separate PR, I'd think.

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

@kevingranade The "Travel to" event is only added to the input context when game::lookaround is called to set up zones. The "throw blind" event is only added when peeking.

So while you set up zones, the game does not know about the throwing event (and in turn does not know that "t" is already bound to it) and lets you bind "t" to "travel".
Later you are peeking and the game does not know about the "travel to" event (and that it was bound to "t") and so you are allowed to bind "t" again.

In theory you could bind "t" to yet another event when you just enter the normal look around command (not peeking and not setting up zones).

@BevapDin BevapDin closed this Dec 9, 2018

@BevapDin BevapDin reopened this Dec 9, 2018

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch from c4a3d78 to 71afc8e Dec 10, 2018

Add blind throwing
Add a new variation on throwing, a "blind throw" which can be used
to throw around obstacles (e.g. corners) in exchange for an
additional move cost and dispersion penalty.

@ralreegorganon ralreegorganon force-pushed the ralreegorganon:blind-throw branch from 71afc8e to aaf6422 Dec 10, 2018

@ralreegorganon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

The "Travel to" event is only added to the input context when game::lookaround is called to set up zones. The "throw blind" event is only added when peeking.

travel_to is added unless we're setting up zones:

if( !select_zone ) {
    ctxt.register_action( "TRAVEL_TO" );
    ctxt.register_action( "LIST_ITEMS" );
}

So in the case where we're peeking, both travel and throw get registered.

I went ahead and swapped the order that the events get registered, so in the case where someone had previously default keybindings for travel in the look menu (t), and when they updated the game they retained their custom keybindings, it will now throw instead of travel when they press t.

Anyone who has a fresh set of default keybindings won't have this problem.

@kevingranade kevingranade merged commit 54c89db into CleverRaven:master Dec 14, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gorgon-ghprb Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.