spxtr / p3

Super Smash Bros. Melee AI interface for Dolphin.
GNU General Public License v3.0
44 stars 8 forks source link

Generate address objects #5

Closed jabdownsmash closed 8 years ago

jabdownsmash commented 8 years ago

As per #4, This PR introduces an addr.py that generates a names dictionary and an addresses list of address objects. addr.py also generates locationsTxt, which is a string that holds a proper value for the Locations.txt file for Dolphin.

spxtr commented 8 years ago

I have a few issues with the design.

There might be some magic that we can do with setattr to fix the stringliness.

Also, I turned on PR testing in travis.

jabdownsmash commented 8 years ago

For the first issue, I agree and I've addressed it.

With regards to the second issue, I want to clarify that the Address class has no real relation with the game state, it's simply a way to smooth out parsing MemoryWatcher output.

With that in mind, is there a reason you would want to iterate through these address objects? settattr can be used to enable access to addresses like so:

addresses['players'][0].stocks()

I'm not sure I see a benefit. A GameState class would do well with this functionality:

if meleeState['players'][0].stocks() == 1:
    pass

However, I don't think that's within the scope of this PR.

jabdownsmash commented 8 years ago

I have changed the names around a bit to improve the semantics:

for address, value in memWatcher:
    ao = addr.AddressObjects.GetByAddress(address)
    parsedValue = ao.parse_bytes(value)
spxtr commented 8 years ago

Can we make it work as the game state class?

jabdownsmash commented 8 years ago

I think that the game state should depend on addr, but addr should be agnostic to the game state.

addr will potentially have hundreds of lines of just address definitions. There are use cases that include not tracking the game state, so I feel that imposing game state tracking functionality upon users that might just want the parsing functionality is unnecessary.

One potential solution would be to have a function that instantiates a map, in a sense. To illustrate: in the develop branch of my fork, a naive approach to managing the game state is taken:

class Melee:
    def __init__(self):
        self._listeners = {}
        self.gameState = {}

    def listen(self,home,callback):

        watcher = mw.MemoryWatcher(home + '/MemoryWatcher/MemoryWatcher')
        for address, value in watcher:
            ao = addr.AddressObjects.GetByAddress(address)

            parsedValue = ao.parse_bytes(value)
            self.gameState[ao.name] = parsedValue

            callback(ao,parsedValue)
            self.dispatch(ao.name, Event(parsedValue))

# later

playerStocks = [melee.gameState['player' + str(i + 1) + 'Stocks'] for i in range(4)]
    # ^^^ this is gross

Obviously, iterating that way isn't ideal, but I personally feel that the simplicity of the gameState in this case is really valuable. So what could happen is this:

class Melee:
    def __init__(self):
        self._listeners = {}
        self.gameState = {}
            # this is the only changed line
        addr.AddressObjects.MultiplesMagic(self.gameState)

# later
playerStocks = [melee.gameState['player'][i].stocks() for i in range(4)]
    # settr at work
spxtr commented 8 years ago

Okay, this seems reasonable. Mind renaming variables/functions to lowercase_underscores, making the documentation a bit clearer, and adding more/improving the tests?

jabdownsmash commented 8 years ago

Will do.

jabdownsmash commented 8 years ago

Alright, how does it look?

spxtr commented 8 years ago

Looks pretty good. I'll hit you with some small style changes later today, been very busy lately.

Thanks :)

jabdownsmash commented 8 years ago

Sorry for exploding this PR, but I've been doing more work and I identified another issue that I've potentially addressed in this branch.

When there are multiple address objects that share the same address, like this:

cls._multiple_address("controller", "Start",    at.BooleanAddress, "804C1FAC", "44", ("1", 12))
cls._multiple_address("controller", "Y",        at.BooleanAddress, "804C1FAC", "44", ("1", 11))
cls._multiple_address("controller", "X",        at.BooleanAddress, "804C1FAC", "44", ("1", 10))
cls._multiple_address("controller", "B",        at.BooleanAddress, "804C1FAC", "44", ("1", 9))
...

Currently only the newest entry will be added to the _address_map. The multipleaddr branch addresses this by making it so that AddressObjects.parse_bytes returns an iterator instead of a value:

for name, parsedValue in ao.parse_bytes(value):
    self.gameState[name] = parsedValue

If that looks okay to you, I can fix the tests and merge that branch into this one.

spxtr commented 8 years ago

I'm not in much of a hurry. Do whatever you think is best here and I'll take a careful look later tonight.

jabdownsmash commented 8 years ago

That should do it.