aeon0 / botty

D2R Pixel Bot
MIT License
531 stars 376 forks source link

Refactor Epic #489

Closed aeon0 closed 2 years ago

aeon0 commented 2 years ago

Miss use the issue/bug report here for a discussion on refactoring just sharing what I am planning. Its work in progress:

Removing features

Plan is to remove features that are either not used or "switched" by a config. One example is:

Looking for input of others that you think we can do without. A few others I would look into are these: message_highlight=1 d2r_windows_always_on_top=0 graphic_debugger_layer_creator=0

Split ui_manager.py

Currently ui_manager.py has many functions that are actually functional and dont need any dependency of the instance. Split up ui_manager in different parts: e.g. one might be a stashing.py. This way we are able to remove a ui_manager.py dependency on some.

Dependency Interfaces

We have sometimes big constructors with repetitive arguments. Espcially on the interface classes. Creating dependency classes will make this a bit more bearable. E.g. for IAct this could look like this:

class IActDependencies(dataclass):
    screen: Screen
    template_finder: TemplateFinder
    pather: Pather
    char: IChar
    npc_manager: NpcManager

General stuff

Find functional things that are currently "trapped" in classes and free them and maybe we can get rid of some dependency injections on some instances this way.

Check again if some implementation details can be separated from control flow. e.g. bot.py should hide all implementation details.

VladimirMakaev commented 2 years ago

To the point of ui_manager split the following could be an option

Composable Screen Objects

Goal:

Create higher level object interfaces for interacting with various objects in the game. E.g. MainMenu, Stash, Inventory, Cube, Keybinds, Waypoints

Context:

At the moment various functions are spread around the codebase and mixed with lower-level coordinate conversion code

Examples:

These are a few ideas how this could look like. But we clearly can define an encapsulation boundary between UI routines and UI elements. Having the ability to decouple one from another

  1. Creating a game could look like:
    main_menu = SceenObject.waitFor(MainMenu)
    # MainMenu would have a “locator” inside pointing to an asset to search for
    act = main_menu.select_difficulty(“Hell”)
  2. Interacting with inventory
    act = Act.from_location(location)
    chest = act.open_chest()
    inventory = ScreenObject.waitFor(Inventory)
    while True:
    col, row = inventory.get_loot_area().next_item()
    item = inventory.hover_over(col, row) # This would return object with item tooltip parsed (when ocr is there)
aliig commented 2 years ago

I'd take a peek at https://github.com/aeon0/botty/tree/ocr/src/ui to see how I rearranged ui_manager and belt_manager in case you need inspiration. I think what you have in mind will be better, but I agreed with the issues you brought up hence my change. :)

VladimirMakaev commented 2 years ago

This is another one I have in mind

Pluggable Routines

Goal:

Ability to integrate different routines into a “run lifecycle" without modification to the current code

Context:

We can define a lifecycle of the run and give an ability to "subscribe" a "routine" on a lifecycle event.

Examples:

We can subscribe routines explicitly via some sort of register method or implicitly via autodiscovery (I prefer explicitly)

bot.register_routine(PreBuffCTA, Lifecycle:RUN_START, config_key = "char.cta_available")
bot.register_routine(RemapTeleKeybind, Lifecycle:PICK_UP_CORPSE_COMPLETE, config_key="char.have_teleport_charges")
bot.register_routine(GemCubbing, Lifecycle:STASH_ITEMS_COMPLETE, config_key="general.cube_gems")
aeon0 commented 2 years ago

I had the same "waitFor" routine in mind. But my problem was, during "open_chest()" you actually need to check it to determine if you actually opened the chest. Otherwise what can happen, botty clicks on chest, but for some reason (e.g. moving while clicking) it missed the chest. Now we will wait forever for inventory. But in general, I am all for higher lvl object interfaces. That is basically what I meant by separating control flow from implementation details.

VladimirMakaev commented 2 years ago

I had the same "waitFor" routine in mind. But my problem was, during "open_chest()" you actually need to check it to determine if you actually opened the chest. Otherwise what can happen, botty clicks on chest, but for some reason (e.g. moving while clicking) it missed the chest. Now we will wait forever for inventory. But in general, I am all for higher lvl object interfaces. That is basically what I meant by separating control flow from implementation details.

open_chest() should be just smart to handle reliability but it should return an object to access stash items once it opened stash. My example also features the character inventory part. Basically if we got a stash inventory we can locate character inventory or alternatively we can return both objects with open_stash() e.g

stash_inventory, char_inventory = act.open_stash()
VladimirMakaev commented 2 years ago

Regarding dependency management I don't think it is necessarily "per-Act" but certain things could be done to minimize the burden.

  1. Define things that are cross-cutting and should be available everywhere. Maybe Config can have a static interface similar to Logger. Also it would be nice to have config to watch files and pickup changes and making it singleton can allow to make it threadsafe

  2. Define things that are "infrastructure" and can be provided via sort of ambient context. E.g.

    runtime = RuntimeServices.get(...)
    runtime.screen.grab(...)
    runtime.mouse.moveTo(...)
    runtime.keyboard.send(...)
  3. Define common "ui services" that are also provided via ambient context. E.g.

    ui = UIServices.get(...)
    ui.npc.open(Npc.Larzuk)

    There are probably other good ways to sort that out but those are the one came to my mind

VladimirMakaev commented 2 years ago

Btw regarding find_window_via_win32_api . I'm using it constantly as on as it simplifies testing a lot. So to clean this up I suggest removing config option and make it on by default. It will fallback to template matching anyway. I made it off by default to reduce regression but it looks quite stable from my point of view.

aliig commented 2 years ago
  • Not the biggest fan of the whole specific discord messaging. I like the generic text-based messaging. But might the minority on this one. (message_api_type)

I like these :)

aliig commented 2 years ago

Closed by #558