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

Display RAS weapon reload time in aim ui #73550

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

osuphobia
Copy link
Contributor

@osuphobia osuphobia commented May 6, 2024

Summary

Interface "Display RAS weapon reload time in aim ui"

Purpose of change

Port over and modified cataclysmbnteam/Cataclysm-BN#2468
Fix #50571
Fix #54997
Fix #73287

Describe the solution

The move points needed for reloading RAS weapon is added to firing cost in aim ui, these moves are not consumed if you don't try to shoot or aim your bow. If you try to make your aim steadier by pressing ., your will reload your bow first.
As d94eea7 is solved, AIM_AFTER_FIRING is enabled for RAS weapon.
Deal with #73287, to make sure the stamina is cost correctly.
Modified the reload time test case to compare the total firing cost.

More details:

Before this PR, pressing f will choose the ammo and immediately reload with it, the moves and stamina are also consumed at this point. This caused several problems which are described in the issues.
After my modifications, calling load_RAS_weapon only determines what ammo the RAS weapon will use, but you don't reload with it at this point, and the moves needed are also not consumed but added to the total time_to_attack needed.
The item_location of ammo selected for reloading is saved at reload_loc, it is a reference of favorite ammo (if exists), or ammo manually selected.

It's just like you are only checking how your bow will perform with currently selected arrow.
The game will call update_ammo_range_from_gun_mode and use reload_loc to caculate and display ammo, range, and relevant data.
This also enabled the use of reload/switch ammo option for RAS weapon, the option was useless as you can't do anything with a weapon already fully loaded.
Canceling at this point will also cost no moves and stamina, and the refunding of moves in unload_RAS_weapon is also needless now.

Once you decide to really use you RAS weapon with currently selected ammo, by either triggering an aim action or one of the four firing actions, you will then reload with it, and consume the moves and stamina needed. The moves and stamina will not be refunded (and there is no need to do so anymore, as AIM_AFTER_FIRING will no longer trigger this mechanic).

Describe alternatives you've considered

Choose the way in #73305.

Testing

Compiled and tested locally.
RAS_time1
RAS_time2

Additional context

Based on #73305, it was reverted #73482 becuase of the attribution issue.
Made further adjustment so the inconsistency pointed out in #73354 (comment) is fixed.
Please do not squash if this is to be merged to keep the credits.

osuphobia and others added 2 commits May 4, 2024 12:35
Co-Authored-By: KheirFerrum <102964889+KheirFerrum@users.noreply.github.com>
@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` EOC: Effects On Condition Anything concerning Effects On Condition Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions labels May 6, 2024
src/activity_actor.cpp Outdated Show resolved Hide resolved
src/ranged.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels May 6, 2024
src/activity_actor.cpp Outdated Show resolved Hide resolved
src/ranged.cpp Outdated Show resolved Hide resolved
@Maleclypse
Copy link
Member

@I-am-Erk is this the desired order of operations for archery?

@I-am-Erk
Copy link
Contributor

I've been trying to review and understand this, @osuphobia, but even asking other devs we're all still a bit unclear on what exactly is going on. I think we need a more clear breakdown in the PR description of what you're doing and why in order to properly review this. I get the impression what you're doing is probably fine, but in order to know for sure, in the current state, I'd have to learn all the RAS weapon code from scratch.

src/turret.cpp Outdated Show resolved Hide resolved
@kevingranade
Copy link
Member

kevingranade commented May 17, 2024

If I'm understanding this correctly, the change here is "defer loading (including spending time and stamina on it) of reload and shoot weapons until you interact with the aim menu by either triggering an aim action or firing", and also "list whole cost of firing in UI, including reload time", is that right?
If so I agree with your overall direction, the only hard blocker here is the static int I call out below, that has to go.

src/ranged.cpp Outdated Show resolved Hide resolved
src/ranged.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label May 18, 2024
Co-authored-by: Kevin Granade <kevin.granade@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label May 18, 2024
@osuphobia
Copy link
Contributor Author

I've been trying to review and understand this, @osuphobia, but even asking other devs we're all still a bit unclear on what exactly is going on. I think we need a more clear breakdown in the PR description of what you're doing and why in order to properly review this. I get the impression what you're doing is probably fine, but in order to know for sure, in the current state, I'd have to learn all the RAS weapon code from scratch.

@I-am-Erk
What I did is basically the same as Kevin's description.
Before this PR, pressing f will choose the ammo and immediately reload with it, the moves and stamina are also consumed at this point.
After my modifications, calling load_RAS_weapon only determines what ammo the RAS weapon will use, but you don't reload with it at this point, and the moves needed are also not consumed but added to the total time_to_attack needed.

It's just like you are only checking how your bow will perform with currently selected arrow in the "first turn".
This also enabled the use of reload/switch ammo option for RAS weapon, the option was useless as you can't do anything with a weapon already fully loaded.
Canceling at this point will also cost no moves and stamina, and the refunding of moves in unload_RAS_weapon is also needless now.

Once you decide to really use you RAS weapon with currently selected ammo, by either triggering an aim action or one of the four firing actions, you will then reload with it, and consume the moves and stamina needed.

src/ranged.cpp Outdated
static int time_to_attack( const Character &p, const item &firing, const item_location &loc );
static int RAS_time = 0;
static int time_to_attack( const Character &p, const itype &firing );
static int RAS_time( const Character &p, const item &firing, const item_location &loc );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this part in a3eb155, can you review it again? @kevingranade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. EOC: Effects On Condition Anything concerning Effects On Condition Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
5 participants