The world is burning, and I’m really just trying to ignore reality, but in addition, I’ve made a mistake. Another mistake, that is.

Apologia

The world is burning. Authoritarianism is rising in a world where people yearn to be free, and some are way more free than others. Health care is a matter of how much money you have. What happens to you when you get stopped for a broken tail light depends on the color of your skin. Instead of coming together, it seems we’re not just falling apart, we’re tearing ourselves apart.

To try to avoid despair, I program. If you’re reading this, maybe you do as well.

Programming

Dave1707 of the Codea forum reported yesterday that the invaders program still doesn’t align correctly on his smaller iPad. At one point, I’m sure that it did. And now, it doesn’t.

Who “refactored” the scaling code for “clarity”, without any tests? This programmer.

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    rectMode(CORNER)
    spriteMode(CORNER)
    stroke(255)
    fill(255)
    local sc = math.min(HEIGHT/256, WIDTH/224)
    scale(sc)
    translate(WIDTH/(2*sc)-112,0)
    TheArmy:draw()
    self.player:draw()
    drawShields()
    drawStatus()
    popStyle()
    popMatrix()
end

This is the version where I went to a dynamic scaling factor, and both the scaling and translation worked fine. But it wasn’t clear.

I refactored to this:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    rectMode(CORNER)
    spriteMode(CORNER)
    stroke(255)
    fill(255)
    self:setUpScale()
    TheArmy:draw()
    self.player:draw()
    drawShields()
    drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:setUpScale()
    local sc = math.min(HEIGHT/256, WIDTH/224)
    scale(sc)
    translate(WIDTH/(2*sc)-112,0)
end

That broke out the scaling better. Then I did this:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    stroke(255)
    fill(255)
    self:setUpScaleAndOrigin()
    TheArmy:draw()
    self.player:draw()
    drawShields()
    drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:setUpScaleAndOrigin()
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(HEIGHT/256, WIDTH/224)
    scale(sc)
    translate(WIDTH/(2*sc)-112,0)
end

The argument was that setting to corner is the reason why we do the translate, and it should be there. OK, fine, still works. Then after several steps described in Space Invaders 48, I wound up with this:

function GameRunner:draw()
    pushMatrix()
    pushStyle()
    noSmooth()
    stroke(255)
    fill(255)
    self:scaleAndTranslate()
    TheArmy:draw()
    self.player:draw()
    drawShields()
    drawStatus()
    popStyle()
    popMatrix()
end

function GameRunner:scaleAndTranslate()
    local gameScaleX = 256
    local gameScaleY = 224
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(HEIGHT/gameScaleX, WIDTH/gameScaleY)
    scale(sc)
    translate(WIDTH/(2*sc)-gameScaleX/2,0)
end

That worked fine on my machine, and I had a little discussion about whether the exercise was worth it. I concluded that it was a good way to get some harmless practice, even when I went further in refactoring than wound up seeming right.

And then Dave finds that the code doesn’t work. It used to work. What’s wrong?

Well, I now know what’s wrong. The game width is 224 pixels and height is 256. Those are the values to which we’re scaling. However:

function GameRunner:scaleAndTranslate()
    local gameScaleX = 256
    local gameScaleY = 224

I don’t know how it is on your planet, but on mine width is X and height is Y. And those two values are reversed. What difference does it make? Well, the scale value turns out to be correct but this line doesn’t:

    translate(WIDTH/(2*sc)-gameScaleX/2,0)

Why? Because that subtrahend is supposed to be 112, i..e. 224/2, and it isn’t, it’s 128, because the variable names are reversed.

I’m famous for reversing width and height and have commented on it several times in these articles. I’m not sure if this is the same mind bug or not, but there it is.

How did this happen? Well, of course we could blame the programmer for not being careful enough. That would be accurate but incomplete. We could also consider that I was refactoring without the help of tests. I was careful, thinking hard, going in small steps, and I messed up nonetheless.

So today, I’m going to write some tests for this tiny function, which is just the kind of tricky function we might well expect to test, and then I’m going to re-fix the bug. To do that, we’re going to have to refactor some more.

Our current magic function is this:

function GameRunner:scaleAndTranslate()
    local gameScaleX = 256
    local gameScaleY = 224
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(HEIGHT/gameScaleX, WIDTH/gameScaleY)
    scale(sc)
    translate(WIDTH/(2*sc)-gameScaleX/2,0)
end

This “function” doesn’t return any results. It is probably possible to read out the system’s transformation matrix and see how we’ve set it, but that’s the sort of thing that keeps us from writing tests at all.

We have the scale value in a local sc. Let’s get the translate x value into tr by copy-paste:

function GameRunner:scaleAndTranslate()
    local gameScaleX = 256
    local gameScaleY = 224
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(HEIGHT/gameScaleX, WIDTH/gameScaleY)
    local tr = WIDTH/(2*sc)-gameScaleX/2
    scale(sc)
    translate(tr,0)
end

I’m sure that doesn’t change anything, since I did it with copy paste and anyway we’re going to test this. Now let’s extract the calculation part of this away from the setting part:

function GameRunner:scaleAndTranslate()
    local sc,tr = self:scaleAndTranslationValues()
    scale(sc)
    translate(tr,0)
end

function GameRunner:scaleAndTranslationValues()
    local gameScaleX = 256
    local gameScaleY = 224
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(HEIGHT/gameScaleX, WIDTH/gameScaleY)
    local tr = WIDTH/(2*sc)-gameScaleX/2
    return sc,tr
end

Lua can return multiple values from a function, so this should be just fine. Again, I copied and pasted. I can run the program to see that it works “on my machine”. It does. So far, so good.

Now we can almost test that function, but not quite. It doesn’t accept any parameters. So let’s pass in WIDTH and HEIGHT and use them inside as parameters:

function GameRunner:scaleAndTranslate()
    local sc,tr = self:scaleAndTranslationValues(WIDTH, HEIGHT)
    scale(sc)
    translate(tr,0)
end

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 256
    local gameScaleY = 224
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(H/gameScaleX, W/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

Now we can call that function with any values we like and check its results. Let’s try. For my first try, I put in my own iPad’s values and came up with the following. I haven’t run it yet:

        _:test("Screen scaling", function()
            local g = GameRunner()
            local sc,tr
            sc,tr = g:scaleAndTranslationValues(1366,1024)
            _:expect(sc).is(4)
            _:expect(tr).is((1366-4*224)/2)
        end)

I figure the tr value like this. Scale should be 4, because 1024/256 is 4. If scale is 4, and the game width is 224, then there should be 1366-4*224 pixels left over in the x direction, and we should translate over by half that value.

I expect this test to fail, because I know those two values are reversed. Let’s see if I’m right.

26: Screen scaling  -- Actual: 42.75, Expected: 235.0

Well that failed. I’m not sure about that number though. Let’s see … we “know” we’re using 256 instead of 224, but why is it so small? Oh … we translate after scaling, so we must divide our expected value by an additional 4 for the scale. The test is:

        _:test("Screen scaling", function()
            local g = GameRunner()
            local sc,tr
            sc,tr = g:scaleAndTranslationValues(1366,1024)
            _:expect(sc).is(4)
            _:expect(tr).is((1366-4*224)/(2*4))
        end)

Now we get this:

26: Screen scaling  -- Actual: 42.75, Expected: 58.75

I’m a bit surprised by the 0.75 part. Let’s not pretend that we don’t know what the bug is, so let’s change the code to be what we think is right:

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 224
    local gameScaleY = 256
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(H/gameScaleX, W/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

Now when we run the test … wow.

26: Screen scaling  -- Actual: 4.5714285714286, Expected: 4
26: Screen scaling  -- Actual: 37.40625, Expected: 58.75

Now I am surprised. Oh, look, there’s another bug:

    local sc = math.min(H/gameScaleX, W/gameScaleY)

Those are reversed, which was compensating in scale for the reversal of the other two, but translate was not compensated. Wow, two cancelling bugs! You rarely see that in the wild, I hope.

Fixing that:

function GameRunner:scaleAndTranslationValues(W,H)
    local gameScaleX = 224
    local gameScaleY = 256
    rectMode(CORNER)
    spriteMode(CORNER)
    local sc = math.min(W/gameScaleX, H/gameScaleY)
    local tr = W/(2*sc)-gameScaleX/2
    return sc,tr
end

26: Screen scaling  -- OK
26: Screen scaling  -- OK

So that’s nice. But let’s do at least one more test, Dave’s iPad Air. Dave has 834x1112 on his iPad. So let’s see if we can do the math on that:

834/224 = 3.72ish
1112/256 = 4.34ish

So the scale will be the lesser of these:

        _:test("Screen scaling", function()
            local g = GameRunner()
            local sc,tr
            sc,tr = g:scaleAndTranslationValues(1366,1024)
            _:expect(sc).is(4)
            _:expect(tr).is((1366-4*224)/(2*4))
            sc,tr = g:scaleAndTranslationValues(834,1112)
            _:expect(sc).is(3.72, 0.01)
        end)

And that runs. Now since the x was the constraint, don’t we expect tr to be zero or thereabouts? I think so. Let’s try that.

        _:test("Screen scaling", function()
            local g = GameRunner()
            local sc,tr
            sc,tr = g:scaleAndTranslationValues(1366,1024)
            _:expect(sc).is(4)
            _:expect(tr).is((1366-4*224)/(2*4))
            sc,tr = g:scaleAndTranslationValues(834,1112)
            _:expect(sc).is(3.72, 0.01)
            _:expect(tr).is(0)
        end)

And that passes, how nice. Shall we do another, or are we convinced?

It’s Saturday, the sky is falling, and while another test might add a bit of confidence, I’m quite sure this is finally right. I’m stopping now, save for the inevitable summing up.

Summing Up

The lesson here should be pretty darn clear.

I started with a scaling calculation that was right only for my machine. When I generalized it, I managed to get it right, but I never tested different values even then, at least not with automated tests. Offline, I did draw some rectangles and eyeball whether I was inside them. But no real tests.

So when I saw, correctly, that that code needed to be more expressive, I went through a few phases of careful refactoring. Finally, I provided more meaningful names for the constants, and in so doing, reversed their meaning. Somehow I managed to un-reverse them in the scaling calculation, so I never noticed that they were wrong.

The calculation in question is tiny but tricky. I’d wager that I’m not the only person who could get it wrong. It deserved a test. I don’t remember thinking about testing but if I had I’d have been daunted by the fact that the function just sets scale and translate, returning no results.

It was easy enough to break out the calculation, but it took a bit of motivation to do it. By the time I did that I was fully convinced that I needed tests, if only to save face.

A couple of tests, and the defects were isolated and fixed. Along the way, that other reversal was found. I sent Dave a patch yesterday, and I’m not at all sure if it was right or not.

This, I’m rather confident in.

Commit: scaling fix with tests.

Don’t do like me, brothers and sisters. When the code is a bit tricky, test it.

See you next time!

Invaders.zip