I hesitate to say that we’ll work on poison and antidotes, the way things have gone, but one of these days it could happen. Is today that day? Click to find out.

I reckon that if I don’t go looking for trouble, I can probably focus on the poison/antidote issue. I think it “should” be simple:

  • Some status somewhere in the health update that subtracts health if the player is poisoned.
  • An inventory item which, when touched, cures poisoning and, I imagine, removes itself.

We should start with the antidote. There’s more code there. We have one item to borrow ideas from, the Pathfinder lamp. That’s part of the Loot objects:

local LootIcons = {Strength="blue_pack", Health="red_vial", Speed="green_flask",
Pathfinder="blue_jar"}

function Loot:actionWith(aPlayer)
    self.tile:removeContents(self)
    if self.kind == "Pathfinder" then
        self:addPathfinderToInventory(aPlayer)
    else
        aPlayer:addPoints(self.kind, math.random(self.min, self.max))
    end
end

function Loot:addPathfinderToInventory(aPlayer)
    local item = InventoryItem("blue_jar", "Magic Jar", aPlayer, "spawnPathfinder")
    Inventory:add(item)
end

The code above is rather a hack, but that’s because the inventory-style Loots haven’t told us yet what they want to look like. For now, we can add one following that style. After checking the names of my handy container sprites:

local LootIcons = {Strength="blue_pack", Health="red_vial", Speed="green_flask",
Pathfinder="blue_jar", Antidote="red_vase"}

function Loot:actionWith(aPlayer)
    self.tile:removeContents(self)
    if self.kind == "Pathfinder" then
        self:addPathfinderToInventory(aPlayer)
    elseif self.kind == "Antidote" then
        self:addAntidoteToInventory(aPlayer)
    else
        aPlayer:addPoints(self.kind, math.random(self.min, self.max))
    end
end

Then this:

function Loot:addAntidoteToInventory(aPlayer)
    local item = InventoryItem("red_vase", "Poison Antodote", aPlayer, "curePoison")
    Inventory:add(item)
end

I think that will cause an antidote Loot to be created and put into inventory. I want to explore and see whether I can find one.

After more time exploring and not finding an antidote, I review creation and find this code:

local RandomLootInfo = {
{"Strength", 4,9},
{"Health", 4,10},
{"Speed", 2,5 },
{"Pathfinder", 0,0}
}

function GameRunner:createLoots(n)
    for i =  1, n or 1 do
        local tab = RandomLootInfo[math.random(1,#RandomLootInfo)]
        local tile = self:randomRoomTile(self.playerRoom)
        Loot(tile, tab[1], tab[2], tab[3])
    end
end

With no entry there for Antidote, we’re not going to get one. I add it. Now I readily find a couple of them:

antidotes found

We get the expected error when I touch one of them. We have no cure method in player, of course.

Now we have to decide how this will work. The shell, of course, is this:

function Player:curePoison()
    
end

But what will it do? We have a timer that updates the player’s health. It’s part of characterSheet:

function CharacterSheet:startAllTimers()
    self._delay = tween.delay(5, self.adjustValues, self)
end

function CharacterSheet:adjustValues()
    self._health:adjust()
    self._speed:adjust()
    self._strength:adjust()
    self:startAllTimers()
end

Those _ things are instances of CharacterAttribute. They adjust like this:

function CharacterAttribute:adjust()
    local diff = self._nominal - self._value
    self._value = self._value + sign(diff)*self._adjustment
end

The first idea I have is to somehow inform the _health attribute that there is poison, and have it come up with a different adjustment if there is.

I’m not comfortable with pushing the poison notion that far down. Another possibility is for the CharacterSheet to be aware of poison and send in a poison adjustment value to the health, or to all the objects.

Let’s try that. But let’s commit the Antidote code first. Commit: initial Antidote Loot created.

Now then. I think I’ll do something naive for now.

function CharacterSheet:init(attrs)
    self:initAttributes(attrs or self:defaultAttributes())
    self.poison = 0
end

function CharacterSheet:startPoison()
    self.poison = -2
end

function CharacterSheet:stopPoison()
    self.poison = 0
end

And in adjust:

function CharacterSheet:adjustValues()
    self._health:adjust(self.poison)
    self._speed:adjust(self.poison)
    self._strength:adjust(self.poison)
    self:startAllTimers()
end

I’ve decided poison really messes you up.

And in CharacterAttribute:

function CharacterAttribute:adjust(poison)
    local diff = self._nominal - self._value
    self._value = self._value + sign(diff)*self._adjustment - poison
end

I think this will cause the attribute to drop by 3 when it’s above normal, and by 1 when it’s below. Of course we have no one setting poison … yet.

The real plan is that certain monsters can poison the player. The Murder Hornets and Death Flies, for example, and probably those Yellow Widow spiders. Right now, I’d like a quick test. First we need a method to call. Let’s try this:

function Player:curePoison()
    self.characterSheet:stopPoison()
end

function Player:poison()
    self.characterSheet:startPoison()
end

Let’s put that on the Flee button.

function Player:flee()
    self:poison()
end

A test fails. They’re not passing in poison, so:

function CharacterAttribute:adjust(poison)
    local diff = self._nominal - self._value
    self._value = self._value + sign(diff)*self._adjustment - poison or 0
end

Hm still fails. Maybe I should pay attention:

6: CharacterAttribute adjust toward nominal -- CharacterAttribute:104: attempt to perform arithmetic on a nil value (local 'poison')

I guess it wants parens:

function CharacterAttribute:adjust(poison)
    local diff = self._nominal - self._value
    self._value = self._value + sign(diff)*self._adjustment - (poison or 0)
end

Right. Testing I quickly realize that I am not very bright. Since poison is defined as -2, subtracting it isn’t as clever as it might be.

function CharacterAttribute:adjust(poison)
    local diff = self._nominal - self._value
    self._value = self._value + sign(diff)*self._adjustment + (poison or 0)
end

Now we can see that it works. In the following movie, at first you’ll see the attributes going down. After a while, I touch the antidote, and they start going back up, the normal healing cycle.

So now we have poison basically working, although no monsters are venomous. Let’s commit: poison applied via Flee works, as does Antidote.

What remains to be done?

  • A message should come out when the Antidote is applied.
  • The antidote should be removed from inventory when applied.
  • Possibly we should ensure that you have at least one Antidote by putting some useful items in Room 1.
  • Some monsters should be venomous, at least sometimes. A message should come out when they poison you.

Let’s see about removing an inventory item when it’s used.

Removing Inventory Items

Inventory items just send a message when touched:

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        self.object[self.message](self.object)
    end
end

We’d like them to be able to remove themselves when touched. We could make that unconditional for now, or we could provide for some that are reusable. I’m calling YAGNI on this, and I’m going to make them remove themselves always.

function InventoryItem:touched(aTouch, pos)
    if aTouch.state == ENDED and manhattan(aTouch.pos,pos) < ItemWidth//2 then
        Inventory:remove(self)
        self.object[self.message](self.object)
    end
end

function Inventory:remove(item)
    local index = 0
    for i,thing in ipairs(inventory) do
        if thing==item then index = i end
    end
    if index > 0 then table.remove(inventory, index) end
end

That should do the job. I don’t usually use sequence / array type tables if I’m going to remove, but let’s see how this goes.

That works, but I don’t like it. I’m using an array-style table here, because I want to know how many items there are in the inventory. We’ll let it go for now, since it works. We should, however, file a message.

The message when we find the item is handled here:

function InventoryItem:informObject()
    local inform = self.object["inform"]
    if inform then inform(self.object, "You have received a "..self.name.."!") end
end

This is called … let’s see … from here:

function Inventory:add(item)
    table.insert(inventory, item)
    item:informObject()
end

If Lua had the right syntax, we could write that informObject message as:

    if self.object:implements("inform") then
        self.object:inform("You have ...
    end

In fact we can make that code a bit more expressive:

function InventoryItem:informObject()
    local implementsInform = self.object["inform"]
    if implementsInform then self.object:inform("You have received a "..self.name.."!") end
end

Now what we really want is an in message and an out message.

function Inventory:add(item)
    table.insert(inventory, item)
    item:informObjectAdded()
end

function Inventory:remove(item)
    item:informObjectRemoved()
    local index = 0
    for i,thing in ipairs(inventory) do
        if thing==item then index = i end
    end
    if index > 0 then table.remove(inventory,index) end
end

Then, naively:

function InventoryItem:informObjectAdded()
    local implementsInform = self.object["inform"]
    if implementsInform then self.object:inform("You have received a "..self.name.."!") end
end

function InventoryItem:informObjectRemoved()
    local implementsInform = self.object["inform"]
    if implementsInform then self.object:inform("You have used a "..self.name.."!") end
end

First, test this.

received and used

That works. Remove duplication:

function InventoryItem:informObjectAdded()
    self:informObject("You have received a "..self.name.."!")
end

function InventoryItem:informObjectRemoved()
    self:informObject("You have used a "..self.name.."!")
end

function InventoryItem:informObject(msg)
    local implementsInform = self.object["inform"]
    if implementsInform then self.object:inform(msg) end
end

Whoops!! In testing the usage messages, which do work, I create a pathfinder monster and follow it and try to go down the WayDown, getting this message:

Player:25: attempt to call a nil value (method 'makeSureOldPlayerIsRemoved')
stack traceback:
	Player:25: in method 'cloneFrom'
	GameRunner:337: in method 'placePlayerInRoom1'
	GameRunner:101: in method 'createLevel'
	GameRunner:368: in method 'playerTurnComplete'
	Player:261: in method 'turnComplete'
	Player:189: in method 'keyPress'
	GameRunner:322: in method 'keyPress'
	Main:38: in function 'keyboard'

This is a major defect that has been released to production!

Very bad. If we had any users, the phones would be ringing off the wall. (Remember when there used to be phones on walls?)

Here’s cloneFrom:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        self:makeSureOldPlayerIsRemoved(tile)
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

And here’s this:

function Player:makeSurePlayerIsRemoved(tile)
    tile:moveObject(oldPlayer)
end

I notice a name discrepancy but also what’s oldPlayer going to be in there?

If this were linked up correctly, it might be like this:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        self:makeSureOldPlayerIsRemoved(oldPlayer,tile)
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

function Player:makeSureOldPlayerIsRemoved(oldPlayer,tile)
    tile:moveObject(oldPlayer)
end

Let’s review moveObject, though.

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

This won’t do. This will set the old player onto the tile in question. We don’t want that.

We want this:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        self:makeSureOldPlayerIsRemoved(oldPlayer)
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

function Player:makeSureOldPlayerIsRemoved(oldPlayer)
    oldPlayer:getTile():removeContents(oldPlayer)
end

Well, I’m pretty sure we want that. Gotta test it, and I’m ticked that we didn’t have a test that found it automatically.

Yes, it works, though I’m not entirely happy about it.

We now have two things to commit, the bug fix and the inventory usage messages. If I were working with a team, and if we did hot fixes, I’d have to commit those separately, maybe even on different branches (yucch!) but not here. We only move forward. Commit: fix WayDown crash, add inventory use message as well as received.

Let’s reflect after all that excitement.

Reflect

I really don’t like it when I release a defect to “production”. I’m not sure what happened there, but the method didn’t even hook up and it had the wrong parameters, so it was a pretty egregious error.

It turns out that I made that mistake, or at least documented it, in D-139, where I had:

Begin quote from 139

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

This works. It’s a bit weird, because we ask why we’re moving this old player even though we don’t need it.

Arguably we should revert this because it’s obscure. But if we do, we leave ourselves open to another instance of leaving the player lying about in a cell to be tripped over.

I think we should leave this but make it more clear what’s going on:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        self:makeSureOldPlayerIsRemoved()
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

function Player:makeSurePlayerIsRemoved()
    Tile:moveEntity(oldPlayer, oldPlayer:getTile(), tile)
end

This pattern is “Explaining Method Name”. We use it when the call we have isn’t clear in its purpose. It’s like a comment, except that it’s executable.

End quote from 139

Yeah, well, Explaining Method Name That Doesn’t Even Work is the full name of that pattern.

Later in that article I found some problem, and wrote this:

Begin second quote from 139

That went fine. I find this problem and fix it:

function Player:cloneFrom(oldPlayer, tile, runner)
    local player = Player(tile,runner)
    if oldPlayer then
        self:makeSureOldPlayerIsRemoved(tile)
        player:cloneCharacterSheet(oldPlayer) 
    end
    return player
end

function Player:makeSurePlayerIsRemoved(tile)
    tile:moveEntity(oldPlayer)
end

I wasn’t passing in the tile. The case never happens anyway, so I am not entirely happy with my explaining method name pattern.

Game is good. Commit: Tile:moveEntity becomes instance method.

End second quote from 139

Man, what an obvious mistake, and entirely untested. And what was that crack about never happens? It certainly does happen.

I’m somewhat irritated by the call directly to removeContents here. It is breaking the rule that everything is done by moveEntity, now named moveObject, which maintains the invariant. Unfortunately, that looks like this:

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

Back when moveObject was a class method, it handled a missing tile in either the from or to position, but when we made it an instance method–a good thing–we didn’t handle this case, and didn’t notice for lack of a test.

Now there is one thing we could do. Let’s think about what we want.

We want to have everyone calling through `moveObject, which enforces the invariant that an object appears only in the contents of the tile upon which it’s called. It does that by removing the object from whatever tile it currently thinks it’s on, and then moving it to the new one and adding to the (deferred) contents of that one.

In the case of the old player, we don’t want to add it to a tile. We’re trying to get rid of it.

I’m thinking of a hack, but rather an elegant one, rather like the pattern called NullObject or MissingObject. Suppose we had a Tile that wasn’t actually in the Dungeon, just hanging about. We could grab that tile and move the old player to it. That would remove her from her existing tile, which is in the dungeon, and put her into the irrelevant tile.

Let’s build an object that will do that. (We actually have one, called FakeTile2, and it’s possible that FakeTile can do it as well.) But let’s do one called RemovalTile.

        _:test("removal tile", function()
            local from = FakeTile2()
            local to = RemovalTile()
            local thing = FakeThing(from)
            to:moveObject(thing)
            _:expect(thing:getTile()).is(to)
            _:expect(from.contents).is(nil)
            _:expect(to.contents).is(thing)
        end)

Now it really does turn out that FakeTile2 can do this but I don’t want a test-side object doing this job.

This test will fail looking for removalTile.

3: removal tile -- TestTileMover:30: attempt to call a nil value (global 'RemovalTile')

I’m going to put this class in the test and then move it to Tile.

RemovalTile = class()

function RemovalTile:addDeferredContents(thing)
    self.contents = thing
end

function RemovalTile:moveObject(thing)
    local method = Tile.moveObject
    method(self, thing)
end

function RemovalTile:removeContents(thing)
    assert(thing==self.contents, "removing wrong thing")
    self.contents = nil
end

The cute bit is that our RemovalTile moveObject method calls the one on Tile, so we don’t have to copy/paste the code and get it wrong someday.

The test runs. Move the class. Use it:

function Player:makeSureOldPlayerIsRemoved(oldPlayer)
    RemovalTile():moveObject(oldPlayer)
end

Test. Darn. I was so happy with that but:

Player:72: attempt to call a nil value (method 'illuminate')
stack traceback:
	Player:72: in method 'setTile'
	Tile:352: in local 'method'
	Tile:13: in method 'moveObject'
	Player:40: in method 'makeSureOldPlayerIsRemoved'
	Player:25: in method 'cloneFrom'
	GameRunner:337: in method 'placePlayerInRoom1'
	GameRunner:101: in method 'createLevel'
	GameRunner:368: in method 'playerTurnComplete'
	Player:261: in method 'turnComplete'
	Player:189: in method 'keyPress'
	GameRunner:322: in method 'keyPress'
	Main:38: in function 'keyboard'

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

Ah, we’re doing this:

function Player:setTile(tile)
    self.tile = tile
    self.tile:illuminate()
end

We just need to put illuminate on our RemovalTile:

function RemovalTile:illuminate()
end

This commonly happens when we build a fake, missing, or null object: we have to implement enough protocol to satisfy all the users. This should now work. And it does.

Commit: clone is using RemovalObject to ensure everyone uses invariant-preserving moveObject.

OK. It’s 1145. I’ve been at this since around 0840 or so, so it’s time to wrap up. Let’s sum up.

Summary

We had an unfortunate defect released to production. We have fixed it but not really prevented it. The defect had two parts.

First, the system called a method makeSureOldPlayerIsRemoved, and there wasn’t a method of that name, but there was one of a name similar enough that I didn’t notice.

Second, that method, with the wrong name, didn’t work anyway. It’s only called when the system creates a new dungeon level greater than one, and we have no tests for that. It just mostly works, which is just fine.

fine

This happens. We have a very large-scale feature that is really quite simply implemented. We “just” create a new player and clone the old one into her. This avoids dealing with all the details of the old player’s contents, and directly preserves what we care about. Then we move the old player out, now with our new RemovalTile. Arguably elegant.

But totally untested. And totally not working since d-139.

I’ll think about how to test it. I’m tired now, and definitely don’t know how. But we’ve seen the impact of such not having tests: stuff breaks, as it inevitably will, and we don’t realize it. We can realize it, if our tests are robust enough.

So this is a sign to robustify the tests, if we can think of a way. Robusterate? Robustomize?

Beyond that, we rather quickly and almost nicely implemented poison / venom. Right now, you can only be poisoned if you touch the Flee button, but the capability is in and pretty solid. And the antidote will fix you right up, no problem.

I think we could argue that the implementation is a bit atomic. We set a poison value into the CharacterAttributes, and pass it down to the attributes during adjustment. It’s set to a magic number, -2, which is intentionally chosen to be one greater in absolute value than the health addend, which is one. That means that health always declines if you’re poisoned (and it declines very rapidly down to your normal values).

It’s not very “object oriented”, but its in about the right place, and as we learn more about such things, if there are any more such things, we’ll begin to see the shape of what we need.

We’ve made a new “rule”, which is that when you use an inventory item, it’s removed from inventory. That makes sense, I think, and if we need any inventory items that work differently, we’ll deal with that when it happens.

Remaining to do? We need to make some monsters that can poison the player, and we should probably ensure that the player can be sure to have at least one antidote before meeting anything venomous.

I’m also noticing that it’s probably past time to arrange Loot more reasonably, and I’d like to have Decor items interact a bit as well. Maybe when you walk over an empty pot you have a pot to um keep things in, or if it’s full you have a nice drink. Maybe a skeleton or bones can do you harm, but can also contain a valuable loot of some kind.

And … probably … When we have more than one of a given kind of Loot, we should show the count rather than duplicates. We’ll see how that goes.

Maybe there needs to be a better display of inventory, showing what it is as well as what it looks like?

Lots to do. Amazing how much there is in a tiny game like this.

At least it works again. That’s nice.

See you next time!

D2.zip