Looking Around
With our new path deviation operation working, it’s time to look at the code and see if it’s asking for anything. We end with a seriously questionable choice.
There will be no April Fool’s jokes here. Sirius XM already pulled on on me, that’s enough.
I think we have plenty of new things that we could invent for our dungeon creation program, and we’ll consider some of those in due time. But the path work we’ve been doing has impacted quite a few classes, and it seems like a good moment to look around and see if the code is crying out for improvement. Or even mumbling: we’ll make small improvements as well as large.
I have one such possibility in mind already. Our new method on Cell is compute_via, which now accepts some parameters asking how many segments the path should be divided into, and how much deviation from the path is desired, and then produces a list of points called “via”, which can be used in the path creation logic.
To me, it does make sense to say we’re building a path “via” these points, but calling them “vias” doesn’t sound good, and I’ve been calling them “waypoints” as I write. It would be nice to settle on one term and if it fits the code, I think I prefer “waypoints”. Here’s some relevant code:
class PathHelper:
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
class Cell:
def find_path_via_waypoints(self, t, rooms, via):
points = [self] + via + [t]
cells = []
for p1, p2 in zip(points, points[1:]):
cells += p1.find_path(p2, rooms)
return list(set(cells))
main.py
...
waypoints = helper.compute_via(5, 10)
path = start.find_path_via_waypoints(target, [s, t], waypoints)
...
class TestPaths:
def test_path_helper(self):
...
via = helper.compute_via()
assert via == [up_expected, down_expected]
...
def test_waypoint_not_available(self):
...
waypoints = helper.compute_via(2, 1)
...
I think the votes are in. We’ll change the method name to compute_waypoints, change the parameter in find_path_via_waypoints to waypoints, and rename that temp in the test.
Type shift+F6 three times, edit the names, and we’re done. Commit.
OK, we’re warmed up. Let’s look a little more deeply, still hanging around in Cell and PathHelper.
Let’s look at all of PathHelper and see what we see.
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)
def compute_waypoints(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
def rotate_offset(self, x_offset, y_offset, theta):
x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
cell = self.source.at_offset(x_rot, y_rot)
return cell
def get_length_and_theta(self) -> tuple[float, float]:
sx, sy = self.source.xy
tx, ty = self.target.xy
dx = tx - sx
dy = ty - sy
length = math.sqrt(dx * dx + dy * dy)
theta = math.atan2(dy, dx)
return length, theta
@staticmethod
def rotate(x, y, angle):
x_rot = x * math.cos(angle) - y * math.sin(angle)
y_rot = x * math.sin(angle) + y * math.cos(angle)
return int(x_rot), int(y_rot)
length and theta are functions only of the source and target cells, which are creation parameters of PathHelper. We could make them member variables, init them at creation, and use the members in compute_waypoints, which would simplify it a bit and make a bit more sense.
We will probably never call compute_waypoints twice on the same helper, but we could. Not a big efficiency issue here but I think it’ll be more clear if we move the calculation up.
class PathHelper:
def __init__(self, source, target):
self.source = source
self.target = target
rooms = [source.room, target.room]
self.length, self.theta = self.get_length_and_theta()
self.map = source.path_map(lambda c: c.is_available or c.room in rooms)
def compute_waypoints(self, segments=3, offset=5):
result = []
for i in range(1, segments):
rotated = self.rotate_offset(i * self.length // segments, offset, self.theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
Green. Commit: tidying.
The target member is only used in get_length_and_theta. Let’s pass source and target as parameters to the get and then we can avoid the target member: Change Signature and use the parms:
@staticmethod
def get_length_and_theta(source, target) -> tuple[float, float]:
sx, sy = source.xy
tx, ty = target.xy
dx = tx - sx
dy = ty - sy
length = math.sqrt(dx * dx + dy * dy)
theta = math.atan2(dy, dx)
return length, theta
Now up in __init__, we can do this:
class PathHelper:
def __init__(self, source, target):
self.source = source
self.length, self.theta = self.get_length_and_theta(source, target)
rooms = [source.room, target.room]
self.map = source.path_map(lambda c: c.is_available or c.room in rooms)
Commit.
Now we look at these two methods:
def compute_waypoints(self, segments=3, offset=5):
result = []
for i in range(1, segments):
rotated = self.rotate_offset(i * self.length // segments, offset, self.theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
def rotate_offset(self, x_offset, y_offset, theta):
x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
cell = self.source.at_offset(x_rot, y_rot)
return cell
I am tempted to try to recast compute_waypoints as a list comprehension, but we’ll hold off on that. A larger flaw is that rotate_offset sounds like an action and what it does is return a cell. (It could also return None but that will work since None won’t be in the map.)
Let’s rename rotate_offset to cell_at_offset and see how that looks:
def compute_waypoints(self, segments=3, offset=5):
result = []
for i in range(1, segments):
rotated = self.cell_at_offset(i * self.length // segments, offset, self.theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
def cell_at_offset(self, x_offset, y_offset, theta):
x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
cell = self.source.at_offset(x_rot, y_rot)
return cell
Yes, I like that better. And yes, I might actually do a rename, look at the result, and then undo it or choose a better name. The objective is code that makes sense when we read it, we might not be sure until we actually read it. Since the change is easy and the undo is easy, we can readily afford to look at the reality rather than imagine it.
Commit. I think we’ll inline the return. I added a type hint as well:
def cell_at_offset(self, x_offset, y_offset, theta) -> Cell:
x_rot, y_rot = self.rotate(x_offset, y_offset, theta)
return self.source.at_offset(x_rot, y_rot)
Commit. (I’m calling all these little changes “tidying”, as is my practice. It is almost unheard of for me to read a commit message. I almost always work forward from where we are, and rarely will just reset back a few levels. No branches.)
I notice this:
@staticmethod
def get_length_and_theta(source, target) -> tuple[float, float]:
sx, sy = source.xy
tx, ty = target.xy
dx = tx - sx
dy = ty - sy
length = math.sqrt(dx * dx + dy * dy)
theta = math.atan2(dy, dx)
return length, theta
@staticmethod
def rotate(x, y, angle):
x_rot = x * math.cos(angle) - y * math.sin(angle)
y_rot = x * math.sin(angle) + y * math.cos(angle)
return int(x_rot), int(y_rot)
The first calls the angle theta and the second calls it angle. If I have a convention at all, it would be to have radians named theta and degrees named angle. Since I have a couple of degrees in math, theta is a meaningful name to me, shouting out “RADIANS”. Rename to theta in the rotate method.
Commit.
Feeling strong here. Let’s go for the comprehension here:
def compute_waypoints(self, segments=3, offset=5):
result = []
for i in range(1, segments):
rotated = self.cell_at_offset(i * self.length // segments, offset, self.theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
Ah. Can’t do it, because of the toggling of the offset. I say “can’t”. We could change cell_at_offset to add or subtract offset depending on whether i was odd or even, or something like that.
We might be able to mush around with passing i into cell_at_offset, etc etc.
I wonder if this will pass:
def compute_waypoints(self, segments=3, offset=5):
result = []
step = self.length//segments
for i in range(1, segments):
rotated = self.cell_at_offset(i * step, offset, self.theta)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
It will. Since theta is now a member, we don’t need to pass it:
def compute_waypoints(self, segments=3, offset=5):
result = []
step = self.length//segments
for i in range(1, segments):
rotated = self.cell_at_offset(i * step, offset)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
def cell_at_offset(self, x_offset, y_offset) -> Cell:
x_rot, y_rot = self.rotate(x_offset, y_offset, self.theta)
return self.source.at_offset(x_rot, y_rot)
I think I like where this is going. Commit. Pass i, do the multiply inside:
def compute_waypoints(self, segments=3, offset=5):
result = []
step = self.length//segments
for i in range(1, segments):
rotated = self.cell_at_offset(i, step, offset)
if rotated in self.map:
result.append(rotated)
offset = -offset
return result
def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
x_rot, y_rot = self.rotate(i*x_offset, y_offset, self.theta)
return self.source.at_offset(x_rot, y_rot)
Interesting … commit. Now if there was a function from i to plus or minus 1, starting with + for odd i …
Remove the offset toggle from compute_waypoints and see the tests fail.
def compute_waypoints(self, segments=3, offset=5):
result = []
step = self.length//segments
for i in range(1, segments):
rotated = self.cell_at_offset(i, step, offset)
if rotated in self.map:
result.append(rotated)
return result
def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
up_dn = 1 if i%2 == 1 else -1
x_rot, y_rot = self.rotate(i*x_offset, up_dn*y_offset, self.theta)
return self.source.at_offset(x_rot, y_rot)
Green. Commit. And now:
def compute_waypoints(self, segments=3, offset=5):
step = self.length//segments
return [rotated
for i in range(1, segments)
if (rotated := self.cell_at_offset(i, step, offset)) in self.map]
def cell_at_offset(self, i, x_offset, y_offset) -> Cell:
up_dn = 1 if i%2 == 1 else -1
x_rot, y_rot = self.rotate(i*x_offset, up_dn*y_offset, self.theta)
return self.source.at_offset(x_rot, y_rot)
We’re green. I’m committing: seriously questionable comprehension.
A long comprehension with a walrus operator. Possibly too cool to live. They say “kill your darlings”, but that is not my way. In my defense, first of all, back off, it’s my blog. Second, comprehensions are a key part of Python and we need to be good at using them. Third, if it confuses us later, we’ll fix it, perhaps change it back, perhaps even have a better idea.
In fact, having written that, I already have a better idea. We won’t explore it, but I think we could get down to something like this:
def compute_waypoints(self, segments=3, offset=5):
...
return [cell
for cell in offset_cells(...)
if cell in self.map]
That would apparently loop twice, but do we care? Not if offset_cells was a generator …
We’ll save that exploration for next time. If it turns out nice, today’s move will have been justified. If not, today’s move is still justified because of “first”, “second”, “third” above.
What do you think? Toot me. What will I do next? Stop by and find out.
See you then!