Today I have to find a way to do it right … and light. How little can I do while still improving the code?

Yesterday I spiked a change that highlights cells on the map that have bee subject to a “look” command, because when playing the game, it’s easy to forget that you haven’t looked around lately, and to fall into a Pit. Because it was a spike, and in a fit of atypical righteousness, I reverted the code, with the plan to do it better today.

Truth is, according to my recollection, it wasn’t that bad. It was, however, a bit ad-hoc, patchwork here and there, and possibly we can do better. Let’s see if I can make a list of some of the things that we might want to deal with.

  • We probably want to keep this new information in our existing Knowledge structure;
  • We’ll therefore face there being two bits of knowledge associated with the same map x,y cell;
  • The most natural thing seems to be to have two kinds of facts: “an X is here”, and “we have looked here”. When the X moves away, we don’t want to forget that we’ve looked there.
  • The current Knowledge structure is sparse. If we haven’t ever looked at x,y, there is no entry for x,y.
  • Knowledge is kept in a simple linear list and cannot presently support two things at the same x,y.
  • Inserting new facts at the front of the Knowledge table nearly does the right thing. This isn’t what you’d call robust.
  • We need to support a shared notion of visibility distance between World and Robot. There is a place for this in the spec.
  • The current GridDisplay is just a class that returns a table of display-oriented information. It should probably return an instance of GridDisplay that can display itself and do other clever things.
  • Some people might disagree with the above, arguing that display should be done by a view object. I think I’d argue that we could rename GridDisplay to CellView and they should be thankful for the gruel we gave them.
  • Some tests are presently checking for nil coming back from looking for Knowledge, and these changes will break them if we return a look Fact. I think this is fine, just part of the job.

Today I can express what I want to accomplish with three goals:

  1. Implement the visibility picture just like yesterday’s, with a lighter background color for cells we’ve looked at;
  2. Go in very small individually committed steps;
  3. Improve everything we change;
  4. Change as little as possible.

(I was wrong about “three”. This happens a lot.)

Let’s get to it.

Visibility

If we’re going to do visibility, we should at least ensure that the Robot and the World use the same notion of how far a look can see. The spec says that the “launch” command returns the visibility distance as one of the data values returned from launch.

We have a magic number in World for visibility. Let’s find that and work from there.

The magic is here:

function World:lookInDirection(packets,lens, direction)
    for xy = 1,5 do
        self:lookFactInto(packets,lens, direction.mx*xy, direction.my*xy)
    end
end

We’ll not review the callers. This code gets called four times, one for each direction of scanning, N, E, S, W. the distance we scan is 5. Let’s set up some World constants and use them.

World.constants = {
    visibility = 8,
}

function World:lookInDirection(packets,lens, direction)
    for xy = 1,self.constants.visibility do
        self:lookFactInto(packets,lens, direction.mx*xy, direction.my*xy)
    end
end

That works. Commit: World has constants.visibility 8, distance used in look.

Now let’s return it from launch. Where do we build the launch response?

function World:launchRobot(name, kind,shieldStrength, shots)
    local robot = RobotState(self, name, kind, shieldStrength, shots)
    self._robots[name]=robot
    local response = {
        result="OK",
        data={},
        state=self:robotState(robot)
    }
    return response
end

Nice. Just return visibility:

function World:launchRobot(name, kind,shieldStrength, shots)
    local robot = RobotState(self, name, kind, shieldStrength, shots)
    self._robots[name]=robot
    local response = {
        result="OK",
        data={visibility=self.constants.visibility},
        state=self:robotState(robot)
    }
    return response
end

We do have some tests that do launch commands. I don’t think we have one that checks the response. Write one:

        _:test("launch response", function()
            local world = World(25,25)
            local rq = {
                robot="xyzzy",
                command="launch",
                arguments={"plugh", 99,99}
            }
            local resp = world:request(rq)
            _:expect(resp.data.visibility).is(World.constants.visibility)
        end)

OK, not much but it does test what we care about. Green. Commit: World returns visibility to Robot on launch.

Now that it’s coming in, we need a place to store this kind of world information.

We don’t have much in the way of tests for Robot launching, though we do it a lot for other tests. All we have to do is create a robot and it launches:

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._name = name
    self.knowledge = Knowledge()
    self._world:launchRobot(name,self,self:standardCallback())
end

Let’s write a test:

        _:test("robot launch parameters", function()
            local world = WorldProxy()
            local robot = Robot("Pookie", world)
            _:expect(robot.worldinfo.visibility).is(World.constants.visibility)
        end)

I’ve just decided that Robot will have a member variable containing whatever world information it remembers. Test should fail with an access to nil.

11: robot launch parameters -- TestRobot:136: attempt to index a nil value (field 'worldinfo')

We really need a new callback for launch, to unpack the special info in its response. So we’ll do it that way.

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._name = name
    self.knowledge = Knowledge()
    self._world:launchRobot(name,self,self:launchCallback())
end

This will fail for want of the callback. It’ll fail a lot.

11: robot launch parameters -- TestRobot:156: attempt to call a nil value (method 'launchCallback')

We implement that trivially:

function Robot:launchCallback()
    return function(...) self:launchResponse(...) end
end

This will fail looking for launchResponse …

11: robot launch parameters -- TestRobot:206: attempt to call a nil value (method 'launchResponse')

I’m being more than usually careful here, just implementing what the test tells me. Better way to work, really. I should do it more often. Implement:

function Robot:launchResponse(response)
    self:standardResponse(response)
end

This will fail the test looking for the worldinfo.

11: robot launch parameters -- TestRobot:136: attempt to index a nil value (field 'worldinfo')

We’re back where we started but now we have a place to stand. For now, we’ll do this in launchResponse:

function Robot:launchResponse(response)
    self.worldinfo = {
        visibility = response.data.visibility
    }
    self:standardResponse(response)
end

I expected this to work, but what I get is this:

10: robot wasd -- TestRobot:211: attempt to index a function value (field 'data')

Have I already forgotten what’s in the response?

No, but I have forgotten that we don’t get a dictionary back, we get a Response object. The code is wrong, should say:

function Robot:launchResponse(response)
    self.worldinfo = {
        visibility = Response:visibility()
    }
    self:standardResponse(response)
end

This will fail for want of visibility:

11: robot launch parameters -- TestRobot:211: attempt to call a nil value (method 'visibility')

Which we kindly implement:

function Response:visibility()
    return self._r.data.visibility
end

I am surprised, however, to get this error:

10: robot wasd -- TestResponse:82: attempt to index a nil value (field '_r')

That’s because the big fool called the class, not the instance.

function Robot:launchResponse(response)
    self.worldinfo = {
        visibility = response:visibility()
    }
    self:standardResponse(response)
end

Now I expect green. I get it. Commit: Robot records visibility from launch.

OK visibility exists and is shared.

What Now?

Well, naively, we want to record what cells we’ve looked at. And, because we’re all great people here, let’s write a test for it.

        _:test("Robot look records LOOK items in Knowledge", function()
            local world = WorldProxy()
            local robot = Robot("Esmerelda", world)
            robot:look()
            local v = robot.worldinfo.visibility
            _:expect(robot:factAt(0,v).content).is("LOOK")
        end)

I think this will drive out some useful code. Test expecting a nil kind of failure.

12: Robot look records LOOK items in Knowledge -- TestRobot:144: attempt to index a nil value

We return a nil when here is no fact. That may not be ideal but it’s what we do. We’re here to build a bridge, not to drain the swamp.

What we want to do is, before we send the “look” command, we should mark the visible cells as having a LOOK fact. That has to be done here:

function Robot:look()
    local callback = function(response) self:lookCallback(response) end
    self._world:look(self._name,callback)
end

And …

function Robot:look()
    self:markCellsLookedAt()
    local callback = function(response) self:lookCallback(response) end
    self._world:look(self._name,callback)
end

And that’ll drive out the mark method:

4: Robot does look/look on l key -- TestRobot:225: attempt to call a nil value (method 'markCellsLookedAt')

Forgive me for just writing it … It turns out like this:

function Robot:look()
    self:markCellsLookedAt(self.worldinfo.visibility)
    local callback = function(response) self:lookCallback(response) end
    self._world:look(self._name,callback)
end

function Robot:markCellsLookedAt(v)
    self:markHCellsLookedAt(v)
    self:markVCellsLookedAt(v)
end

function Robot:markHCellsLookedAt(v)
    for x = -v,v do
        self:addFactAt("LOOK",x,0)
    end
end

function Robot:markVCellsLookedAt(v)
    for y = -v,1 do
        self:addFactAt("LOOK",0,y)
    end
    for y = 1,v do
        self:addFactAt("LOOK",0,y)
    end
end

This will make my existing test work, but break some others.

I am mistaken here. My new test didn’t even pass.

12: Robot look records LOOK items in Knowledge  -- 
Actual: nil, 
Expected: LOOK

That’s because factAt returns the content. Test should be:

        _:test("Robot look records LOOK items in Knowledge", function()
            local world = WorldProxy()
            local robot = Robot("Esmerelda", world)
            robot:look()
            local v = robot.worldinfo.visibility
            _:expect(robot:factAt(0,v)).is("LOOK")
        end)

OK, now it’s just the others. Here’s the first one and it confuses me.

1: Robot updates knowledge on move  -- 
Actual: LOOK, 
Expected: fact

Here’s that test:

        _:test("Robot updates knowledge on move", function()
            local world = WorldProxy:test1()
            local robot = Robot("Louie",world)
            robot:look()
            _:expect(robot:factAt(3,0)).is("fact")
            _:expect(robot:factAt(3,1),"exists not yet visible").is(nil)
            robot:forward(1)
            _:expect(robot:factAt(3,-1)).is("fact")
            _:expect(robot:factAt(3,0)).is(nil)
            robot:look()
            _:expect(robot:factAt(3,0), "after second look").is("not visible on first look")
        end)

The test1 method adds a couple of facts:

function World:test1()
    local world = World()
    world:addFactAt("fact",3,0)
    world:addFactAt("not visible on first look", 3,1)
    return world
end

Why did my visibility stuff break that?

I resort to dumping the facts, and I find that the LOOK facts are first in the table. Right. The World doesn’t tell us about the facts it has until we look. So the important fact is covered by the LOOKs. Yesterday I changed Knowledge this way:

function Knowledge:privateaddFactAt(aFact)
    table.insert(self.facts, 1, aFact)
end

That will reverse the table. Also, in looking at the dump, I noticed two occurrences of a LOOK at the same coordinates. The fix is this:

function Robot:markVCellsLookedAt(v)
    for y = -v,-1 do
        self:addFactAt("LOOK",0,y)
    end
    for y = 1,v do
        self:addFactAt("LOOK",0,y)
    end
end

I had 1 there at the top, not -1. Too clever. I do not have a test for this. But I think most of my tests should run now, other than the ones looking for nil.

5: Robot moves forward on w key  -- 
Actual: LOOK, 
Expected: nil

The test, corrected, is:

        _:test("Robot moves forward on w key", function()
            local world = WorldProxy()
            world:addFactAt("O",3,1)
            local robot = Robot("Louie", world)
            _:expect(robot:y(),"initial").is(0)
            robot:look()
            _:expect(robot:factAt(3,0)).is("LOOK")
            robot:keyboard("w")
            _:expect(robot:y(), "post").is(1)
            robot:look()
            _:expect(robot:factAt(3,0)).is("O")
        end)

We are green. Commit: Robot records LOOK fact for each cell visible on a look command.

We are left with everything working except the display. No tests for that, we test it visually. We want to return a new color for LOOK cells. We have this:

        _:test("GridDisplay", function()
            local gd = GridDisplay()
            _:expect(gd:display(nil).color).is(color(0,256,0,48))
            _:expect(gd:display("OBSTACLE").color).is(color(0,256,0,128))
            _:expect(gd:display("PIT").color).is(color(0))
            _:expect(gd:display("RN").color).is(color(0,256,0,48))
            _:expect(gd:display("RE").color).is(color(0,256,0,48))
            _:expect(gd:display("RS").color).is(color(0,256,0,48))
            _:expect(gd:display("RW").color).is(color(0,256,0,48))
            _:expect(gd:display("RN").asset).is(asset.builtin.UI.Yellow_Slider_Up)
            _:expect(gd:display("RE").asset).is(asset.builtin.UI.Yellow_Slider_Right)
            _:expect(gd:display("RS").asset).is(asset.builtin.UI.Yellow_Slider_Down)
            _:expect(gd:display("RW").asset).is(asset.builtin.UI.Yellow_Slider_Left)
        end)

Might as well add a line:

_:expect(gd:display("LOOK").color).is(color(0,256,0,96))

That’ll fail for a moment:

1: GridDisplay  -- 
Actual: (0, 256, 0, 48), 
Expected: (0, 256, 0, 96)

And:

function GridDisplay:display(content)
    if content == "OBSTACLE" then
        return {color=color(0,256,0,128)}
    elseif content == "LOOK" then
        return {color=color(0,256,0,96)}
    elseif content == "PIT" then
        return { color=color(0)}
    elseif content and content:sub(1,1) == "R" then
        local a = self:assetInDirection(content)
        return {asset=a, color = color(0,256,0,48)}
    else
        return {color=color(0,256,0,48)}
    end
end

I expect green, no pun intended. I get it, and after some wandering and looking, the screen looks right as well:

screen showing light green where we've looked

I think we’re done. Commit: robot marks cells seen in lighter green color.

Let’s sum up.

Summary

I’ve been at it for two hours, counting writing the article. Five commits:

  • World has constants.visibility 8, distance used in look;
  • World returns visibility to Robot on launch;
  • Robot records visibility from launch;
  • Robot records LOOK fact for each cell visible on a look command;
  • Robot marks cells seen in lighter green color.

Everything is covered by tests, though the marking test is weak:

        _:test("Robot look records LOOK items in Knowledge", function()
            local world = WorldProxy()
            local robot = Robot("Esmerelda", world)
            robot:look()
            local v = robot.worldinfo.visibility
            _:expect(robot:factAt(0,v)).is("LOOK")
        end)

I meant to beef that up a bit. I don’t feel the need to test them all but want to test the ends:

        _:test("Robot look records LOOK items in Knowledge", function()
            local world = WorldProxy()
            local robot = Robot("Esmerelda", world)
            robot:look()
            local v = robot.worldinfo.visibility
            _:expect(robot:factAt(0,v)).is("LOOK")
            _:expect(robot:factAt(0,1)).is("LOOK")
            _:expect(robot:factAt(0,-1)).is("LOOK")
            _:expect(robot:factAt(0,-v)).is("LOOK")
            _:expect(robot:factAt(1,0)).is("LOOK")
            _:expect(robot:factAt(-1,0)).is("LOOK")
            _:expect(robot:factAt(v,0)).is("LOOK")
            _:expect(robot:factAt(-v,0)).is("LOOK")
        end)

Commit: improve test for LOOK marking.

Pretty much everything but the display was test-driven. I did take big bites when I implemented the markCells methods, and I did in fact make a mistake in there: I recorded a look fact for 0 and 1 twice. This is harmless: we’re recording lots of extra LOOK facts every time we do a look in the same row or column as a prior look.

So far that is not biting us, but it is a bit of “technical debt” in the implementation of knowledge. We do not care about duplicates, but we do have this glitch in it where we care about the most recent entries.

What we really need, I think, is a Knowledge that is more aware of having multiple facts at the same location, and that can return all the facts. This is going to be a hassle, because right now we are always assuming a single fact whenever we look.

So there is trouble brewing on the horizon. The fact that we’ve talked about it here doesn’t much reduce the likelihood that it will bite us, unless we make a card for it. So I will. OK, there’s an illegible sticky on my keyboard tray now, saying, let me look closely, “fix Knowldg fuv multifle faotg, vemore dups”. That should remind me to solve the problem.

Basically I’d score the day about a 9 out of 10. Decent code, nothing left worse than it was, and a nice new feature end to end.

See you next time!