Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

Display RAS weapon reload time in aim ui

Open osuphobia opened this issue 9 months ago • 1 comments

Summary

Interface "Display RAS weapon reload time in aim ui"

Purpose of change

Port over and modified https://github.com/cataclysmbnteam/Cataclysm-BN/pull/2468 Fix https://github.com/CleverRaven/Cataclysm-DDA/issues/50571 Fix https://github.com/CleverRaven/Cataclysm-DDA/issues/54997 Fix https://github.com/CleverRaven/Cataclysm-DDA/issues/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 https://github.com/CleverRaven/Cataclysm-DDA/commit/d94eea7b5acdb915e6c9908ff72e74456def0e61 is solved, AIM_AFTER_FIRING is enabled for RAS weapon. Deal with https://github.com/CleverRaven/Cataclysm-DDA/issues/73287, to make sure the stamina is cost and regained correctly. Modified the reload time test case to compare the total firing cost.

Describe alternatives you've considered

Choose the way in https://github.com/CleverRaven/Cataclysm-DDA/pull/73305.

Testing

Compiled and tested locally. RAS_time1 RAS_time2

Additional context

Based on https://github.com/CleverRaven/Cataclysm-DDA/pull/73305, it was reverted https://github.com/CleverRaven/Cataclysm-DDA/pull/73482 becuase of the attribution issue. Made further adjustment so the inconsistency pointed out in https://github.com/CleverRaven/Cataclysm-DDA/issues/73354#issuecomment-2081741740 is fixed. Please do not squash if this is to be merged to keep the credits.

osuphobia avatar May 06 '24 16:05 osuphobia

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

Maleclypse avatar May 13 '24 22:05 Maleclypse

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 avatar May 17 '24 17:05 I-am-Erk

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.

kevingranade avatar May 17 '24 17:05 kevingranade

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.

osuphobia avatar May 18 '24 01:05 osuphobia