BurnySc2 / python-sc2

A StarCraft II bot api client library for Python 3
MIT License
508 stars 158 forks source link

Disallow operating on old Unit objects #156

Open BurnySc2 opened 2 years ago

BurnySc2 commented 2 years ago

Problem: Accessing health, position and other attributes that can change in between frames may show different values every X frames. This means checking unit.is_idle may keep returning the same boolean based on unit.orders. unit.distance_to(other) will also not return the correct value because the unit.position is outdated, or if the scipy matrices are accessed then the unit objects may have shuffled matrix index values.

Suggestions: Disallow using distance_to and similar functions when using outdated unit objects. This only fixed the distance calculation. The same issue persists for unit.orders and unit.health, and authors may confused themselves.

Zane-Larking commented 5 months ago

I've got a couple of suggestions for reducing confusion. First create a getter for the _proto in the Unit class that checks the Unit's _bot_object.state.game_loop and game_loop attributes are equal (I see Unit.is_memory exists) and log a warning (or through an exception) if they aren't. Though, I don't know how much overhead this would add.

Note my understanding of the code base is pretty surface level. Sorry if anything I say from here is misleading 😅

With the Unit._proto.orders coming from the protobuf raw message you'd expect them to not change until the next frame. Thus if a Unit receives actions, Unit.is_idle will not update until the following frame. I'd suggest updating the Unit._proto.orders in BotAIInternal.do(). I do see it's a cached property so I'd suggest the following:

Either make 'orders' a (non-cached) property or

Further, though I haven't had the time to myself, I was thinking of adding a subscriber based "Units/Unit refresher class" that would run in the BotAIInternal._prepare_units and update the Unit prototypes of those subscribed (based on tags). You could, potentially, also have it also remove GameState.dead_units (Haven't considered this enough) so the user wouldn't need to manage dead units and structures.

For added performance for the above I'd suggest storing the underlying list of Units in ascending/descending tag order to make Units.find_by_tag() O(log n). This can be done by sorting the BotAIInternal.state.observation_raw.units O(n log n) prior to the bot Units' assignment and modifying the underlying List methods.