Refactor aiming UI and its target selection / aim preservation

[olanti-p]

Summary

SUMMARY: Infrastructure "Refactored aiming UI and its target selection"

Purpose of change

target_handler::target_ui is a very old and very complex function. It accepts 8 arguments, some of them can be nullptr, some of them it can deduce, and some of them it doesn't really need.

This single function handles 5 cases of target selection:

• aiming with wielded weapon (including across turns via ACT_AIM activity)
• reach attacking (with e.g. spear)
• throwing item (with a sub-case of blind throwing)
• manning vehicle turret
• selecting target for multiple vehicle turrets

And allows to do many interesting things:

• move target aim point around (within given radius that can change)
• move view around (including Z-levels)
• "snap-to-target" mode, makes view follow the aim point
• semi-automatically select & cycle targets
• reload the weapon (forces the UI to close and opens reloading UI)
• change weapon firing mode
• switch ammo when manning turret
• aim - for 10 moves or for a turn
• "aim-and-fire", i.e. you press a button, and the game aims and fires for you
• predict how much your aim will drop if you move the aim point

It also draws/prints:

• terrain with aiming trajectory and aiming cursor
• info about cursor range/elevation/number of available targets
• info about gun being fired (mode, ammo, recoil)
• into about target creature

All of that the function handles as a 588-line monolith of interconnected C++. It also handles window creation and input context, operates on ACT_AIM, modifies player state variables in many places (and sometimes doesn't restore them back), uses 3 closures and contains some complex aim thresholds I don't really understand.

It also has a little sister called target_ui(...); // magic mod (350 LoC) which is basically a copy-pasted version, but for aiming spells. It contains some custom drawing code, but is missing some patches people applied to the normal version (notably the window expanding to the whole sidebar, changing map zoom and edge-scrolling).

Furthermore, target_ui() has a number of visual/usability/inconsistency bugs:

• you can try to fire an empty gunmod by starting aiming using a loaded weapon then switching firing mode to use the gunmod (gives debugmsg)
• on TILES version, aim point disappears when goes out of sight
• you could try to fire a gunmod farther then it actually can be fired by switching to long-ranged firing mode, selecting target, then switching back to the gunmod (didn't test if it works or gives a debugmsg)
• changing gun mode (and radius) doesn't update list of targets to cycle/display the number of
• parts of the UI jump up and down when moving aim point on and off creatures
• you can set a target even if none of the vehicle turrets are in range
• if you temporarily toggle on snap-to-target, move your aim point and aim long enough to pass a turn, the view gets forcibly reset
• if you move your cursor far enough to hit range boundaries, then aim for enough moves to pass a turn, the trajectory would be re-calculated and sometimes differ from the first version
• in some cases, aim bars get obscured by the list of controls
• in some cases, player sprite isn't flipped to face the current target
• if you toggle on 3D FoV and snap-to-target, move the aim point onto a different Z-level, then exit the UI, the player's sprite disappears
• if you enter the UI, aim at a creature, then move aim point away, leave the UI and enter it again, your aim would get worse. That could make sense if moving the aim point had an immediate impact on the game (like changing vehicle direction right now works), but it's just a visual cue for the user, it shouldn't do this

Describe the solution

Considering the huge number of arguments and long-living variables, I've decided it would be best to make this function into a class. This would make it easy to split the code into small segments without passing around long lists of arguments.

To avoid potential misuse, header bloat and boilerplate, I've put class declaration into ranged.cpp - this way, the class can only be used from the outside world by including ranged.h and calling one of the 6 provided mode_xxx functions. Each function takes bare minimum arguments necessary for the given mode, derives what it needs and assembles an instance of the class.

The class itself has a single public method run(). It handles setup, event loop, and exit. The code inside the event loop has been untangled with the focus on modifying player &pc and member variables only when necessary (e.g. aim point validation, check for which turrets are in range, trajectory calculation - all of these now happen when aim point moves, not on every input). The drawing code has been separated into small self-contained methods - "panels" - with each panel guaranteed to occupy a constant number of lines. I've had to increase window height by 1 to help with text overlapping, though.

I tried, as much as I could, to minimize visual changes and completely avoid big scary aim threshold calculations. But I had to refactor target selection (since it was there since before you could aim at empty tiles), and that produced some noticeable changes:

• If you kill a target, your aim point no longer resets. That means, semi-auto weapons become more effective against groups of enemies: as long as you can handle the recoil, you can feed the bullets into the horde with more-or-less constant accuracy.
• Fixes Shooting at an enemy that disappears/dies at the same time causes an infinite aim loop #39200

The new "change state only on user input" system allowed to implement some checks in a centralized manner:

• Detect when aim point is out of range of current gun mode
• Detect when none of the vehicle turrets are in range
• Detect if current gun mode is empty
• Detect if aiming at yourself

And these checks made it possible to cleanly implement (or refactor) dynamic activation/deactivation of UI controls (see screenshots).

Separating window creation / input context / drawing, I believe, should help migrate it to ui_adaptor (no idea how it's done, didn't even try).

Describe alternatives you've considered

Gaze upon ranged.cpp and despair.

Declaring the class in ranged.h, but decided against it due to drawbacks mentioned earlier.

Making 6 child classes (1 for each mode), but I don't like inheritance, and many modes differ only slightly. That would cause either a billion little methods or a bunch of copy-pasted & customized big ones.

Testing

I've tested each commit separately, and (with a few fixed cases), none of them seemed to break what they had changed.
The final result has been tested a bit using both TILES and CURSES, with "force redraw" and "3D FoV" turned on and off for all 6 modes. Things seem to be working, and none of the listed bugs have been observed.

The overlapping is somewhat better, with a bit bigger window and different drawing order, but still happens in some cases.
Edit: The overlapping should be completely gone now. If there is not enough space to print all controls, the code now discards some entries based on how (imo) useless they are. There is a (somewhat) new "[?] show all controls" reminder at the bottom border as well, that should help new players or those who don't bother remembering shortcuts.

Also, I'm not familiar with printing / string formatting functions. It works for English, and looks correct.

Upd: The new algorithm for target selection might cause some weird behavior due to player::last_target and player::last_target_pos not differentiating between UI modes.

Additional context

What a wall of text. It needs a wall of pictures!

UI elements not jumping with/without target selected (normal layout)

UI elements not jumping with compact layout

List of controls gets shorter when extremely short on space

UI elements not jumping with narrow layout from #37715

Trying to use an empty gunmod

Switching from a long-ranged gunmod to a short-range firing mode

No turrets in range, can't fire

[olanti-p]

That should be it. I've used the merge as an opportunity to polish out a few parts - namely, the list of controls (see updated Testing and Additional context). There are no plans for any other substantial changes, so no WIP tag

[ifreund]

Is it intentional that if you kill a target, the next time you press f to fire you will initially be aiming at the body of the zombie you just killed? seems like a UX regression from master due to the need to press tab to get the new target. Or is that linked to the "If you kill a target, your aim point no longer resets" thing?

[olanti-p]

It was more of a side-effect of the aim point preservation. I was conflicted should I do something about it (due to how it changes from the old behavior) or leave it as is (due to how it simulates player's aim point).
But I agree that it feels like a regression. Should be fixed now (the aim point is still preserved, just no visual indication).

[ifreund]

Thanks! For future reference I'd appreciate it if you could keep PRs under 1000 lines or so, stuff this big is hard to review (though breaking up the commits like this helps a lot).

[olanti-p]

Yeah, sorry about that. I just couldn't leave it halfway... Thank you for your time!

[anothersimulacrum]

This condition being removed means that NPCs can be made angry when hit by healing spells.

[anothersimulacrum]

This true should be some other condition to prevent that.

[olanti-p]

Yeah, this true was supposed to be that removed condition :/