Complete this? Generalize it?
We need the validity check in PathHelper. And I see how to generalize it. Which should be first? Updated: found a test.
To complete our PathHelper and make it useful, we have to ensure that the waypoints are all available in the path map from source to target. Should be easy enough. We’ll want a test for it, but that, too, should be easy enough.
Our current code for making the waypoints includes some magic numbers:
class PathHelper:
def compute_via(self):
length, theta = self.get_length_and_theta()
y_offset = 5
up = self.rotate_offset(length//3, y_offset, theta)
dn = self.rotate_offset(2 * length//3, -y_offset, theta)
return [up, dn]
The particular offset, 5, the fact that we do exactly two waypoints, and the length computations dividing by 3 (and multiplying by 2) are all pretty magical. I realized at some point last night that we could specify two parameters, the number of waypoints needed and the offset desired, and generate the waypoints with a loop, allowing for a wider range of styles. We could even specify two offsets and choose randomly over a range.
No one has asked for this. Of course, the moment that I mention it to myself, I’m going to see the value of the idea and approve it, so there is no real problem there. We are go for adding that capability.
Which of our two features shall we do first, and why? We’re going to do the looping one first, and there is one somewhat good reason beyond just wanting to: there is duplication of code in the compute_via method. Those two calls to rotate_offset are very similar. We wouldn’t always remove two consecutive calls to the same method, but if we think about how we’ll do the validity check, we see that it’ll go something like this:
# untested un-compiled code
class PathHelper:
def compute_via(self):
length, theta = self.get_length_and_theta()
y_offset = 5
result = []
up = self.rotate_offset(length//3, y_offset, theta)
if self.is_valid(up):
result.append(up)
dn = self.rotate_offset(2 * length//3, -y_offset, theta)
if self.is_valid(dn):
result.append(dn)
return result
I just typed that into the article, not into the code, but it seems about right. Now we have enough duplication to make us really want to reduce it. So let’s do the loop part now, if not all the parameters.
Each time through the loop we want to change the sign of the y offset. Each time through the loop, we want to multiply length times a larger integer, 1, 2, etc. And we want to accumulate our results into a list that starts empty. We’ll go in small steps.
def compute_via(self):
length, theta = self.get_length_and_theta()
result = []
y_offset = 5
up = self.rotate_offset(length//3, y_offset, theta)
result.append(up)
dn = self.rotate_offset(2 * length//3, -y_offset, theta)
result.append(dn)
return result
Green. Commit: refactoring toward parameterized via.
Let’s do the loop next:
def compute_via(self):
length, theta = self.get_length_and_theta()
result = []
y_offset = 5
for i in range(1,3):
rotated = self.rotate_offset(i*length//3, y_offset, theta)
result.append(rotated)
y_offset = -y_offset
return result
I expected that to work, but a test has failed. Oh. I said rotate where I should have said rotate_offset. Fixed in the code and above.
Green. Commit. We have a magic number, the 3. Extract it: it’s the number of segments we want in our waypointed path.
def compute_via(self):
length, theta = self.get_length_and_theta()
result = []
segments = 3
y_offset = 5
for i in range(1, segments):
rotated = self.rotate_offset(i * length // segments, y_offset, theta)
result.append(rotated)
y_offset = -y_offset
return result
Green. Commit. Now we can make segments and offset parameters, with those defaults. That includes a rename of y_offset to offset.
def compute_via(self, segments=3, offset=5):
length, theta = self.get_length_and_theta()
result = []
for i in range(1, segments):
rotated = self.rotate_offset(i * length // segments, offset, theta)
result.append(rotated)
offset = -offset
return result
Green. Commit: parameterize compute_via.
Now just for fun, I’m going to draw a very jagged path in main, since we are set up for that.
That’s five segments, offset 10. Very interesting. Brings to mind another way that we might generate a dungeon. Perhaps we could pick a few points, draw very zig-zagged paths between them, and then plant rooms at those points. Seems that it might generate interesting layouts.
Validity
Our via points are valid if they are accessible to source and target. The path map between source and target is exactly the cells that are accessible along the way from source to target. (Although we may actually short-terminate the generation: we’ll have to look into that.)
Let’s get that map when we create the PathHelper.
Oh I almost forgot: we really need a test for this. We need a layout with two points wanting to be connected, with vias, and we need a chosen via cell not to be available.
def test_waypoint_not_available(self):
space = CellSpace(10, 3)
r0 = []
r2 = []
for x in range(10):
r0.append(space.at(x, 0))
r2.append(space.at(x, 2))
room0 = Room(r0, r0[0], 'room0')
room2 = Room(r2, r2[0], 'room2')
source = space.at(0,1)
target = space.at(9,1)
helper = PathHelper(source, target)
waypoints = helper.compute_via(2, 1)
assert waypoints == []
That was a bit fiddly. We have a 3-high space, with a room all the way across at 0 and another all the way at 2. We want to find a path between (0,1) and (9,1), with one waypoint at distance 1. We expect waypoints to be empty. Currently it is not:
[Cell(4, 2)] != []
Now, we’ll get a path map in PathHelper and use it:
class PathHelper:
def __init__(self, source, target):
self.source = source
self.target = target
self.map = source.path_map(lambda c: c.is_available)
def compute_via(self, segments=3, offset=5):
length, theta = self.get_length_and_theta()
result = []
for i in range(1, segments):
rotated = self.rotate_offset(i * length // segments, offset, theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
We create a path map in __init__ and check our proposed waypoint for presence. If it’s there we include it, if not, then not. We are green on the new test and the others. I’ll run main as well. And I’m glad I did: the path goes straight across. Something is not OK here.
And why didn’t my other test fail?
def test_path_helper(self):
space = CellSpace(30, 30)
source = space.at(5, 5)
target = space.at(5, 26)
up_expected = space.at(0, 12)
down_expected = space.at(10, 19)
helper = PathHelper(source, target)
via = helper.compute_via()
assert via == [up_expected, down_expected]
That one must have returned values. What happened in main then?
Ah: We have to allow the cells source and target to be accepted. They are in rooms and therefore not available.
class PathHelper:
def __init__(self, source, target):
self.source = source
self.target = target
rooms = [source.room, target.room]
self.map = source.path_map(lambda c: c.is_available or c.room in rooms)
I think we’re good. We have a test that shows compute_via excluding used points, and accidentally saw it affecting main. We have a test that gets vias correctly. And main also works now that we have fixed PathHelper.
It would be nice to have a test that fails when the PathHelper fix is not in.
I think adding those cells to rooms would have made the latest test fail.
def test_waypoint_not_available(self):
space = CellSpace(10, 3)
r0 = []
r2 = []
for x in range(10):
r0.append(space.at(x, 0))
r2.append(space.at(x, 2))
room0 = Room(r0, r0[0], 'room0')
room2 = Room(r2, r2[0], 'room2')
source = space.at(0,1)
source_room = Room([source], source, 'source_room')
target = space.at(9,1)
target_room = Room([target], target, 'target_room')
helper = PathHelper(source, target)
waypoints = helper.compute_via(2, 1)
assert waypoints == []
I’ll change the PathHelper not to use the rooms: Ah, no, not good enough. That test never tested anything: the vias got rejected because of the bug. We’ll change that test to make the source and target elements of the two rooms r0 and r2.
Hm. After a number of tries, I haven’t been able to create a test that fails when that fix isn’t in compute_via. The reason is that if the fix is not in, the path map will not include the source and target cells, if they are in rooms, and the map will be truncated. So there will be no via points found. So we get the right answer for the wrong reason.
I’m tired. I’ll have to think about this a bit. With the fix in, I’m convinced it’s all working, it just isn’t well enough tested to detect an accidental removal of that or clause in the lambda. I’ll commit: looping compute via validates via cells properly.
Summary
We have done what we set out to do: the via computation is now parameterized as to number of segments to create and the offset to use, and it validates the proposed waypoints to ensure they are able to be reached by the path drawing logic.
However, there is a slight bitter taste in my mouth because we do not have a test that fails if the PathHelper doesn’t allow the two rooms to be included in its valid cells. I’ll have to clear my mind and think about how to check that. I thought I had a test that would catch it, but it didn’t.
Need break. But I’m confident in the code.
The process, I think, was rather nice. We had duplication in the compute_via method. We extracted variables until we had broken out the offset negation and multiplication of length by the soon-to-be loop index, then wrote the loop. Then we moved the extracted variables up to the parameter list, with suitable defaults. So all that went nicely.
And the validity check is good as well, checking the proposed cells for presence in the path map from source to target, ensuring that they aren’t isolated or off in rooms.
Hm, maybe isolation is the approach to take on the test we need. I’ll think about that.
So, oddly enough, this went well … except that it required running main to detect the need for a more complex can_use. That’s not good enough.
We’ll figure out a test and add it. For now, it’s past time to make a chai.
See you next time!
Added Later
I realized that with the check for rooms taken out, the via is correct in my tests, but that the map itself is what is wrong. So I added a check to ensure that the map includes the end points:
def test_waypoint_not_available(self):
space = CellSpace(10, 3)
r0 = []
r2 = []
for x in range(10):
r0.append(space.at(x, 0))
r2.append(space.at(x, 2))
room0 = Room(r0, r0[0], 'room0')
room2 = Room(r2, r2[0], 'room2')
source = space.at(0,1)
source.room = 333
target = space.at(9,1)
target.room = 666
helper = PathHelper(source, target)
waypoints = helper.compute_via(2, 1)
assert source in helper.map # added
assert target in helper.map # added
assert waypoints == []
That fails without the check for rooms and works with it. That satisfies my concern.