This isn’t Houston, but we definitely have a problem. And it’s not a new one. And I’m tired of it.

Yesterday, when the Internet finally came back, I was creating the zip file for yesterday’s article, and played the game a bit. I was sure that I noticed invader bombs falling from empty columns and other odd places as well, offset from where the invaders were.

off

In the photo above, you can see two missiles falling on the far right, with no invaders above to have dropped them. This simply will not do. And it has happened before. I don’t remember whether I’ve written about it or not, and not inclined to go search the archives. I am sure I’ve seen the problem before, and I thought it was fixed, either with the bomb improvements back in #42, or perhaps later.

Be that as it may, it’s clearly not working now.

What I believe I’m seeing is that a column has to be clear for the defect to show up: that’s pretty obvious, since otherwise there’d be an alien to appear to drop the bomb. But in addition, it only seems to happen if the bottom row of aliens is gone. I’m not certain of that but it’s what I think I’ve observed.

We have a few tests relating to this capability. Three of them check the column-selection logic for targeted and untargeted bombs, and one checks the logic that finds the lowest live invader in a column, to start the bomb just below her.

        _:test("plunger bombs select correct columns", function()
            local theArmy = Army()
            local plungerBomb = theArmy.plungerBomb
            _:expect(plungerBomb:nextColumn()).is(0x01)
            _:expect(plungerBomb:nextColumn()).is(0x07)
            for i = 1,13 do
                plungerBomb:nextColumn()
            end
            _:expect(plungerBomb:nextColumn()).is(0x08)
            _:expect(plungerBomb:nextColumn()).is(0x01)
        end)
        
        _:test("squiggle bombs select correct columns", function()
            local theArmy = Army()
            local squiggleBomb = theArmy.squiggleBomb
            _:expect(squiggleBomb:nextColumn()).is(0x0b)
            _:expect(squiggleBomb:nextColumn()).is(0x01)
            _:expect(squiggleBomb:nextColumn()).is(0x06)
            _:expect(squiggleBomb:nextColumn()).is(0x03)
            for i = 1,16 do
                local col = squiggleBomb:nextColumn()
                _:expect(col).isnt(nil)
            end
        end)
        
        _:test("rolling shot finds correct column", function()
            local Gunner = Player()
            local army = Army(Gunner)
            local bomb = army.rollingBomb
            army:setPosition(vec2(10,180))
            Gunner.pos = vec2(10,32)
            local col = bomb:nextColumn(army)
            _:expect(col).is(1)
            Gunner.pos = vec2(26,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(2)
            Gunner.pos = vec2(30,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(2)
            Gunner.pos = vec2(91,32)
            col = bomb:nextColumn(army)
            _:expect(army:xPosition()).is(10)
            _:expect(col).is(6)
            Gunner.pos = vec2(0,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(1)
            Gunner.pos = vec2(208,32)
            col = bomb:nextColumn(army)
            _:expect(col).is(11)
        end)
        
        _:test("finding bottom-most invader", function()
            local army = Army()
            local y
            y = army:lowestInvaderY(1)
            _:expect(y).is(105)
            army:directKillInvader(1)
            _:expect(army:lowestInvaderY(1)).is(121)
            _:expect(army:lowestInvaderY(2)).is(105)
            army:directKillInvader(1 + 11)
            _:expect(army:lowestInvaderY(1)).is(137)
            army:directKillInvader(1 + 2*11)
            _:expect(army:lowestInvaderY(1)).is(153)
            army:directKillInvader(1 + 3*11)
            _:expect(army:lowestInvaderY(1)).is(169)
            army:directKillInvader(1 + 4*11)
            _:expect(army:lowestInvaderY(1)).is(nil)
        end)

I really don’t think the column-checkers matter (and I think the column selection is right), because the lowestInvaderY returns nil if there are no invaders in the column, and we do check that:

function Army:dropBomb(aBomb)
    if aBomb.alive then return end
    local col = aBomb:nextColumn(self)
    local y = self:lowestInvaderY(col)
    if y ~= nil then
        aBomb.pos = vec2(self:bombX(col), y - 16)
        self.bombCycle = 0
        aBomb.alive = true
    end
end

I have two objectives here. The first is to fix this defect once and for all. The second, less important, is to figure out what the defect is. I want to understand it, because that will give me more confidence that it’s fixed.

I’ve observed that there are multiple ways we access the row and column of invaders. And they are not all the same.

We store them like this:

    self.invaders = {}
    for row = 1,5 do
        for col = 11,1,-1 do
            local p = vec2(col*16, 185-row*16)
            table.insert(self.invaders, 1, Invader(p,vaders[row], scores[row], army))
        end
    end

In this logic, the last invader stored has row 5 and column 1. And because the insert goes ahead of position 1, that means that invader[1] is the bottom left, i.e. in column 1. In this reckoning, the rows are 1 through 5, starting from the top of the screen.

We check for lowest Y coordinate this way:

function Army:lowestInvaderY(col)
    for row = 0,4 do
        local index = col + 11*row
        if self.invaders[index].alive then
            print(col,index)
            return self.invaders[index].pos.y
        end
    end
    return nil
end

Here invader[1] is in what we’re calling here row zero, and that’s because we want to count upwards in Y and invader 1 has the lowest Y. We step by 11 to get the next row, because there are 11 invaders in each row. You can see a print there, that I was using to see if I could figure out where things were going wrong.

I’ll beef that print up a bit to try to learn more:

function Army:lowestInvaderY(col)
    for row = 0,4 do
        local index = col + 11*row
        if self.invaders[index].alive then
            print("found in col ", col,index)
            return self.invaders[index].pos.y
        end
    end
    print("none in ", col)
    return nil
end

left

In this picture we see a bomb falling far off to the left of the invaders’ leftmost column. I’m sure that is still column 1: I only killed off the rightmost column, and the whole bottom row. The print says:

found in col 1 12"

Which means that it has correctly found the 12th alien, as it should since #1 is not alive. And it has dropped the bomb in the wrong place. And I know what the defect is.

function Army:dropBomb(aBomb)
    if aBomb.alive then return end
    local col = aBomb:nextColumn(self)
    local y = self:lowestInvaderY(col)
    if y ~= nil then
        aBomb.pos = vec2(self:bombX(col), y - 16)
        self.bombCycle = 0
        aBomb.alive = true
    end
end

function Army:bombX(col)
    return 8 + self:xPosition() + (col-1)*16
end

We set the X coordinate of the bomb to the X coordinate of invader #1, plus an adjustment for the column. This would work, if the position of the first invader were correct. But it isn’t.

Dead invaders don’t update their position:

function Invader:update(motion, army)
    if not self.alive then return true end
    army:reportingForDuty()
    self.picture = (self.picture+1)%2
    self.pos = self.pos + motion
    Shield:checkForShieldDamage(self)
    if self:overEdge() then
        army:invaderOverEdge(self)
    end
    return false
end

Gotcha. Now I’m going for chai. See you in 30 mins.

Fix. But can we test it?

I’m back. Chai was free today, because *$ was updating their registers. Life is good.

The fix, I think, is simple and obvious. When we find a live invader in the column, we should return her position, not just her Y. Then we’ll have the exact coordinates beneath which to drop the bomb.

However, what about testing? Clearly this is an area that needs testing: it has been wrong more than once, and wrong for a rather long time. Perhaps as long as ten or more days.

For the defect to show up, we need to kill invader #1, and then we need all the invaders to have been incremented a ways along. That’s rather tricky, because Army:update only updates one live alien per iteration, to give that cool rippling effect:

function Army:update()
    self:updateBombCycle()
    self:possiblyDropBomb()
    local continue = true
    while(continue) do
        continue = self:nextInvader():update(self.motion, self)
    end
    self.rollingBomb:update(self)
    self.plungerBomb:update(self)
    self.squiggleBomb:update(self)
    self.saucer:update(self)
end

With that so embedded in the middle of things, it’s hard to get at it for our test. What if we were to extract that code as a separate method. We can do that safely, I think:

function Army:update()
    self:updateBombCycle()
    self:possiblyDropBomb()
    self:updateOneLiveInvader(self.motion)
    self.rollingBomb:update(self)
    self.plungerBomb:update(self)
    self.squiggleBomb:update(self)
    self.saucer:update(self)
end

function Army:updateOneLiveInvader(motion)
    local continue = true
    while(continue) do
        continue = self:nextInvader():update(motion, self)
    end
end

Note that I passed in the motion vector. Why? Well, the real reason is that I intend to override it in the test. It’s good for the game, however, because it can save a table access. Are you buying that? I’m not, not really. I did it for the test. I could have hammered motion in my test but this seemed better. Now for the test:

I got this far …

        _:test("bomb drops below bottom-most live invader", function()
            local army = Army()
            local x,y
            y = army:lowestInvader(1)
            x = army:
        end)

… and realized that Army has no methods to call to return the information I want to test. (This is probably also why it doesn’t quite work.

Here’s dropBomb again:

function Army:dropBomb(aBomb)
    if aBomb.alive then return end
    local col = aBomb:nextColumn(self)
    local y = self:lowestInvaderY(col)
    if y ~= nil then
        aBomb.pos = vec2(self:bombX(col), y - 16)
        self.bombCycle = 0
        aBomb.alive = true
    end
end

function Army:bombX(col)
    return 8 + self:xPosition() + (col-1)*16
end

What I want here is to get the position of the lowest invader in the column and set the bomb’s position to 16 below that. When we look at bombX, we see that it adjusts what it thinks is the correct value over by 8, to center the bomb under the invader. So we really want to get the invader’s position and offset it by vec2(8,-16).

Let’s TDD a new method, Army:lowestInvaderPosition(column) and then use it. I’d like to do it so that my test fails using the old scheme, but I’ll bypass that to get it right.

Here’s my test:

        _:test("bomb drops below bottom-most live invader", function()
            local army = Army()
            local v, vold
            vold = army:lowestInvaderPosition(1)
            army:directKillInvader(1)
            for inv = 1,55 do
                army:updateOneLiveInvader(vec2(16,0))
            end
            v = army:lowestInvaderPosition(1)
            _:expect(v.x).isnt(vold.x)
        end)

I fetch the lowest position in column 1, which will be the starting position. Saving that, I kill the first invader, then move everyone over by 16. The first invader won’t move. Then I look for the lowest invader in column 1 and expect his x not to be the same as the old one.

Now, for grins, I’ll implement that lowest position thing using my old technology:

function Army:lowestInvaderPosition(col)
    local y = self:lowestInvaderY(col)
    if y == nil then return nil end
    local x = self:bombX(col)
    return vec2(x,y)
end

This isn’t quite good but it should fail as expected.

21: bomb drops below bottom-most live invader  -- Actual: 24.0, Expected: 24.0

The 24,24 thing is because CodeaUnit isnt doesn’t return a very good message. Now let’s try to do this right.

function Army:lowestInvaderPosition(col)
    local v = self:lowestInvader(col)
    if v == nil then return nil end
    return v.pos + vec2(8,-16)
end

function Army:lowestInvader(col)
    for row = 0,4 do
        local index = col + 11*row
        if self.invaders[index].alive then
            return self.invaders[index]
        end
    end
    return nil
end

So let’s run that. I expect a pass. And I get it. Now can we make this test better?

        _:test("bomb drops below bottom-most live invader", function()
            local army = Army()
            local v, vold
            vold = army:lowestInvaderPosition(1)
            army:directKillInvader(1)
            for inv = 1,55 do
                army:updateOneLiveInvader(vec2(16,0))
            end
            v = army:lowestInvaderPosition(1)
            _:expect(v.x).is(vold.x + 16)
        end)

Yes! Just one more problem, this function isn’t lowestInvaderPosition, it is bombPosition. Renaming. Now we need to use it:

function Army:dropBomb(aBomb)
    if aBomb.alive then return end
    local col = aBomb:nextColumn(self)
    local y = self:lowestInvaderY(col)
    if y ~= nil then
        aBomb.pos = vec2(self:bombX(col), y - 16)
        self.bombCycle = 0
        aBomb.alive = true
    end
end

Becomes:

function Army:dropBomb(aBomb)
    if aBomb.alive then return end
    local col = aBomb:nextColumn(self)
    local bombPos = self:bombPosition(col)
    if bombPos ~= nil then
        aBomb.pos = bombPos
        self.bombCycle = 0
        aBomb.alive = true
    end
end

I expect the game to play correctly now. Hold on, I’ll check.

Whoa. Test failed. What happened?

20: finding bottom-most invader -- Tests:218: attempt to call a nil value (method 'lowestInvaderY')

Ah, someone using that old method, which I have removed.

        _:test("finding bottom-most invader", function()
            local army = Army()
            local y
            y = army:lowestInvaderY(1)
            _:expect(y).is(105)
            army:directKillInvader(1)
            _:expect(army:lowestInvaderY(1)).is(121)
            _:expect(army:lowestInvaderY(2)).is(105)
            army:directKillInvader(1 + 11)
            _:expect(army:lowestInvaderY(1)).is(137)
            army:directKillInvader(1 + 2*11)
            _:expect(army:lowestInvaderY(1)).is(153)
            army:directKillInvader(1 + 3*11)
            _:expect(army:lowestInvaderY(1)).is(169)
            army:directKillInvader(1 + 4*11)
            _:expect(army:lowestInvaderY(1)).is(nil)
        end)

I kind of like that test, because it cross-checks the subscripting logic, which is weird. Let’s create a function in the test that unfolds the position. It’ll be strange but should work.

        _:test("finding bottom-most invader", function()
            local army = Army()
            function lowestY(col)
                local p = army:bombPosition(col)
                if p == nil then return nil end
                return p.y + 16
            end
            local y
            y = lowestY(1)
            _:expect(y).is(105)
            army:directKillInvader(1)
            _:expect(lowestY(1)).is(121)
            _:expect(lowestY(2)).is(105)
            army:directKillInvader(1 + 11)
            _:expect(lowestY(1)).is(137)
            army:directKillInvader(1 + 2*11)
            _:expect(lowestY(1)).is(153)
            army:directKillInvader(1 + 3*11)
            _:expect(lowestY(1)).is(169)
            army:directKillInvader(1 + 4*11)
            _:expect(lowestY(1)).is(nil)
        end)

That works after a bit of tweaking that I’ll spare you. Now to see if the defect is gone. That will take a while as I destroy enough aliens to get the picture.

Yes! It’s visibly correct as well.

We are pleased. Commit: fix bomb x coordinates were wrong.

Let’s sum up.

Testing Is Important

That’s today’s big lesson: testing is indeed important, especially when what’s going on is tricky. What’s perhaps extra interesting is that I do have probably more lines of test around this issue than any other feature of the program … and it was still wrong. Why?

The proximate cause was that when I first started thinking about the bomb drop position, I was concerned about the Y coordinate, so I set out to compute it. Then it turned out that I needed the X coordinate also, no real surprise, so I computed it. I computed them in two different ways, with a shortcut for the X to save looking for the lowest invader again.

For some reason I didn’t think of returning the whole position of the lowest invader when I found her, nor even of having a find lowest invader in a column function. I believe that the reason was that the code that used the information had the two ideas, x and y, broken out, so I worked on y and then on x.

All that would have been OK, if not fine, had the code actually worked. But I didn’t ever really test “where is this bomb going to come from” directly. Instead I tested information leading to the capture of that information, and the information I tested all came up good.

And since I never moved the invaders during the test, the X calculation worked: it only fails if the invaders move after the first invader has shuffled off her mortal coil.

At this moment I don’t see a simple lesson to learn other than “when it’s complicated test it carefully”. Naturally, I already though I had done that. I was mistaken.

The weirdness of the code might have triggered an improvement at some point, but it wasn’t that weird, so I can imagine it would have been left alone, perhaps forever.

So today’s lesson is a harsh one, with no easy behavior change to improve. Testing matters.

See you next time!

Invaders.zip