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 target UI refactor #804

Merged
merged 11 commits into from
Aug 27, 2021
Merged

Conversation

olanti-p
Copy link
Contributor

Purpose of change

Fix some bugs, add some small features, clean up the code.

Describe the solution

Due to the sheer amount of commits intermixed with various refactors I had to cheat a bit.

First commit contains the final result copy-pasted from revision 89c6d05885724d4f838fab2ddc1bbfe962a85c8b of DDA with a bit of cleanup and minor modifications to make it compile.

These changes encompass the following PRs:
CleverRaven/Cataclysm-DDA#39785
CleverRaven/Cataclysm-DDA#40103
CleverRaven/Cataclysm-DDA#40198
CleverRaven/Cataclysm-DDA#40221
CleverRaven/Cataclysm-DDA#40348
CleverRaven/Cataclysm-DDA#40417
CleverRaven/Cataclysm-DDA#40684
CleverRaven/Cataclysm-DDA#40727
CleverRaven/Cataclysm-DDA#42264
CleverRaven/Cataclysm-DDA#42988
CleverRaven/Cataclysm-DDA#42992
They also include some refactors done by ZhilkinSerg, jbytheway, kevingranade and Qrox, and maybe some other small changes I've missed.

Separately from that cherry-picked code from PRs
CleverRaven/Cataclysm-DDA#40236
CleverRaven/Cataclysm-DDA#43135
CleverRaven/Cataclysm-DDA#46008
CleverRaven/Cataclysm-DDA#42991
CleverRaven/Cataclysm-DDA#43099 (we don't have keycode support, but the changes are good on their own)

Testing

There's a lot of stuff, but it should "just work" as all bugs have been ironed out in DDA version.
I playtested it a bit, checked that none of the modes give CTD; specifically tested BN-exclusive changes and fixes:

  1. Initiating aiming from monster list:
    • is impossible if gun has bayonet, but is not loaded;
    • selects that exact monster even if there are other monsters closer to avatar;
    • automatically switches gun to ranged mode if gun is in bayonet mode.
  2. Initiating aiming with wielded weapon:
    • is impossible if gun has bayonet, but is not loaded;
    • is impossible if selected gun mode is empty, but some other mode is not;
  3. Manually firing vehicle turret:
    • monsters behind solid vehicle walls are considered as valid targets
  4. Aiming 10 tiles north, 2 tile west and 2 tile up generates a sensible line of fire due to fixed bresenham
  5. Spell AOE is visible in both tiles and curses
  6. It's possible to throw item without wielding it first

Additional context

The combined diff is a bit of a mess, reviewing on a per-commit basis might be easier.

olanti-p and others added 9 commits August 24, 2021 17:21
Main differences:
* misc small changes to make game compile
* removed item::current_reach_range method as it is redundant in hindsight
* added explicit low bound of 1 to item::reach_range
* reverted bits of keycode mode support (BN doesn't have it)
(cherry picked from commit b24639ae618952523aca77fbf30e5aee6b4ef867)
(cherry picked from commit dec478e910bb13bdf1991a85a03c19e5357a1dce)
(cherry picked from commit b0c6a4788b05d14014eb6eeb5f94eac7006d562a)
(cherry picked from commit f7cd56fa189e0e4168ea6b4d0061b1dc3f1d9a84)
Fix trying to aim from monster list with a gun
that has bayonet selected as active gun mode.
@Coolthulhu Coolthulhu self-assigned this Aug 27, 2021
@@ -332,7 +332,7 @@ void npc::check_or_use_weapon_cbm( const bionic_id &cbm_id )
//
// Well, because like diseases, which are also in a Big Switch, bionics don't
// share functions....
bool Character::activate_bionic( int b, bool eff_only )
bool Character::activate_bionic( int b, bool eff_only, bool *close_bionics_ui )
Copy link
Member

Choose a reason for hiding this comment

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

That pointer to bool is filthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, can't argue that. Should probably switch the func to return enum.

Copy link
Member

Choose a reason for hiding this comment

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

Enum would work, but it sounds like pushing menu logic into non-menu functions.
What's wrong with the old way it was handled, ie. the check at call location?

Copy link
Member

@Coolthulhu Coolthulhu Aug 27, 2021

Choose a reason for hiding this comment

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

Alright, it's ugly but it's working. I'll merge it now, refactor can come later.

EDIT: Rest of the code looks fine, it's just this one weird pointed bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with the old way it was handled, ie. the check at call location?

There was some problem in DDA with it, I don't quite remember the details. DDA also had some changes to bionics ui & bionic lockpick around that time that didn't make it into BN, so this part may actually be unnecessary. I'll check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants