Dungeon 8
Feature or improved code? Maybe a feature spike? Who knows, not I.
Today’s menu of options includes:
- Improve the performance of the neighbor-checking code, using the hash part of a Lua table rather than the array part;
- Figure out exact door placement, which relates to the overlap that we check when we look for neighbors;
- Try drawing an expanded room, to get a feeling for how we might do that.
Let’s do the hash table thing first. It shouldn’t be too hard, might be a good thing to warm up on at 0703 on this lovely warm Autumn morning.
Lua’s only actual data structure is the table. A table has two internal aspects that can be used in two smoothly-cooperating ways. It can be used as an array, with integer subscripts ranging from 1 upward. It manages array insertion and deletion neatly when you use it that way, all very nice and tidy. And the table can be used as an associative store, a hash table. Or both.
You could readily use table insertion to stuff items into the table in array style, meanwhile accessing it in the associative fashion.
It sounds weirder than it is, because in practice I’d use any given table one way or the other but generally not both. Doing otherwise seems to me to be likely to be confusing to the eventual reader of the code, i.e. me and possibly you.
Right now, the Neighbors are stuffed into a table array, like this:
function Room:insertNewNeighbor(neighbors, neighbor)
if not self:neighborPresent(neighbors,neighbor) then
table.insert(neighbors,neighbor)
end
end
The table.insert call appends the provided item at the end of the array. Elsewhere we process the array in order, here:
for i,l in ipairs(Neighbors) do
l:draw()
end
The ipairs function produces all the array elements, paired with their integer subscript. It’s implemented as a neat co-routine kind of thing, works like you’d expect. Tables also have a pairs function. Where ipairs produces only the contiguous elements stored at integer locations, pairs produces all the elements, whether stored at integer locations or as key-value pairs.
This gives me a useful opportunity since I’m about to work on this table. Right now, the code above draws the Neighbors in subscript order. They each draw one of those red or yellow connecting lines between neighboring rooms. But we can let them draw in any order, so I can change this call right now to pairs.
No change to the map. But this supports my larger plan, which is to treat the Neighbors table as a key-value, hashed table, rather than an array. To let me do that, every Neighbor needs to have a unique key. I propose to use this:
function Neighbor:key()
return self.room1.number*1000 + self.room2.number
end
Since we’ve made sure that Neighbors are always created with room1’s number less than room2’s, this key should be unique.
Now we can do this:
function Room:insertNewNeighbor(neighbors, neighbor)
if not self:neighborPresent(neighbors,neighbor) then
neighbors[neighbor:key()] = neighbor
end
end
And to make the presence check work, we can just switch to using pairs, though we won’t stop there.
function Room:neighborPresent(neighbors,neighbor)
for i,n in pairs(neighbors) do
if n:matches(neighbor) then return true end
end
return false
end
I expect everything to work just fine. And it seems to. (Pardon those red tests, I haven’t gotten around to fixing them. Yes, I’m a bad person.)
Now, I can remove the for loop:
function Room:neighborPresent(neighbors,neighbor)
return neighbors[neighbor:key()] ~= nil
end
I’m going to be very disappointed if this fails, and start questioning my reasoning and wondering if I need even more tests. But we do have a test running on this subject. Here goes:
Seems good but I’d best make that test work and fix whatever the other one is as well:
16: primary and secondary paths -- Actual: 0, Expected: 3
16: primary and secondary paths -- Tests:187: attempt to index a nil value (local 'n12')
Here’s that test:
_:test("primary and secondary paths", function()
local r1 = Room:fromCorners(200,200,300,300)
r1.number = 1
_:expect(r1.h).is(100)
local r2 = Room:fromCorners(200,100, 250,199)
r2.number = 2
_:expect(r2.h).is(99)
local r3 = Room:fromCorners(251,100, 300,199)
r3.number = 3
_:expect(r3.h).is(99)
local rooms = {r1,r2,r3}
local col = color(255,0,0)
Neighbors = {}
r1:colorNeighborsIn(rooms, col)
_:expect(#Neighbors).is(3)
local n12 = Neighbors[1]
local n23 = Neighbors[2]
local n13 = Neighbors[3]
_:expect(n12.room1.number).is(1)
_:expect(n12.room2.number).is(2)
_:expect(n12.isPrimary).is(true)
_:expect(n23.room1.number).is(2)
_:expect(n23.room2.number).is(3)
_:expect(n23.isPrimary).is(true)
_:expect(n13.room1.number).is(1)
_:expect(n13.room2.number).is(3)
_:expect(n13.isPrimary).is(false)
end)
The first expectation, on #Neighbors, is relying on the array aspect of the Neighbors table, which we don’t use any more. The # function gives the length of the array part only. We’d better check that. I’ll create a little function to do it:
function count(aTable)
local n = 0
for k,v in pairs(aTable) do
n = n + 1
end
return n
end
And called thus:
_:expect(count(Neighbors)).is(3)
This should fix the first error. Hm. Something about count being a global. Changed the name to tableCount and we’re good. Moving right along to the other error:
~~~lua16: primary and secondary paths – Tests:187: attempt to index a nil value (local ‘n12’)
This is due to the fact that we can't access the neighbors by integer scripts any more. But we do have the ability to access them by their keys. Again a helper function:
~~~lua
function neighborKeyOf(room1, room2)
local n = Neighbor(room1,room2,false)
return n:key()
end
I’ll just create a new neighbor instance and fetch its key. Now I can write this:
local n12 = Neighbors[neighborKeyOf(r1,r2)]
local n23 = Neighbors[neighborKeyOf(r2,r3)]
local n13 = Neighbors[neighborKeyOf(r1,r3)]
I do expect and hope that this will work. And it does. Now we can delete those checks on room number, since none of this would work if they weren’t right:
_:test("primary and secondary paths", function()
local r1 = Room:fromCorners(200,200,300,300)
r1.number = 1
local r2 = Room:fromCorners(200,100, 250,199)
r2.number = 2
local r3 = Room:fromCorners(251,100, 300,199)
r3.number = 3
local rooms = {r1,r2,r3}
local col = color(255,0,0)
Neighbors = {}
r1:colorNeighborsIn(rooms, col)
_:expect(tableCount(Neighbors)).is(3)
local n12 = Neighbors[neighborKeyOf(r1,r2)]
local n23 = Neighbors[neighborKeyOf(r2,r3)]
local n13 = Neighbors[neighborKeyOf(r1,r3)]
_:expect(n12.isPrimary).is(true)
_:expect(n23.isPrimary).is(true)
_:expect(n13.isPrimary).is(false)
end)
I also removed the height checks, which were there to assure me that my mental arithmetic was working.
So we have converted the neighbors function from an array that was searched for each new insertion to a hash table that just experiences one probe per insertion. The speed up is … undetectable. But it’s a better implementation speed not withstanding, because it’s simpler.
Commit: neighbors converted to hash table.
We should probably deal with the feature envy while we’re at it. All the managing of the neighbors table seems to be in Room, except for the initial triggering of the path search in Main:
Rooms[1]:colorNeighborsIn(Rooms, color(255,0,0,100))
I think that the coloring decision and the decisions about whether rooms are close enough together to connect, does belong in Room. But the management of the neighbors table seems to me to be either a class-side function of Neighbor, or an occasion for a new class, maybe NeighborTable.
I’m not sure, though. Historically I’ve kept tables like these in globals, until that begins to irritate me, and then I’ve typically stored them either on the class side of the obvious class (like putting the Neighbors table in Neighbor), or, occasionally, just grouping the global tables in some top level object like the GameRunner of the Space Invaders program.
If we didn’t have to draw the lines, the neighbors table could be local to the Room class, although not quite a member variable, it’d still need to be accessible from any member. And drawing the lines is a nicety, done for our own enlightenment and development convenience. It’s not really a user feature.
Be all that as it may, I’m not feeling enough pressure on this to make a change. For now, we’ll leave Neighbors as a global, until we better understand what it is and where it belongs.
What now?
I think figuring out door placement is the thing to do next, since when we display an expanded room, we’ll want to have the doors marked. Our connection-finding logic checks to see that potentially adjacent walls are close enough together, and that they have enough overlap to allow for a door. Right now, that overlap is 10 pixels on the small map, if I’m not mistaken.
Rooms are created like this:
function Room:init(x,y,w,h)
self.number = 666
self.color = color(0,255,0,25)
self.x = x or math.random(WIDTH//4,3*WIDTH//4)
self.y = y or math.random(HEIGHT//4,3*HEIGHT//4)
self.w = w or math.random(WIDTH//40,WIDTH//20)*2
self.h = h or math.random(HEIGHT//40,WIDTH//20)*2
end
Their width and height is limited to between 1/20th and 1/10th of screen size, and is forced to be even. (I got tired of fractional boundaries. On an iPad screen, the low value is (68,50) and the high is twice that, or (176,100). I’m feeling that maybe those values need to be more constrained, depending on how the rooms are drawn in expanded mode, where they’ll presumably be tiled with some kind of flooring textures, as these games usually consider the room to be made up of cells. So possibly divisibility by 4 or something would be better than by 2.
I suspect this is just another one of those things that you worry about, think a bit, and then usually decide to defer, and sometimes decide to do a little work or experiment just to increase confidence. Here, I have no doubt that the room size can be constrained in any reasonable way without breaking anything we already have in place, so I guess I’ll just get over it.
But where shall we place our doors? Given two rooms overlapping, and given that they overlap enough to leave room for a door, or we wouldn’t consider them connected, we can always safely place the door at the center of the overlapped area in each room.
It occurs to me that if we only draw one room at a time, so that when they go through a door the new room draws, we may wish to align the two rooms so that the player’s position on the screen doesn’t change. Another thing not to worry about just now. We might even display more of the map anyway, scrolling around in the parts they’ve seen. Who knows, that’s for another day.
Overlap. Doors. I think that from a design viewpoint, a room “has” doors. Each door will need a position along one of the four walls (We have no non-rectangular rooms. If we need them, it’s going to hurt. No matter. If it hurts, I won’t do it.) Each door has some kind of existence in two rooms, the one you’re in, and the one you’re going to. And it will have a different position in each room. It’ll be on the opposite wall, and could be positioned pretty much anywhere along that wall.
I hadn’t considered it before, and I’m not sure this is a great idea, but the Neighbor object is a good candidate for a Door. It already knows the two rooms that it connects. If when we create a Neighbor, we could give it to the two rooms involved, and they’d each hold on to it. Either the door/neighbor would hold the door coordinates or the room would. I guess the door has to, as there are lots of potential doors.
(That makes me think of a special kind of room. Imagine a room studded with doors, each one leading to a small chamber containing evil monsters and great treasure. We could imagine that occurring now by chance, but it’s quite unlikely. We could build one if we want to. Make a card for that, maybe we’ll do it someday.)
OK, I’ve convinced myself that the neighbors thing is a good candidate for being a door. Each door basically has two sides, the room 1 side and the room 2 side, and for each side we know the wall coordinates of the door, in some form we find useful.
I think what I’d like to have happen during development is for the doors to appear as some simple graphical element in the big map, a white rectangle, perhaps, positioned where the door should be. Since there are two aspects to every door, we should draw them separately, which will highlight the error when we inevitably get it wrong.
Right now, we search for connections like this:
function Room:colorNeighborsIn(rooms, aColor)
self:setColor(aColor)
for i,r in ipairs(rooms) do
if r:isNeighbor(self) then
if r:getColor() ~= aColor then
self:insertNewNeighbor(Neighbors, Neighbor:primary(self, r))
r:colorNeighborsIn(rooms, aColor)
else
self:insertNewNeighbor(Neighbors, Neighbor:secondary(self, r))
end
end
end
end
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
return (self:nsCloseEnough(aRoom) and self:xOverlap(aRoom)>doorWidth) or
(self:ewCloseEnough(aRoom) and self:yOverlap(aRoom)>doorWidth)
end
function Room:ewCloseEnough(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (hisW>=myE and hisW-myE<ce) or (myW>=hisE and myW-hisE<ce)
end
function Room:nsCloseEnough(aRoom)
local ce = Room:closeEnough()
local hisN = aRoom:north()
local hisS = aRoom:south()
local myN = self:north()
local myS = self:south()
return (hisS>=myN and hisS-myN<ce) or (myS>=hisN and myS-hisN<ce)
end
At the time I created this1, the north-south check and the east-west check seemed to belong together. But for our actual doors, we’ll need to know which actual wall the door is on. We’ll need to break this up, no doubt.
When I create new structures like this, I am almost immediately consumed by the process of creating them. How am I going to tease apart the NSEW stuff, and so on. But I try to remember that the real need is for the new structure to be useful to the part of the code that uses it. We only create these doors once, and we use them often. So let’s think about how they’ll probably be used.
We’re in some room R. We draw the floor, and the walls around the outside. Now we want to draw a door everywhere there should be one. We have a collection of our doors, which look a lot like Neighbor instances with other cool stuff in them. (We could have just our own door info, broken out from the linking Neighbor instances. I think that can only lead to rooms that should have related info getting unrelated info owing to some defect. I want to keep both sides of the door together in a Neighbor-like object, because they belong together.)
So we’ll loop over our doors, drawing a door, with code that’ll look sort of like this:
for k,door in pairs(self.doors) do
x,y = getDoorInfo(self,door)
drawDoor(x,y)
end
We’ll pass ourself in, because the Neighbor-Door instance knows its two rooms, so it can use whichever room we are to give us the info we want, which is mostly the x and y of the door. (We’ll want rotation too, as we’ll see when we draw this, but we can ignore that concern for now.)
Now when the rooms are measured for being neighbors, their coordinates are in the form of the big map. So it’ll be natural for the Neighbor-Door to have those coordinates, and part of the yet-to-be-written “expand room” logic will need to convert those coordinates to expanded. That should be little more than a multiply and a subtract. (Unless I miss my guess, in which case I’ll be writing a paragraph with “I” in it in the future.)
So I think I want x and y above to be such that the value is exactly the midpoint of the overlap between the rooms, as applied to that wall. Now it happens that we really only detect neighboring cells once, if the recursion is working as it’s supposed to. And the overlap is different on each room. We check that like this:
function Room:xOverlap(aRoom)
sLo,xx,sHi,xx = unpack(self:corners())
rLo,xx,rHi,xx = unpack(aRoom:corners())
return max(min(sHi,rHi)-max(sLo,rLo),0)
end
function Room:yOverlap(aRoom)
xx,sLo,xx,sHi = unpack(self:corners())
xx,rLo,xx,rHi = unpack(aRoom:corners())
return max(min(sHi,rHi)-max(sLo,rLo),0)
end
Those are really too cryptic to think about, aren’t they? Ah, but wait! I’m wrong! The overlap isn’t different for each room, not while they are in the big map.
The x coordinates of the overlap above are the same. The y coordinates are different, they’re the corresponding wall positions. But the hard part, the center of the span, is the same. Woot!
So let’s imagine that our Neighbor can return, for each door and a given room, the x,y coordinates of door center, which will be the center of the overlap as applied to that room.
I think where I’d like to wind up is somewhere like this: the code here finds and colors connections:
function Room:colorNeighborsIn(rooms, aColor)
self:setColor(aColor)
for i,r in ipairs(rooms) do
if r:isNeighbor(self) then
if r:getColor() ~= aColor then
self:insertNewNeighbor(Neighbors, Neighbor:primary(self, r))
r:colorNeighborsIn(rooms, aColor)
else
self:insertNewNeighbor(Neighbors, Neighbor:secondary(self, r))
end
end
end
end
If when it finds a connection, that is at the location of the two calls to insertNewNeighbor, it has found a door. So, the room should add that new neighbor to its own collection of doors. And the neighbor should contain all the information needed to draw the door.
I’m thinking of cute ways to change this recursion around to handle this, for example by replacing the if r:isNeighbor(self) with a call to Neighbor, passing ‘self’ and ‘r’, such that Neighbor would return the neighbor if it already has one, and a new one if the rooms are adjacent, and a nil or a fake neighbor if they’re not. I’m not sure about how that would shape up.
Let’s proceed a bit differently. We have a number of tests for adjacency, that test the overlap calculation and the close enough calculation independently. We can scavenge from those if we need to. Let’s TDD some new capability on Neighbor:
- Given two rooms, Neighbor:fromRooms(r1,r2) returns a Neighbor object including all the necessary door information for both rooms, capable of returning x,y as described above, if the rooms are adjacent by our current rules. If they aren’t … it returns nil.
I don’t like returning nil, and maybe we’ll do something different, but for now, that’ll do for some tests.
Now we can write tests expressing what we expect. We’ll build up our expectations slowly, of course.
_:test("Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
_:expect(r1:yOverlap(r2)).is(20)
end)
Here, r2 is to the right of r1 and has overlap between y = 124 and 144, or 20.
So the rooms are where I want them. Now let’s create a neighbor and test it:
_:test("Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
_:expect(r1:yOverlap(r2)).is(20)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).isnt(nil)
end)
Inch by inch, step by step … we are not surprised by this message:
17: Neighbor has Door information -- Tests:194: attempt to call a nil value (method 'fromRooms')
Now in starting to write fromRooms, I found this:
function Neighbor:primary(room1, room2)
return Neighbor(room1,room2,true)
end
function Neighbor:secondary(room1, room2)
return Neighbor(room1,room2,false)
end
function Neighbor:fromRooms(room1, room2)
end
We already have two constructors, primary and secondary. At the time we use our new constructor, will we know whether the linkage is primary or secondary? I don’t think so. Something here probably will need to change. For now, I’m going to just call the base constructor and extend the initialization:
function Neighbor:fromRooms(room1, room2)
return Neighbor(room1,room2, nil)
end
I’m sure I’m going to get in trouble here. I’m setting my mind into “spike mode”, and as soon as this test runs, I’m going to commit, to make a save point.
And it runs now. Commit: first Neighbor door test.
Now we’re sort of on a path where the Neighbor decides whether the two rooms it has been handed are properly adjacent, and if so it fills in its blanks and returns an instance, otherwise returns nil. (We may find that we don’t want to do that. We’ll see. In general we don’t want to return nils and so it’s likely. But for now, let’s do the extra stuff in fromCorners before we return our new neighbor:
You’d think I could just call the existing adjacency stuff from here:
function Neighbor:fromRooms(room1, room2)
local n = Neighbor(room1,room2, nil)
if room1:isNeighbor(room2) then
return n
else
return nil
end
end
I want a new test for this, checking for the nil:
_:test("Distant rooms aren't neighbors", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(211,124, 311, 144)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).is(nil)
end)
Both tests run so far. We have a place to stand, but we want a lot more information about the doors than we have at the moment. Let’s think about what we want.
How about a little table, one element for each room, containing the door information (namely the x and y). We could index that table by the room itself, or the room number. Both are unique and long-lasting at the moment. Room is easier:
I can’t help thinking about these four cases we’ve been dealing with, room2 to the north or south or east or west of room1, making room1 to the south or north or west or east of room2. I’ve got all that broken down into a couple of cases but now I think I want it broken all the way out. I could break apart the nsCloseEnough and ewCloseEnough into four options. That’s closer to what I need, I think.
Now it is tempting to stop and design some clever thing that will package all this up into one nifty manipulation of vectors or something. I might be smart enough to do that. I might also be smart enough to almost do it. Let’s just write it out longhand, and then see how we can make the longhand code better. So first, I’ll break up those ns and ew methods:
I’ll keep the combined ones to keep the tests running, but call the new ones. And I’m going to try to improve the names:
function Room:ewCloseEnough(aRoom)
return self:closeEnoughOnEastTo(aRoom) or self:closeEnoughOnWestTo(aRoom)
end
function Room:closeEnoughOnEastTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (hisW>=myE and hisW-myE<ce) or (myW>=hisE and myW-hisE<ce)
end
function Room:closeEnoughOnWestTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (hisW>=myE and hisW-myE<ce) or (myW>=hisE and myW-hisE<ce)
end
Here, I’ve just copied the entire method down and called it twice. Everything should work, and it does. Now to customize each of the two new methods to its single direction:
function Room:closeEnoughOnEastTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (hisW>=myE and hisW-myE<ce)
end
function Room:closeEnoughOnWestTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (myW>=hisE and myW-hisE<ce)
end
There I just removed the half of each return statement that refers to the other direction. (I continue to find this very hard to think about, but working by rote has its merits. Shall I do north south now as well? I think yes. I kind of know how to do it, so I’ll do it before I forget.
function Room:nsCloseEnough(aRoom)
return self:closeEnoughOnNorthTo(aRoom) or self:closeEnoughOnSouthTo(aRoom)
end
function Room:closeEnoughOnNorthTo(aRoom)
local ce = Room:closeEnough()
local hisN = aRoom:north()
local hisS = aRoom:south()
local myN = self:north()
local myS = self:south()
return (myS>=hisN and myS-hisN<ce)
end
function Room:closeEnoughOnSouthTo(aRoom)
local ce = Room:closeEnough()
local hisN = aRoom:north()
local hisS = aRoom:south()
local myN = self:north()
local myS = self:south()
return (hisS>=myN and hisS-myN<ce)
end
Everything still looks good. Tests are green and paths look good on the map.
Commit: break out room:closeEnoughOnWestTo(aRoom) etc.
Now I can begin to use those in my Neighbor creation to decide whether we’re adjacent, and where to put the door if we are.
This is taking longer than I’d like. It’s starting to make me tense but the tests are still good. Let’s extend our test a bit and see what we can do:
_:test("Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
_:expect(r1:yOverlap(r2)).is(20)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).isnt(nil)
local r1x,r1y = n:getDoor(r1)
_:expect(r1x).is(200)
end)
Hm. We could do this dynamically, if we were sure the rooms are close enough together. Right now we are sure, because we checked isNeighbor as we created the Neighbor and returned nil otherwise. That might make this easier. Let’s see. We can do just our current case, which is that r1 is close enough on the west to r2. Unless I’m upside down and backward, which I often am. So … I came up with this:
getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
return aRoom:west()
else
return nil
end
end
function Neighbor:otherRoom(aRoom)
if self.room1 == aRoom then
return self.room2
else
return self.room1
end
end
We need both rooms, the one we’re being asked about, and the other one. So I wrote otherRoom. I expect this code to work. It doesn’t, saying
17: Neighbor has Door information -- Actual: nil, Expected: 200
So, what happened? Is r1 not close enough on the west to r2? Best check that first:
_:test("Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
_:expect(r1:yOverlap(r2)).is(20)
_:expect(r1:closeEnoughOnWestTo(r2), "not west?").is(true)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).isnt(nil)
local r1x,r1y = n:getDoor(r1)
_:expect(r1x).is(200)
end)
I added the not west test, and it fails. Have I reversed east and west? It could happen.
I’ll try East to see. When I do, the test passes. Dammit. If I weren’t so darn self-confident, I’d think I suck at left and right and east and west. Let’s see that code:
function Room:closeEnoughOnWestTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (myW>=hisE and myW-hisE<ce)
end
Sure enough. If I’m west of you, My east is < his west. Right? So these two are backward and should be this way … I can’t see why this ever worked. There must not be enough tests around this stuff. Anyway …
function Room:closeEnoughOnWestTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (hisW>=myE and hisW-myE<ce)
end
function Room:closeEnoughOnEastTo(aRoom)
local ce = Room:closeEnough()
local hisE = aRoom:east()
local hisW = aRoom:west()
local myE = self:east()
local myW = self:west()
return (myW>=hisE and myW-hisE<ce)
end
That will make this test pass. Better double check north and south.
function Room:closeEnoughOnNorthTo(aRoom)
local ce = Room:closeEnough()
local hisN = aRoom:north()
local hisS = aRoom:south()
local myN = self:north()
local myS = self:south()
return (myS>=hisN and myS-hisN<ce)
end
function Room:closeEnoughOnSouthTo(aRoom)
local ce = Room:closeEnough()
local hisN = aRoom:north()
local hisS = aRoom:south()
local myN = self:north()
local myS = self:south()
return (hisS>=myN and hisS-myN<ce)
end
OK, I think that’s actually right. My general flaw is getting east and west screwed up, not north and south. Although once a long time ago, driving home to Omaha from Alamogordo, New Mexico, at night, I drove south for a long time before I realized I had taken a wrong term. We didn’t have GPS in those days.
Anyway I think this is right now, and I think I’d better do a lot more tests. The tests think I’m already in trouble:
17: Neighbor has Door information -- Actual: 100.0, Expected: 200
What does my code say now?
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
return aRoom:west()
else
return nil
end
end
No, darn it. If I’m on his west the door is on my east wall. This is massively difficult for me to keep all in my head. This makes me think that the model isn’t as clear as it could be. Arguably it’s just that my head isn’t as clear as it could be, but when that’s the case, it still means the model should be better. Anyway:
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
return aRoom:east()
else
return nil
end
end
Now our actual plan was to return both x and y here, so I’d better return a y. I guess it’s time to make the test require it.
...
local r1x,r1y = n:getDoor(r1)
_:expect(r1x).is(200)
_:expect(r1y).is(134)
I reckon that half-way between 124 and 144 should turn out to be 134 in our coordinates. I’m doubting myself but not quite that much yet.
So now that our code knows we are properly adjacent, it can ask for the overlapping coordinates:
We have this function, which isn’t quite what we want:
function Room:yOverlap(aRoom)
xx,sLo,xx,sHi = unpack(self:corners())
xx,rLo,xx,rHi = unpack(aRoom:corners())
return max(min(sHi,rHi)-max(sLo,rLo),0)
end
We actually want the low and high values.
I’m not loving this unpack-corners stuff, but it’s not far off from what we need. The function above reminds us that perhaps we shouldn’t assume a non-empty overlap at this point, although right now it’s true by our construction.
I think in the spirit of expressing intention, I’ll ask for what I want, the center of the overlap along y.
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
local y = aRoom:yOverlapCenter(aRoom)
return x,y
else
return nil
end
end
We’re left with the need for that new method:
function Room:yOverlapCenter(aRoom)
xx,sLo,xx,sHi = unpack(self:corners())
xx,rLo,xx,rHi = unpack(aRoom:corners())
local lowY = max(sLo,rLo)
local highY = min(sHi,rHi)
return (lowY + highY)//2
end
Well, that fails my test:
17: Neighbor has Door information -- Actual: 150.0, Expected: 134
Ah, I confused myself with that aRoom/otherRoom bit.
function Room:yOverlapCenter(aRoom)
xx,sLo,xx,sHi = unpack(self:corners())
xx,rLo,xx,rHi = unpack(aRoom:corners())
local lowY = max(sLo,rLo)
local highY = min(sHi,rHi)
return (lowY + highY)//2
end
My test runs. I can compute the door location for a room to the west of an adjacent room.
And it’s nearly lunch time. Commit: west adjacent door calculation works in Neighbor.
Now a quick reflection, and I’m outa here.
Reflection
We’ve made only a small mess so far. The breakout of the NSEW neighbor stuff has resulted in methods with better names, although still not good enough for me to get them right every time. I must be geographically dyslexic or something.
The door calculation works in one direction only, but the other directions should fall right out, subject only to the aforementioned geographic confusion, which will sort out if I’m careful to write a few more tests.
We will then probably xwind up with some duplication, involving some double-checking on adjacency, and the four branches of if that I’m about to right will look very similar as well. Whether they’ll be similar enough that I can factor them down to something smaller remains to be seen. They might be better off standing alone. We’ll decide that when we see the code.
In my earlier days, I would certainly have worked hard to build a general-purpose handler for neighbors that dealt in some clever geometrical way with all the options, all the sides by side and over-unders. That is no longer the way, at least not for me. I am much more inclined to let the appearance of duplication trigger a refactoring that removes it, and to do that refactoring by rote insofar as possible. I try never to reach for generality that isn’t right there in front of me.
You, of course, can do as you see fit. What’s offered here is a view of what might happen when you do as I do. It might not happen: you might be smarter than I am, and make fewer mistakes. I hope so.
See you next time!
-
He says”I when he’s describing probable mistakes, and “we” when things are going well. It doesn’t seem fair to him to blame you for stuff. You’re not even here. ↩