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

Port over and update 'Streamline aim and fire experience (#59977)' #2468

Conversation

KheirFerrum
Copy link
Collaborator

@KheirFerrum KheirFerrum commented Mar 23, 2023

Summary

SUMMARY: Interface "Port over 'Streamline aim and fire experience (#59977)' "

Purpose of change

Ports over #59977.

Fixes issues with RELOAD_AND_SHOOT weapons described in #54997. No need to fix issue described in #50571 as it is already fixed in BN

Describe the solution

Allows re-entering of aim UI immediately after firing so long as there are still targets. Option is default on but can be turned off from the settings menu under Interface.

Add "reload_time" to aim_activity_actor as a member variable, which will save moves needed to load weapon. This will allow it to be refunded if the player starts the aim activity then immediately exits. This cost will then be referenced in the aiming UI. Displayed separately from the firing cost. If the "first turn" has passed, the firing cost is triggered and the unload cost is shown instead.

Describe alternatives you've considered

Testing

  • Spawn rifle, test that when firing rifle at targets, we return to the UI, check that it doesn't do anything stupid when it runs out of ammo, whether we have appropriate ammo/magazines or not.
  • Repeat test with revolver and similar weapons with integral magazines.
  • Repeat test with bow and crossbow, using multiple different types of ammo.
  • Ensure first_turn is reset and removed appropriately during each cycle.

Additional context

Not sure if we want to add unload cost to the menu as well. It might be useful for players to know.

  • Added.

* Re-enter aiming ui after shooting

* Calm down player's view snapping around while aiming

* Temporary workaround for RAS bugs

* Option to disable reaiming
@github-actions github-actions bot added the src PR changes related to source code. label Mar 23, 2023
@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from 4152838 to bd62de7 Compare March 24, 2023 01:25
@KheirFerrum KheirFerrum marked this pull request as ready for review March 24, 2023 02:07
Now displays loading cost and unloading costs.

First turn changed to indicate whether the player has chosen to aim or use the aim_and_shoot function.
@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from bd62de7 to e647020 Compare March 25, 2023 11:35
Was causing aim_and_shoot actions to abort prematurely.
@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from 3a8eba1 to 45d4acd Compare March 25, 2023 11:58
It should appear under the SNAP_TO_TARGET instead of under the disassemble query.
@KheirFerrum KheirFerrum changed the title Port over 'Streamline aim and fire experience (#59977)' Port over and update 'Streamline aim and fire experience (#59977)' Mar 25, 2023
src/ranged.cpp Outdated
if( activity->first_turn ) {
you->moves -= activity->reload_time;
activity->first_turn = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this bit at all. It's "realistic", but it won't be communicated well and is unintuitive.
Instead, it's better to have the cost be added to cost of firing and only that. Having RAS weapons go through normal reload function and apply all the associated move costs immediately is a mistake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The action cost of loading can be two turns easily though, I'm concerned about this forcing players to commit to a rather long action in a tense situation. If we're going to have this cost at all, at least if you decide to aim you're likely in a "relatively" safe position.

I'm also not entirely sure how it's unintuitive, the goal of this PR is pretty much to try and communicate it. I'm essentially mimicking the original function but allowing a cancel out of it. The original loading cost could never be cancelled (Edit: And was applied just by entering the firing menu).

@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from f9a399a to 14048d5 Compare March 29, 2023 14:03
Had to move the reload into `fire_gun()` otherwise the item location becomes a nullpointer and the game crashes.
@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from 14048d5 to 9a8bbab Compare March 29, 2023 14:11
@KheirFerrum KheirFerrum force-pushed the Port-Streamline-aim-and-fire-experience- branch from 7884e52 to 3ddd760 Compare March 29, 2023 19:14
@github-actions github-actions bot added the tests PR changes related to tests label Mar 29, 2023
@olanti-p
Copy link
Member

Ammo type doesn't show up anymore when aiming with a RAS weapon
image

Copy link
Member

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

Found a neat interaction: you can shoot your bow, then walk over to the arrow, then shoot the bow again without explicitly picking up arrow first.

src/ranged.h Outdated Show resolved Hide resolved
src/activity_actor.cpp Show resolved Hide resolved
src/activity_actor.cpp Outdated Show resolved Hide resolved
@KheirFerrum
Copy link
Collaborator Author

Found a neat interaction: you can shoot your bow, then walk over to the arrow, then shoot the bow again without explicitly picking up arrow first.

You could always do this I recall, it just has to be within 1 tile.

Use class instead of item_location.h Add read arguments for arguments added to save.
Copy link
Member

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

Ok, this PR is a definite improvement overall.
Drastically cuts down on the keypresses, and spaghetti removal is always good.

@olanti-p olanti-p merged commit 5a69bd0 into cataclysmbnteam:upload Mar 30, 2023
@KheirFerrum KheirFerrum deleted the Port-Streamline-aim-and-fire-experience- branch March 30, 2023 22:06
scarf005 pushed a commit to scarf005/Cataclysm-BN that referenced this pull request Mar 31, 2023
…ataclysmbnteam#2468)

* Streamline aim and fire experience (#59977)

* Re-enter aiming ui after shooting

* Calm down player's view snapping around while aiming

* Temporary workaround for RAS bugs

* Option to disable reaiming

* Update options.cpp

Fix translation

* Fix RELOAD_AND_SHOOT weapons

Now displays loading cost and unloading costs.

First turn changed to indicate whether the player has chosen to aim or use the aim_and_shoot function.

* Properly set first_turn for non-ras weapons.

Was causing aim_and_shoot actions to abort prematurely.

* Reorder options

It should appear under the SNAP_TO_TARGET instead of under the disassemble query.

* Redo move calculation of RAS weapons.

Had to move the reload into `fire_gun()` otherwise the item location becomes a nullpointer and the game crashes.

* Convert all item locations in this chain to not be references.

Fix test

* Fix Ammo Display

* Fixes

Use class instead of item_location.h Add read arguments for arguments added to save.

---------

Co-authored-by: Alexey <irwiss@users.noreply.github.com>
scarf005 added a commit that referenced this pull request Mar 31, 2023
* Update ranged.cpp

Fix dumb code I made

* Update src/ranged.cpp

Co-authored-by: scarf <greenscarf005@gmail.com>

---------

Co-authored-by: scarf <greenscarf005@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src PR changes related to source code. tests PR changes related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants