So Close

The Repo on GitHub

I do manage to make an important change this morning, but I had hoped for much more. Progress is really quite good but I had fireworks in mind, not just sparklers.

My hope this morning was to get the World and Bot fully separated. As you’ll see we don’t quite manage it, but we do have all the accesses isolated. However, there is an issue. We presently set direction directly in the Bot when we want to change direction. We should do that with a command. We’d best do that first.

There’s always something …

In Bot there’s this:

class Bot:
    @direction.setter
    def direction(self, direction):
        self._knowledge.direction = direction

And senders:

class Bot:
    def change_direction(self):
        direction = self.direction
        while direction == self.direction:
            direction = random.choice(Direction.ALL)
        self.direction = direction

    def move(self):
        if random.random() < self.direction_change_chance:
            self.change_direction()
        step_succeeded = self.took_a_step()
        if not step_succeeded:
            self.change_direction()

Oh No!: Look at that move! It seems to be checking to see if in fact the move worked, right then and there. That won’t do at all. Dig further:

    def took_a_step(self):
        old_location = self.location
        self.step()
        return self.location != old_location

    def step(self):
        self.world.step(self)
        self.tired -= 1

This code demands that we check immediately whether a step took place or not. We just can’t do it that way: when world and bot operate over a connection, the bots’ messages and returned information will almost certainly be batched, so we just cannot do this.

I think there is a way around this: we have an update_knowledge method called from do_something:

class Bot:
    def do_something(self):
        self.update_knowledge()
        self.state = self.state.update(self._knowledge)
        self.do_state_actions()
        self.move()

    def update_knowledge(self):
        self._knowledge.gain_energy()

If we save old_location in took_a_step, instead of checking it there, we can check it here. That will be next time around. Let’s make that change and see if we can recover. I suspect some tests will break.

    def took_a_step(self):
        old_location = self.location
        self.step()
        return True

If this works, we’ll change the name of that and not check the result, but let’s see how bad things get. With this in place, just one test breaks:

    def test_changes_direction(self):
        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(10, 5)
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction != Direction.EAST
        client_bot.do_something()
        world.update_client_for_test(client_bot)
        assert client_bot.location != Location(10, 5)

This is failing, as one would hope, on the check for no longer facing Direction.EAST.

Now, our new plan will not fix this test on its own: we are changing the timing of direction change entirely. We need to change this:

    def move(self):
        if random.random() < self.direction_change_chance:
            self.change_direction()
        step_succeeded = self.took_a_step()
        if not step_succeeded:
            self.change_direction()

Note: I know I said that I wasn’t going to change that but since I had to look at it, I’ll change it:

    def move(self):
        if random.random() < self.direction_change_chance:
            self.change_direction()
        self._old_location = self.location
        self.step()

Pathing: I changed path here a bit, based on the real code. What we imagine will be right, and what we should really do, don’t always align. I think it’s best to go with reality, not fantasy.

I inlined the took_a_step method, and saved old_location in a new member variable. Added it to __init__ as None.

We still have that one broken test, but now I think we can change it in a reasonable way. I write a new test based ob the old one:

    def test_change_direction_if_stuck(self):
        def move_and_update():
            client_bot.move()
            world.update_client_for_test(client_bot)
            client_bot.update_knowledge()

        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction != Direction.EAST

Note that I decided to package up the three steps we need, moving, then getting the updated knowledge from World, then running the update_knowledge method. I think the test is easier to understand this way. The old test can be removed, then we have some naming to think about. I’m going to commit this change, as I think it is necessary. Commit: move direction change if stuck to after update from world.

Reflection

The name update_knowledge is wrong: we have done that. This method is called at the beginning of a do_somethign cycle:

    def do_something(self):
        self.update_knowledge()
        self.state = self.state.update(self._knowledge)
        self.do_state_actions()
        self.move()

What would be a better name for this? It’s checking results of the last move, but it also is updating the tired count, which, honestly, does that even do anything? No, it is an appendix. Energy is handled in Knowledge. (Another good reason why we need a different object for World, by the way, since energy is a concept unique to our particular Bots. One can imagine that the World might one day have an energy concept of its own, but this isn’t it.) In short, remove tired from Bot. Commit: remove unused tired member.

I was mistaken about who was updating tired. It did have to go, but update_knowledge updates energy, which I think is appropriate:

    def update_knowledge(self):
        self._knowledge.gain_energy()
        if self.location == self._old_location:
            self.change_direction()

I don’t see a good name for this. Getting “knowledge” out of it would be good. I would inline it into do_something so as to see the whole picture, in hopes that a rearrangement might occur to me, but I need the method because of the test that calls it. Rename to update_for_state_machine. Commit: rename method.

Reflection++

I am 200 lines into an article that was supposed to make sweeping important changes and so far nothing exciting has occurred. But we are making things happen in the right place.

I think most of Bot’s calls to world are isolated now. Let’s check.

class Bot:
    def do_state_actions(self):
        for action in self.state.action(self._knowledge):
            match action:
                case 'take':
                    self.world.take_forward(self)
                case 'drop':
                    self.world.drop_forward(self, self.inventory[0])
                case _:
                    assert 0, f'no case {action}'

    def step(self):
        self.world.step(self)

There is that one outlier, ‘step’ and we want a new action for setting direction.

step is called from move, which we do explicitly in do_something. The state machine only handles taking and dropping.

I see, through a glass, darkly, an idea. Let’s arrange to have a direction change added to the beginning of the state actions, if we want one, and to have a move operation / step operation added to the end, before we do the actions.

I have a good feeling about this idea: I think it will get us moving, although it will surely break that one direction test again. No matter, we’ll do something better.

Right now, the actions that come back from the machine are just strings. For direction, we’ll … oh I hate this but I think we have to get this done, we’ll return a string for the direction to face.

I go all in:

    def do_something(self):
        actions = []
        actions += self.update_for_state_machine()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        self._old_location = self.location
        actions += ['step']
        self.perform_actions(actions)

I’ve converted all those calls that add into actions to return a list of actions. The direction one will be returning ‘Direction.EAST’ and so on, I believe. Some tests break, mercifully few, presumably because the perform_actions method doesn’t understand all the possibilities yet:

    def perform_actions(self, actions):
        for action in actions:
            match action:
                case 'take':
                    self.world.take_forward(self)
                case 'drop':
                    self.world.drop_forward(self, self.inventory[0])
                case _:
                    assert 0, f'no case {action}'

First error:

>                   assert 0, f'no case {action}'
E                   AssertionError: no case step

Just as one would hope!

    def perform_actions(self, actions):
        for action in actions:
            match action:
                case 'take':
                    self.world.take_forward(self)
                case 'drop':
                    self.world.drop_forward(self, self.inventory[0])
                case 'step':
                    self.world.step(self)
                case _:
                    assert 0, f'no case {action}'

Now only one test fails, presumably on direction:

Yes but it didn’t fail looking for the command, as I expected. It fails because it didn’t change direction. Does the test make the wrong calls now? Yes indeed!

    def test_change_direction_if_stuck(self):
        def move_and_update():
            client_bot.move()
            world.update_client_for_test(client_bot)
            client_bot.update_for_state_machine()

        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction != Direction.EAST

I wonder if we can do an entire do_something cycle here. I’m worried about state changes but let’s try it.

Before I chase that, I try running the game and get this very curious failure:

    assert 0, f'no case {action}'
AssertionError: no case (0, -1)

That’s a direction, probably in the form of a string. I was expecting the text name. I implement a name method on my enum.

OK, after just a bit of trouble, the game works, and my one test still fails. Here’s what’s germane:

class World:
    def set_direction(self, bot, direction_name):
        match direction_name:
            case 'NORTH':
                bot.direction = Direction.NORTH
            case 'EAST':
                bot.direction = Direction.EAST
            case 'WEST':
                bot.direction = Direction.WEST
            case 'SOUTH':
                bot.direction = Direction.SOUTH
            case _:
                pass

class Bot:
    def do_something(self):
        actions = []
        actions += self.update_for_state_machine()
        self.state = self.state.update(self._knowledge)
        actions += self.state.action(self._knowledge)
        if random.random() < self.direction_change_chance:
            actions += self.change_direction()
        self._old_location = self.location
        actions += ['step']
        self.perform_actions(actions)

    def perform_actions(self, actions):
        for action in actions:
            match action:
                case 'take':
                    self.world.take_forward(self)
                case 'drop':
                    self.world.drop_forward(self, self.inventory[0])
                case 'step':
                    self.world.step(self)
                case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST':
                    self.world.set_direction(self, action)
                case _:
                    assert 0, f'no case {action}'

    def update_for_state_machine(self):
        self._knowledge.gain_energy()
        if self.location == self._old_location:
            return self.change_direction()
        else:
            return []

    def change_direction(self):
        direction = self.direction
        while direction == self.direction:
            direction = random.choice(Direction.ALL)
        return [direction.name()]

I think but am not certain that all the sends to world are in perform_actions. Let me check that. No, not quite, there is still step and it has callers. Who are these people? Two direct tests and some calls from move, also used in tests.

Let consolidate those by having them execute the perform_actions code. Then every access to the world will be isolated.

class Bot:
    def step(self):
        self.perform_actions(['step'])

Perfect! Now all the references to World are isolated in the perform_actions method. Commit: all access to world is in perform_actions. No, wait, I have that broken test.

Ah. I set the direction to the bot in world.set_direction. I need my own copy.

    def set_direction(self, client_bot, direction_name):
        bot = self.map.at_id(client_bot.id)
        match direction_name:
            case 'NORTH':
                bot.direction = Direction.NORTH
            case 'EAST':
                bot.direction = Direction.EAST
            case 'WEST':
                bot.direction = Direction.WEST
            case 'SOUTH':
                bot.direction = Direction.SOUTH
            case _:
                pass

I really thought that was going to do it. I decide to write a much more direct test:

    def test_requests_direction_change_if_stuck(self):
        bot = Bot(10, 10)
        bot._old_location = Location(10, 10)
        actions = bot.update_for_state_machine()
        assert actions[0] in ["NORTH", "SOUTH", "WEST"]

That works just fine. I am of a mind to remove those other two direction tests, but I’d like to be sure that the information will get back to the client bot.

OK, how about this:

    def test_change_direction_if_stuck(self):
        def move_and_update():
            print("move and update")
            client_bot.perform_actions(['step'])
            world.update_client_for_test(client_bot)
            actions = client_bot.update_for_state_machine()
            client_bot.perform_actions(actions)

        world = World(10, 10)
        client_bot = world.add_bot(9, 5)
        client_bot.direction_change_chance = 0.0
        move_and_update()
        assert client_bot.location == Location(10, 5)
        assert client_bot.direction == Direction.EAST
        move_and_update()
        assert client_bot.location == Location(10, 5)
        move_and_update()
        world_bot = world.map.at_id(client_bot.id)
        assert world_bot.direction != Direction.EAST
        assert client_bot.direction != Direction.EAST

I wasn’t getting the update back after the switch to returning the change as an action. I also added this test:

    def test_requests_direction_change_if_stuck(self):
        bot = Bot(10, 10)
        bot._old_location = Location(10, 10)
        actions = bot.update_for_state_machine()
        assert actions[0] in ["NORTH", "SOUTH", "WEST"]

That also passes. Now we can commit: all access to world is in perform_actions.

We are so close … but not there yet. I really thought today would be the day. And my estimate from last week of just a couple more days is right out.

We are so close. I’d try it now, but I need a break, and I’ve learned that when I need a break, either I take one, or I break the code. Breaks are good for me, not for the code.

Let’s sum up.

Summary

Despite my feeling that we got nothing done, we actually encountered and dealt with at least five sticky issues today:

The direction-setting got moved from client side to server side;
We converted all world requests to short lists of strings;
We do still call the world based on those strings, but only from one method, which we can now deal with;
We adjusted one test and wrote a new one for checking direction;
We now accumulate and execute commands from four sources:
- updates due to prior actions, e.g. location checks;
- state machine actions;
- random direction change;
- the mandatory step we always take.

And, bottom line, all the actions outside tests are now centralized into a single call to perform_actions. I think we can wire that to our DirectConnection and have the bot fully operational via the connection, which will mean it can work client-server.

See you next time!