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

Refactor 'Examine inventory item' menu #1802

Merged
merged 5 commits into from
Sep 3, 2022

Conversation

olanti-p
Copy link
Member

Summary

SUMMARY: Interface "Refactored 'Examine inventory item' menu"

Purpose of change

Current problems with this menu:

  1. It's some old code that seems to be around from before input_context and assignable keybindings were a thing
  2. It's spread out all over the place, mainly throughout the god classes:
    • Main chunk of code is defined as a method of game
    • Helper methods for action validity (rate_action_x) can be found in all 3 of Character, player and avatar
    • Methods for executing actions are either methods of game class or Character family, or functions from avatar_action namespace
    • Action rating enum itself (hint_rating) is defined in item.h
  3. Lambda arguments have oddly specific default arguments that only get used once
  4. It seems to have claims on providing some long-unused(broken?) functionality such as additional menu layouts and scrolling of parent UIs
  5. Its helper functions (rate_action_x) are getting used in some other places in the code, which works, but feels way too hacky for my taste

Describe the solution

  1. Create a dedicated module for the menu, examine_item_menu.h/.cpp
  2. Move hint_rating enum and all rate_action_x functions there, both declarations and definitions
  3. Get rid of rate_action_x usages outside this menu, rewrite relevant bits. Tweak some rate_action_x functions for bugfixing/untangling purposes.
  4. Create item_functions.h/.cpp to same purpose as character_functions.h/.cpp and map_functions.h/.cpp
  5. Remove avatar-centric action methods out of game class, either dissolve them or move to avatar_action namespace as functions
  6. Rewrite the menu itself to use an input_context with keybinds and action names defined in JSON file
  7. Make PgUp/PgDn scrolling smoother, make Up/Down wrap to action list end/start.
  8. Move throwing action into an activity to fix AIM or item details screen obscuring part (or all) of the terrain view when throwing through this menu.

Testing

Tried actions in the menu, tried throwing (and peek-throwing) items, scrolling and resizing the menu.

Additional context

Resizing doesn't always work perfectly with AIM, but it doesn't crash.

@Coolthulhu Coolthulhu self-assigned this Aug 26, 2022
@Coolthulhu
Copy link
Member

Much cleaner!

One thing I don't get: why is throwing implemented as a long action now?

if( get_option<bool>( "ANDROID_INVENTORY_AUTOADD" ) ) {
add_key_to_quick_shortcuts( oThisItem.invlet, "INVENTORY", false );
}
#endif
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 see an equivalent Android special case in the new version. Is it no longer needed?

I could try testing it on an emulator, but it's rather painful to set it up.

Copy link
Member Author

@olanti-p olanti-p Aug 28, 2022

Choose a reason for hiding this comment

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

Oh, crap, I forgot this was a thing.

Not completely sure what it's needed for actually, I'll have to poke the original version on the phone.

Copy link
Member Author

@olanti-p olanti-p Aug 28, 2022

Choose a reason for hiding this comment

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

Update: I don't get it. Compiled a release with this PR, and I fail to see any difference before and after in-game.

@olanti-p
Copy link
Member Author

why is throwing implemented as a long action now?

It's needed for AIM hiding logic.
It detects a long action, hides the UI, pushes it to the activity stack, then lets the action proceed (throwing UI shows up, user throws item or aborts the operation), then long action ends and AIM pops up back from the stack.

@Coolthulhu Coolthulhu merged commit 3f2dd6e into cataclysmbnteam:upload Sep 3, 2022
@Coolthulhu
Copy link
Member

It's needed for AIM hiding logic.

Evil, but good enough for now.
Unless we re-do all actions as activities one day. Then it will retroactively become good and pure.

@olanti-p olanti-p deleted the refactor-examine-item branch September 3, 2022 11:37
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.

None yet

2 participants