P-243 - What We See
Python Asteroids+Invaders on GitHub
I didn’t see it on my drawing, I didn’t see it in my code, but then I saw it in the code in my mind. The lessons today, oddly, are about dozing, and estimation.
Late yesterday, I had an insight. In the test where I was working out the offset for the explosion, there is this code:
topleft_dist_target_to_missile =
Vector2(missile.rect.topleft) - Vector2(target.rect.topleft)
assert topleft_dist_target_to_missile == (6, -1)
topleft_dist_missile_to_explosion =
Vector2(explosion.rect.topleft) - Vector2(missile.rect.topleft)
assert topleft_dist_missile_to_explosion == (-1, -1)
offset = topleft_dist_missile_to_explosion + topleft_dist_target_to_missile
Let me make it a bit more obvious by removing those asserts:
topleft_dist_target_to_missile =
Vector2(missile.rect.topleft) - Vector2(target.rect.topleft)
topleft_dist_missile_to_explosion =
Vector2(explosion.rect.topleft) - Vector2(missile.rect.topleft)
offset = topleft_dist_missile_to_explosion + topleft_dist_target_to_missile
That’s right: missile.rect.topleft cancels out in those two assignments. Let’s use PyCharm’s Inline refactoring:
offset =
Vector2(explosion.rect.topleft)
- Vector2(missile.rect.topleft)
+ Vector2(missile.rect.topleft)
- Vector2(target.rect.topleft)
And, now we can definitely see that minus plus thing. Remove them both.
offset =
Vector2(explosion.rect.topleft)
- Vector2(target.rect.topleft)
Let me remind us of the card, from yesterday. Notice the one marked “Right”:
Looking at the card, it is “easy to see” that the offset of the big blue square, relative to the big red square, is its top left minus the top left of the big red square. Perhaps if I had labeled the picture better I’d have seen it.
We see what we see when we see it. I can think of several ways to explain (cough excuse) not seeing the simple answer sooner, but I don’t think doing so is going to make me any less likely to be slow to see something at some future time. However, there may be a small lesson hiding in how I finally happened to see it:
Where did this insight come from?
I was kind of dozing, thinking about the code, trying to envision the way the two step calculation worked, picturing vectors in my head. I “saw” a line from big red topleft to small blue topleft, and another from there to big blue topleft. Being a vector expert of some renown, I realized that the sum of those two vectors is the vector from big red topleft to big blue topleft.
And I vaguely remembered that the code referred to missile.rect.topleft twice. If the signs were opposite so that it would cancel out, then I understood my mental diagram. If they were both positive, I couldn’t imagine how it could make sense. So I pulled up yesterday’s article on my iPad and looked at the code and sure enough they cancelled out. And now we are quite near to having a very nice solution to our real problem, cleaning up the collision erasure code.
- The lesson?
- Think of the thing in different ways, code, drawing, possibly interpretive dance? Never stop thinking? Look for similarities and duplication in the code? Give things meaningful names? Take more naps? You decide.
Let’s get back to work.
Yesterday
Yesterday I wrote that check_bits helper function that checked a mask, given a list of cells that should be True, and that prints a picture of the mask and if it doesn’t meet the criterion, throws an error. It looks like this when it fails:
11111000
11111000
11111000
11111111
11111111
11111111
11111111
11111111
Bill Wake noticed that and offered this comment:
@RonJeffries I hate the overhead it adds to the first test, but I wonder if a custom input format, perhaps array of strings, would make the tests more clear.
I definitely was relieved to see the table after “The result for the final check is this:” appeared:)
So yesterday, while not writing Yet Another Article, I ginned up a test to create mask from a string, and while it isn’t as lovely as it might be, it works:
def test_thing_from_bits(self):
data = """
10101010
01010101
10101010
01010101
10101010
01010101
10101010
01010101
"""
# This makes a mask
data = data.strip()
lines = data.split("\n")
assert len(lines) == 8
rows = len(lines)
cols = len(lines[0])
mask = pygame.Mask((cols, rows))
for y in range(rows):
line = lines[y].strip()
for x in range(cols):
char = line[x]
mask.set_at((x, y), char == "1")
# This just makes the list for the check
hits = []
for y in range(8):
for x in range(8):
even = y%2
if x%2 != even:
hits.append((x,y))
# And it works
self.check_bits(mask, hits)
So that will let me more readily write some tests of the code for whatever new object we might build. Remember, we’re on this quest because the code that adjusts the mask and image of the Shield is complicated and probably would benefit from a little object to help out.
So let’s extract a function here in the tests, to prepare for some actual work.
PyCharm Extract Method does the job:
def test_thing_from_bits(self):
data = """
10101010
01010101
10101010
01010101
10101010
01010101
10101010
01010101
"""
mask = self.mask_from_string(data)
hits = []
for y in range(8):
for x in range(8):
even = y%2
if x%2 != even:
hits.append((x,y))
self.check_bits(mask, hits)
def mask_from_string(self, data):
data = data.strip()
lines = data.split("\n")
rows = len(lines)
cols = len(lines[0])
mask = pygame.Mask((cols, rows))
for y in range(rows):
line = lines[y].strip()
for x in range(cols):
char = line[x]
mask.set_at((x, y), char == "1")
return mask
I cleverly removed the assert from the function, so now t “should” work for any number of rows and columns, though I freely grant that I have not tested that.
I suspect that we’ll want to refactor the check_bits function at some point. It has checking and printing built in, and we might want just some printing. We’ll see.
Let’s commit: New tests, test_masking.py
Review the Concern
The concern we’re working on is the bit-mask and image code in Shield. We’re using our Tasks reminder code to defer the operation, and the operation itself is a bit complex. We’re considering creating a helper object to deal with the issue. So let’s have a look at what’s there now.
class Shield(InvadersFlyer):
def update_mask_and_visible_pixels(self, collider, explosion_mask, overlap_mask, shot):
self._tasks.remind_me(lambda: self.erase_shot_and_explosion_from_mask(shot, collider.offset(), overlap_mask, explosion_mask))
self._tasks.remind_me(lambda: self.erase_visible_pixels(overlap_mask, self._mask))
def erase_shot_and_explosion_from_mask(self, shot, collider_offset, shot_overlap_mask, explosion_mask):
self._mask.erase(shot_overlap_mask, (0, 0))
self.erase_explosion_from_mask(collider_offset, explosion_mask, shot)
def erase_explosion_from_mask(self, collider_offset, explosion_mask, shot):
expl_rect = explosion_mask.get_rect()
offset_x = (shot.rect.w - expl_rect.w) // 2
adjust_image_to_center = collider_offset + Vector2(offset_x, 0)
self._mask.erase(explosion_mask, adjust_image_to_center)
def erase_visible_pixels(self, shot_mask, shield_mask):
rect = shot_mask.get_rect()
surf = shield_mask.to_surface()
self._map.blit(surf, rect)
I need to walk through this to recreate my understanding of it. The update_ method just triggers the other methods later. It’s the others that do the work.
erase_shot_and_explosion_from_mask does just that. It first just erases the shot_overlap_mask from the Shield’s mask. We recall (probably) that an overlap mask has the same rectangle as the original receiver, so the erase automatically works with no special alignment. Then the erase_ method erases the explosion.
erase_explosion_from_mask does some weird calculation to get the offset, including halving the difference of the widths of the shot and explosion. It’s trying to get the new top left. We know a better way now, but it requires information that we do not have, the center of the shot that hit us.
We’re green. Let me try something. I’ll write the code I wish I could write (Programming by Intention) and then arrange to get the information I need. This may take a try or two.
def erase_explosion_from_mask(self, collider_offset, explosion_mask, shot):
explosion_rect.center = shot.rect.center
adjust_image_to_center = Vector2(explosion.rect.topleft) - Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
If we just had the explosion, we could do that, and I think it would work. Let’s pass it on down and see what we can do.
This will be a more extensive change than I really like, so I’ll work up in a sort of backward fashion. The thing is this: The Shield currently only has the masks, not the actual explosions:
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
explo = BitmapMaker.instance().invader_shot_explosion
self._explosion_mask = pygame.mask.from_surface(explo)
player_explo = BitmapMaker.instance().player_shot_explosion
self._player_explosion_mask = pygame.mask.from_surface(player_explo)
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
OK, as long as we’re here, I’ll save the explosions:
class Shield(InvadersFlyer):
def __init__(self, position):
map = BitmapMaker.instance().shield
self._invader_shot_explosion = BitmapMaker.instance().invader_shot_explosion
self._invader_explosion_mask = pygame.mask.from_surface(self._invader_shot_explosion)
self._player_shot_explosion = BitmapMaker.instance().player_shot_explosion
self._player_explosion_mask = pygame.mask.from_surface(self._player_shot_explosion)
self._map = map.copy()
self._map.set_colorkey("black")
self._mask = pygame.mask.from_surface(map)
self._rect = self._map.get_rect()
self._rect.center = position
self._tasks = Tasks()
- Added in Reflection
- Note that I have just changed direction. I saw an opportunity to get the info that I need, and fetch it. That sets me on s top-down path, pushing the info further down, instead of coding my intention and working my way back up to get the info. At the time, I didn’t even realize I was changing direction. I saw an opportunity to step in roughly the right direction, and took it. This is the way. Planning is everything, plans are nothing.
But I still have to get the info into the erase code. The issue is that the code handles two different explosions, the player shot one and the invader shot one.
Grr, this is harder than I had hoped.
The action starts in these two interactions:
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(shot, self._invader_explosion_mask)
def interact_with_playershot(self, shot, fleets):
self.process_shot_collision(shot, self._player_explosion_mask)
I think that now we really want the actual explosion. Let’s include it in these calls and push it own down. Here, I’ll make the change by hand:
def interact_with_invadershot(self, shot, fleets):
self.process_shot_collision(
shot,
self._invader_shot_explosion,
self._invader_explosion_mask)
def interact_with_playerexplosion(self, explosion, fleets):
pass
def interact_with_playershot(self, shot, fleets):
self.process_shot_collision(
shot,
self._player_shot_explosion,
self._player_explosion_mask)
def process_shot_collision(self, shot, explosion, explosion_mask):
collider = Collider(self, shot)
if collider.colliding():
overlap_mask: Mask = collider.overlap_mask()
self.update_mask_and_visible_pixels(collider, explosion_mask, overlap_mask, shot)
Now pass it down, I think just ahead of the explosion mask makes the most sense:
def process_shot_collision(self, shot, explosion, explosion_mask):
collider = Collider(self, shot)
if collider.colliding():
overlap_mask: Mask = collider.overlap_mask()
self.update_mask_and_visible_pixels(
collider,
explosion,
explosion_mask,
overlap_mask,
shot)
def update_mask_and_visible_pixels(self, collider, explosion, explosion_mask, overlap_mask, shot):
self._tasks.remind_me(lambda: self.erase_shot_and_explosion_from_mask(shot, collider.offset(), overlap_mask, explosion_mask))
self._tasks.remind_me(lambda: self.erase_visible_pixels(overlap_mask, self._mask))
And only the first method needs it:
def update_mask_and_visible_pixels(self, collider, explosion, explosion_mask, overlap_mask, shot):
self._tasks.remind_me(lambda: self.erase_shot_and_explosion_from_mask(
shot,
collider.offset(),
overlap_mask,
explosion,
explosion_mask))
self._tasks.remind_me(lambda: self.erase_visible_pixels(overlap_mask, self._mask))
def erase_shot_and_explosion_from_mask(
self,
shot,
collider_offset,
shot_overlap_mask,
explosion,
explosion_mask):
self._mask.erase(shot_overlap_mask, (0, 0))
self.erase_explosion_from_mask(collider_offset, explosion_mask, shot)
And one more push and we have it:
def erase_shot_and_explosion_from_mask(
self,
shot,
collider_offset,
shot_overlap_mask,
explosion,
explosion_mask):
self._mask.erase(shot_overlap_mask, (0, 0))
self.erase_explosion_from_mask(
collider_offset,
explosion,
explosion_mask,
shot)
def erase_explosion_from_mask(
self,
collider_offset,
explosion,
explosion_mask,
shot):
explosion_rect.center = shot.rect.center
adjust_image_to_center = Vector2(explosion.rect.topleft) - Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
And now I can code what I wanted to code:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center = Vector2(explosion.rect.topleft) - Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
And now I can find out if this spike works. Unfortunately, I have no test for this. So I have to run the game. I am hopeful but not totally confident. Good thing, too, because the pasted last bit is wrong.
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center = Vector2(explosion_rect.topleft) - Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
Even so, it doesn’t work. The shot explosions are not in the right place. That’s bad news but there is some good news as well. The changes needed to get the information down into the erase were simple and if I replace the code there with the original, the game should work. That will make it possible to write some tests. Yes, that works, so I leave all the passing of the explosion in and change this back:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):)
expl_rect = explosion_mask.get_rect()
offset_x = (shot.rect.w - expl_rect.w) // 2
adjust_image_to_center = collider_offset + Vector2(offset_x, 0)
self._mask.erase(explosion_mask, adjust_image_to_center)
Commit: Pass explosions down through erasure, in prep for improved approach.
Now we are in a position to do some testing. And clearly enough, we need it.
- Added in Reflection
- Arguably I start doing the wrong thing here. Arguably I should have turned to testing. But I wanted to understand what was happening. I thought I understood already what should happen. It turns out, as you’ll see below, the problem is a very simple one.
But I want more info. Let’s see what the difference is between the two answers. It might be informative.
print(adjust_image_to_center, new_adjust_image_to_center)
[-10, 0] [-6, 0]
[38, -16] [-6, 0]
[22, -16] [-6, 0]
[54, -16] [-6, 0]
[30, -16] [-6, 0]
[-2, -16] [-6, 0]
[58, -16] [-6, 0]
[54, -16] [-6, 0]
[10, -16] [-6, 0]
[18, -16] [-6, 0]
[78, 0] [-6, 0]
Well, one clear thing is that my new scheme is returning a constant. Ah. We want the offset from the Shield topleft to the explosion topleft. My new calculation does not consider the Shield at all.
- Added in Reflection
- I immediately see the bug, that I’m not referring to the Shield. But then I get distracted by the
collider_offset, which is one way to solve the problem, just not the way I’m working on. A mistake, fortunately not too costly.
What is the collider_offset?
Vector2(self.right_rect.topleft) - Vector2(self.left_rect.topleft)
That’s the offset from the Shield to the Hit. Might be what we need.
- Aside
-
I am doing the wrong thing here, by some standards. I am debugging the code. Well, close to it: I’m certainly mucking about in the code trying to figure out why my new code differs from the old code. That does have to be done, but it would probably be better done in a clean testing environment. But that’s not what I’m doing. And I’m not going to stop, not yet.
- Added in Reflection
- At least I am aware of what I’m doing. I consciously choose, rightly or wrongly, to continue with the debug / understand loop rather than turn to tests. If I already had tests, I’d probably have turned to them. Without them, the code in hand seemed more fruitful.
A little more printing tells me that adding in collider_offset is going to work:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center = collider_offset + Vector2(explosion_rect.topleft) - Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
And it does. So, we rationalize to ourselves, “OK, simple mistake, let’s carry on”. Looking at the Vector2 bit, we ask what that is and extract a variable:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
from_shot_to_explosion_topleft = Vector2(explosion_rect.topleft) - Vector2(shot.rect.topleft)
adjust_image_to_center = collider_offset + from_shot_to_explosion_topleft
self._mask.erase(explosion_mask, adjust_image_to_center)
I don’t like that code much. The reason is that we are asking Collider for something and then adjusting it. Either collider should provide what we need or we shouldn’t have to ask it for anything. I guess the collider doesn’t know the explosion but I think we’ll find that part of what is coming out of collider is being cancelled out by what we’re doing here.
- Added in Reflection
- This is right on two counts. First, it’s a smell that we can’t get what we want from collider, and second, it is a correct insight—i.e. clever guess—that we are probably cancelling something out of the collider offset. A bit more searching shows that my guess is right and we’re good.
Let’s do some comparative printing. I am still doing the wrong thing, but in my defense I’m trying to figure out what the current code does to try to improve it. That I’m having this much difficulty tells me that the current code isn’t helping as much as it might.
With some fancy printing I see this:
Collider shot: (402, 736) - Shield: (378, 752) = offset: [24, -16]
adj = explosion (396, 736) - shot (402, 736) = [-6, 0]
The collider offset, which we use, includes the shot position (402, 736) and the Shield position, while we are removing the shot position and adding in the explosion position.
So we should be able to compute the explosion position minus the shield position and have what we need. Which is what we set out to do hundreds of lines ago, but did wrongly.
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center =
Vector2(explosion_rect.topleft)
- Vector2(self._rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
And this works. Permit me to commit, since I’ve arguably already sinned. Commit: modify, perhaps improve, mask erasure.
Let us reflect.
Reflection
The bug was that I differenced the explosion topleft and the shot topleft and I should have differenced the explosion topleft and the shield (self) topleft. That single mistake made all the difference:
Incorrect:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center =
Vector2(explosion.rect.topleft)
- Vector2(shot.rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
Correct:
def erase_explosion_from_mask(self, collider_offset, explosion, explosion_mask, shot):
explosion_rect = explosion.get_rect()
explosion_rect.center = shot.rect.center
adjust_image_to_center =
Vector2(explosion_rect.topleft)
- Vector2(self._rect.topleft)
self._mask.erase(explosion_mask, adjust_image_to_center)
If I had not made that mistake, I’d have committed that code and, if I were a good person, I would have used my new tool to write a few tests to demonstrate how that logic works and probably would have test-driven a new image masher object to do the job more clearly.
What caused the mistake? Possibly poor naming in the tests where I discovered the solution of differencing the toplefts. Possibly the things that were accessible in the erase_ method threw me off. We’ll never know.
Should I have turned immediately to better testing? Well, the received wisdom says yes, and my own experience as abstracted into language says yes. But my experience as abstracted into my actions must have said no, because instead I set out to discover what was going on.
And the truth is, inspecting the code, with the help of a couple of prints, did show me that the erase_ code was cancelling out half of the collider offset and that showed me more clearly that I needed the Shield’s top left, which I had not used before, and that was in fact the simple defect I’d put in at the top.
So I found the problem, it was a simple problem, almost a copy-paste issue, and last night’s dozing insight has been shown to be correct and to make the code a tiny bit more clear.
Would I have found the defect sooner had I stopped and written tests? Possibly, but I think that most of what was done here needed to be done anyway, all the pushing down of the explosion and such. I think it would have taken a few tests to get ready and then what? Possibly two methods in Shield, one getting the offset one way and the other another, to compare them? Isn’t that pretty much what we did anyway, some comparison printing?
I honestly do not know. I want to say that it was a risky decision not to turn directly to testing, because debugging might not have been fruitful and testing almost certainly would have been, but that in this case, the risk paid off.
That’s a little bit like telling someone to hold your beer and then not injuring yourself, and saying that therefore jumping off the roof into a pile of leaves was a perfectly sensible thing to do.
Be that as it may, my insight from last night has proven to improve the erase_ code a little bit, and encourages me to move forward on creating a new helper object.
And that’s where we see an important lesson.
Lesson
Detailed estimation does not work.
I started this morning with existing tests that showed me how to compute the mask offset easily. I expected to type in the code, see that it worked, commit the Shield, and move directly to test-driving an ImageMasher or whatever the new object would be. I estimated the time to get to that test driving as a half hour or less. I expected good progress on ImageMasher within two or three hours of starting this article.
I am three hours in and probably ready to start on ImageMasher. A one-morning effort, estimated this morning, with all the information right here, has turned into a two-morning effort, caused by an issue that I did not foresee, that I do not see now how I could have reasonably avoided, and that consumed the entire session.
This, my friends, is why detailed estimation of programming does not work. We cannot foresee the little things that take large amounts of time. This one was a tiny mistake. I “should not” have made it, but I did.
We can project our progress pretty accurately, by tracking actual feature delivery, because in our actuals, the errors will be taken into the average. In my view we cannot estimate stories by looking at them or thinking about them, unless we multiply our estimates by some integer between e and pi, and even then we’ll be wrong.
Fortunately, no one is holding me to my estimates. My job is to do what I do, show my work, explain my thinking as best I can, and leave it to you to do with it what you will. If you learn something useful from me, great. If you just learn “don’t do what Ron did”, that’s great too.
See you next time!