Let’s keep on drilling. Real soon now, we’ll have hexes in the game.

It’s 1919 local, Tuesday evening, I’m on the Friday Night Coding Slack, and I’m going to do a bit of work on the D3 spike. I think the plan is to refactor so that there’s only one Direction class, to be used in either kind of map.

Here’s Direction:

HexDirection = class()

function HexDirection:init(x,y,z)
    self.x = x
    self.y = y
    self.z = z
end

function HexDirection:direction(n)
    -- n = 1-6
    return HexDirection:directions()[n]
end

function HexDirection:directions()
    return{ HexDirection(1,-1,0), HexDirection(1,0,-1), HexDirection(0,1,-1),HexDirection(-1,1,0), HexDirection(-1,0,1), HexDirection(0,-1,1) }
end

CartesianDirection = class()

function CartesianDirection:init(x,z)
    self.xDir = x
    self.yDir = z
end

function CartesianDirection:__tostring()
    return "CartesianDirection("..self.xDir..","..self.yDir..")"
end

I’ll start from HexDirection, because it has the right number of coordinates. I wind up with this, including some speculation about the literals we’ll need.

Direction = class()

function Direction:init(x,y,z)
    self.x = x
    self.y = y
    self.z = z
end

function Direction:hexDirection(n)
    assert(1 <= n and n <= 6, "Impossible hex direction "..n)
    return Direction:hexDirections()[n]
end

function Direction:cartesianDirection(n)
    assert(1 <= n and n <= 4, "Impossible cartesian direction "..n)
    return Direction:cartesianDirections(n)
end

function Direction:hexDirections()
    return{ Direction(1,-1,0), Direction(1,0,-1), Direction(0,1,-1),Direction(-1,1,0), Direction(-1,0,1), Direction(0,-1,1) }
end

function Direction:cartesianDirections()
    return {Direction(1,0,0), Direction(0,0,1), Direction(-1,0,0), Direction(0,0,-1)}
end

Tests are all green, as one would expect. But we have no tests for the use of directions. Must remember to do that. Anyway, commit: Direction class created, HexDirection removed.

I should be removing some classes. We don’t really need HexMap, CartesianMap, and CartesianDirection. Let’s remove those and see what happens.

Removing CartesianMap (but not Square class), tests all run. Commit: Remove CartesianMap class.

Point

We want to wind up with a single Point class. Aside from tests, we only use the two Point classes here:

function Map.hex(x,z)
    return HexPoint(x,-x-z,z)
end

function Map.cartesian(x,z)
    return HexPoint(x,z)
end

We do have one issue (at least one), which is that we want to impose constraints on the HexPoint:

function HexPoint:init(x,y,z)
    self._x = x
    self._y = y
    self._z = z
    if self:invalid() then error("Invalid HexCoordinates "..x..","..y..","..z) end
    self.keyString = x..","..y..","..z
end

function HexPoint:invalid()
    return 0 ~= self:x() + self:y() + self:z()
end

We can certainly just turn off the check, because our own methods above do the right thing. Let’s try that. We may want to do something to protect Points from end users, but there may in fact be no point to creating your own. As it were.

I think what we need is two creation methods, Point:hex() and Point:cartesian(), where the necessary checking it done, and then dropping into init. But I’d like to make it impossible to create an unchecked hex point.

Chet asked me “is anyone else ever going to work on this program?”, implying that I should just not do it wrong. I think he’s right. Let’s try this:

function HexPoint:hex(x,y,z)
    local pt = HexPoint(x,y,z)
    if pt:invalid() then error("Invalid HexCoordinates "..x..","..y..","..z) end
    return pt
end

function HexPoint:cartesian(x,z)
    return HexPoint(x,0,z)
end

function HexPoint:init(x,y,z)
    self._x = x
    self._y = y
    self._z = z
    self.keyString = x..","..y..","..z
end

We need to change what the Map does:

function Map.hex(x,z)
    return HexPoint:hex(x,-x-z,z)
end

function Map.cartesian(x,z)
    return HexPoint:cartesian(x,z)
end

Run the tests. I expect either that they’ll pass, or they’ll discover a reference to HexPoint that doesn’t go through the class method. I’m too lazy to look.

1: HexPoint Creation  -- Actual: nothing thrown, Expected: Invalid HexCoordinates 1,1,1

Ha, see? Vindicated!

        _:test("HexPoint Creation", function()
            local coord = HexPoint:hex(0,0,0)
            _:expect(coord:invalid()).is(false)
            local f = function() HexPoint:hex(1,1,1) end
            _:expect(f).throws("Invalid HexCoordinates 1,1,1")
        end)

Fixed that, but I found the next few tests need fixing as well.

OK, here’s an idea:

function HexPoint:hex(x,y,z)
    local pt = HexPoint(x,y,z, true)
    if pt:invalid() then error("Invalid HexCoordinates "..x..","..y..","..z) end
    return pt
end

function HexPoint:cartesian(x,z)
    return HexPoint(x,0,z, true)
end

function HexPoint:init(x,y,z, secret)
    assert(secret, "Invalid call to HexPoint()")
    self._x = x
    self._y = y
    self._z = z
    self.keyString = x..","..y..","..z
end

I’ll add a test for that, but that should catch lots of other perps.

        _:test("HexPoint Creation", function()
            local coord = HexPoint:hex(0,0,0)
            _:expect(coord:invalid()).is(false)
            local f = function() HexPoint:hex(1,1,1) end
            _:expect(f).throws("Invalid HexCoordinates 1,1,1")
            f = function() HexPoint(1,0,-1) end
            _:expect(f).throws("Invalid call to HexPoint()")
        end)

Now to tick thru the tests failing. Easily done, just tedious. I’m going to break for the evening. Commit: HexPoint can be used for hex and cartesian. CartesianPoint still remains.

I’ll pick up here tomorrow AM …

0930 Wednesday

OK, here we are. Are we there yet? Almost. I think we can get rid of CartesianPoint now, with everyone going through the HexPoint creation methods hex and cartesian.

We need to find and rewire all our tests and any other code that may refer to CartesianPoint. Let’s just do it:

        _:test("CartesianPoint Arithmetic", function()
            local point = CartesianPoint(2,3)
            local notDir = CartesianPoint(1,1)
            local f = function() return point + notDir end
            _:expect(f).throws("Attempt to add a CartesianPoint plus a non-CartesianDirection")
            local dir = CartesianDirection(1,2)
            local result = point + dir
            _:expect(result).is(CartesianPoint(3,5))
        end)

I try this:

        _:test("Cartesian Point Arithmetic", function()
            local point = HexPoint:cartesian(2,3)
            local notDir = HexPoint:cartesian(1,1)
            local f = function() return point + notDir end
            _:expect(f).throws("Attempt to add a HexPoint plus a non-Direction")
            local dir = Direction:cartesianDirection(1)
            local result = point + dir
            _:expect(result).is(HexPoint:cartesian(3,3))
        end)

I get one OK for the test and then this:

3: Cartesian Point Arithmetic -- HexPoint:46: attempt to call a nil value (method 'is_a')

The code:

function HexPoint:__add(aDirection)
    if aDirection == nil then error("nil aHexPoint") end
    if aDirection:is_a(Direction) then
        -- subverts the true check.
        return HexPoint(self:x()+aDirection.x, self:y()+aDirection.y, self:z()+aDirection.z, true)
    else
        error("Attempt to add a HexPoint plus a non-Direction")
    end
end

Hm. That sure looks right to me. Putting a print after the creation of dir prints a table. Don’t Directions print with tostring? I suspect I’ve done something bad.

function Direction:cartesianDirection(n)
    assert(1 <= n and n <= 4, "Impossible cartesian direction "..n)
    return Direction:cartesianDirections(n)
end

Yes indeed. That should say:

function Direction:cartesianDirection(n)
    assert(1 <= n and n <= 4, "Impossible cartesian direction "..n)
    return Direction:cartesianDirections()[n]
end

Tests are green. Commit: converted one test to centralized use of HexPoint and Direction.

I want to mention that much of the joy of having reasonable TDD tests comes when doing changes like these. The tests turn up little mistakes that are otherwise hard to notice. Let’s do the next:

        _:test("CartesianPoint key", function()
            local pt = CartesianPoint(1,1)
            _:expect(pt.keyString, "string").is("1,1")
            _:expect(pt:key(), "key").is("1,1")
        end)

Well, that’ll be wrong as soon as we plug in the other point type. Test should be:

        _:test("Cartesian Point key", function()
            local pt = HexPoint:cartesian(1,2)
            _:expect(pt:key(), "key").is("1,0,2")
        end)

I see no reason to test the member variable, given that the method returns the right answer.

The only references now to CartesianPoint are in that tab and in Square. Let’s check Square to see what it has to say for itself.

Square = class()

function Square:init(aCartesianPoint)
    self.cartesianPoint = aCartesianPoint
end

function Square:point()
    return self.cartesianPoint
end

No problem, we’ll leave it. We should be able to remove that whole tab, but first commit: fixed last test referring to CartesianPoint.

Now delete that tab and test. Tests run, picture looks good. Commit: remove CartesianPoint class.

There are no references to HexMap. Remove that tab. Test. We’re good. Commit: remove HexMap class.

Regroup

OK, we’re down to just about the level we were aiming for. Let’s sit back a few degrees and see what’s next.

HexPoint -> MapPoint
We should rename HexPoint. I was considering just Point, but that’s such a generic name that it should probably be reserved for something all mathematical. MapPoint seems a good compromise.
Point Creation
I’ve done that semi-clever trick of a “secret” parameter to be passed in when you create a point, which allows the methods hex and cartesian to do any unique validation they may wish, and then have their decisions respected. The main benefit of that is that it will prevent me from accidentally typing MapPoint(3,5) when I should have said MapPoint:cartesian(3,5).

But I think I see a better way to do it. Someone asked last night, during the Friday Coding zoom meeting, whether Lua has private classes. We have that ability. Might try that.

Square and Hex
Those two classes are just helper classes for testing purposes. I think they should be moved to the Test tab, just to keep the tab count down. Done, commit: moved Square and Hex to Test tab.

OK, let’s do some of that. I’ll rename HexPoint to MapPoint. I’ve learned again not to trust Codea’s replace, so I’ll paste over to Sublime to do the job.

I think we should probably rename Direction to MapDirection, for consistency. Sure, why not.

That goes fairly smoothly. Tests find the cases I missed. Commit: Rename classes to MapPoint and MapDirection

Let’s skip the private classes idea, but I’ll mention here what it is.

We could have a public class MapPoint, with just two class methods, hex and cartesian, and with an assert in init so that you can’t create instances. Those two methods would create instances of the “real” point, which would be defined in a class local to that tab:

local ActualPoint = class()

Then no one else could create an Actual point, just our methods. Could be a good way to be extra safe if one were feeling paranoid.1

Let’s sum up and get outa here.

Summary

Subject to me not coming up with another even more brilliant idea, I think we’re ready to plug our new map object into the Dung program. We wind up with just three classes, Map, MapPoint, and MapDirection, just under 200 lines of code, counting whitespace. It’s likely that some of those lines aren’t needed, but I think it’s pretty close to just what is required.

We’ve definitely reduced the number of classes with a substantial net reduction in the total lines of code. Unfortunately, if there’s a way to get the summary counts from WorkingCopy, I don’t know it. I’d guess we’ve reduced the code to about half of what it was at its peak.

I think we’re ready to plug these new classes into the Dung program, and I expect it to go fairly easily but not trivially. We do have code in Dung that refer to x’s and y’s, and to vec2 instances, and such. These are design flaws in the existing program. We know better than to use primitive types for this sort of thing, but in the heat of the moment, they seem good enough.

I do feel good about the impossible demand to convert our game to use both hexagonal and square maps. I think that by the end of next week, we should be dealing with creating a bit of art to make our hexes look like something.

There will be some fancy work needed there, because the decisions about how to draw things will depend on the state of the Map but will have impact way down in the drawing logic.

That reminds me of something.

I think we should do something about the way we create different types of Maps. Remember that flag we use?

function Map:init(xWidth, zHeight, mapType, klass)
    assert(mapType == Map.cartesian or mapType == Map.hex, "MapType must be cartesian or hex")
    self.mapType = mapType
    self:createRectangle(xWidth, zHeight, klass)
end

We should have two class methods to do the job, something like Map:hexMap(...) and Map:cartesianMap(...). And I think we’re going to get tired of typing cartesian, by the way. Would “hex” and “square” be OK with everyone? We’ll think about that.

I also think that the magic parameter there, which is presently just a sort of class method, should be made into a couple of small strategy classes.

Stop …

There I go again, making it better. But this is, in a way, our last change to make this part of the system as good as it can be, at least within reason.

I’m going to write and use the two class methods, and perhaps hide the magic parameter better.

Map = class()

local function hex(x,z)
    return MapPoint:hex(x,-x-z,z)
end

local function cartesian(x,z)
    return MapPoint:cartesian(x,z)
end

function Map:hexMap(xWidth, zHeight, klass)
    return Map(xWidth, zHeight, hex, klass)
end

function Map:cartesianMap(xWidth, zHeight, klass)
    return Map(xWidth, zHeight, cartesian, klass)
end

function Map:init(xWidth, zHeight, mapType, klass)
    assert(mapType == cartesian or mapType == hex, "MapType must be cartesian or hex")
    self.mapType = mapType
    self:createRectangle(xWidth, zHeight, klass)
end

The two local functions needed to be defined before the class, so that they’re sufficiently defined when referenced. I still think we’ll want then to be two little classes. That will probably happen when we get around to drawing. I also expect that we’ll come back here and lift some drawing code from the old Hex class, which is now lost to time and tides.

Commit: New hexMap and cartesianMap creation methods on Map.

Bottom line for the day: I think we’re ready to do this. Pending new brilliance, we’ll start wring this in tomorrow.

I hope you’ll join me then.

  1. What’s that noise? ↩