Let’s drive the bus a bit further. And, I’ve seen a suggestion for a slightly different angle. We should consider that. Ohhh and a garbage problem!

The Angle

I saw some suggested code, somewhere, for a listener kind of thing, which is of course the subscriber half of our event bus thing.

Our calling sequence for subscribing is this:

    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")

We subscribe passing in self, which is the object that wants to receive the notification, a method, self.addTextToCrawl, and the event name, which is also “addTextToCrawl”. (Arguably it should be a more generic name, like “publishText”, or “notify”. There’s no guarantee that the crawl is the only publisher, or even that there is a crawl at all.)

When the EventBus notifies subscribers, it does this:

function EventBus:publish(event, optionalSender, optionalInfo)
    for subscriber,method in pairs(self:subscriptions(event)) do
        method(subscriber, event, optionalSender, optionalInfo or {})
    end
end

The method (a function) is called, passing the subscriber, event, sender if any, and info if any. I could argue that the info should precede the sender, since info is more likely to be provided than sender. We’d only offer the sender if we are offering to enter into a conversation with the subscribers, which seems unlikely.

The example I read passed a function explicitly, rather like this:

    Bus:subscribe(self, "addTextToCrawl", function(info) self.addItem(CombatRound():displayText(info.text)) end)

OK, one look at that makes me prefer my scheme, since those embedded function definitions tend to be hard to read and we could always do it with our current scheme. If I found myself wanting to include the functions explicitly like that, I might order the parameters to put the function last, which is more readable and more “Lua standard”.

But we’re doing OO, so I think we’ll stick with passing in a method, even if we write a special one.

OK, we considered that. I’m glad we had this little chat. Let’s do some “work”.

Looking for Text Publishers

We’re set up to allow anyone who wants to display text to just publish it, so let’s see who else calls the runner’s addTextToCrawl and convert them.

function Player:curePoison()
    self.runner:addTextToCrawl("You have been cured of poison!")
    self.characterSheet:stopPoison()
end

function Player:poison()
    self.runner:addTextToCrawl("You have been poisoned!")
    self.characterSheet:startPoison()
end

function Player:doCrawl(kind, amount)
    local msg = string.format("+%d "..kind.."!!", amount)
    self.runner:addTextToCrawl(msg)
end

function Player:inform(message)
    self.runner:addTextToCrawl(message)
end

What we have here, friends, is duplication. Let’s first remove it:

function Player:curePoison()
    self:inform("You have been cured of poison!")
    self.characterSheet:stopPoison()
end

function Player:poison()
    self:inform("You have been poisoned!")
    self.characterSheet:startPoison()
end

function Player:doCrawl(kind, amount)
    local msg = string.format("+%d "..kind.."!!", amount)
    self:inform(msg)
end

One more thing, a nicety. I’m going to put those informs last in the method, No deep reason but I know that Lua does tail calls, so this will be a tiny bit faster and less deep in the call stack. Just a nicety, hardly worth doing.

function Player:curePoison()
    self.characterSheet:stopPoison()
    self:inform("You have been cured of poison!")
end

function Player:poison()
    self.characterSheet:startPoison()
    self:inform("You have been poisoned!")
end

Now we’ll fix inform:

function Player:inform(message)
    Bus:publish("addTextToCrawl", self, {text=message})
end

A quick test, and we’re good to go.

cured

And that works. Commit: Player uses EventBus to publish messages.

Are there other callers to runner’s addTextToCrawl? There are not. We can remove that method now, reducing the face of GameRunner a bit.

Commit: remove addTextToCrawl method from GameRunner.

Let’s do more on the floater

The GameRunner also has this method:

function GameRunner:addToCrawl(array)
    self.cofloater:addItems(array)
end

This is sent here:

function GameRunner:initiateCombatBetween(attacker, defender)
    if attacker:willBeDead() or defender:willBeDead() then return end
    local co = CombatRound(attacker,defender)
    self:addToCrawl(co:attack())
end

function Spikes:actionWith(player)
    local co = CombatRound(self,player)
    co:appendText("Spikes ".. self.verbs[self.state]..player:name().."!")
    local damage = math.random(self:damageLo(), self:damageHi())
    co:applyDamage(damage)
    self.tile.runner:addToCrawl(co:getCommandList())
end

function Decor:damage(aPlayer)
    local co = CombatRound(self,aPlayer)
    co:appendText("Ow! Something in there is sharp!")
    local damage = math.random(1,5)
    co:applyDamage(damage, "_speed")
    self.tile.runner:addToCrawl(co:getCommandList())
end

These are … interesting. CombatRound:attack returns the command list:

function CombatRound:attack()
    if self.attacker:willBeAlive() and self.defender:willBeAlive() then
        self:append(self:display("  "))
        local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
        self:append(self:display(msg))
        self:attemptHit()
    end
    return self:getCommandList()
end

The applyDamage function does not return the list:

function CombatRound:applyDamage(damage, kind)
    self.defender:accumulateDamage(damage, kind)
    local op = { op="extern", receiver=self.defender, method="damageFrom", arg1=damage, arg2=kind }
    self:append(op)
    self:append(self:display(self.defender:name().." takes "..damage..self:wordFor(kind).." damage!"))
    if self.defender:willBeDead() then
        local op = OP("extern", self.defender, "youAreDead", self)
        self:append(op)
        local msg = self.defender:name().." is down!"
        self:append(self:display(msg))
        self.defender:playerCallback(self, "playerIsDead")
    end
end

We only have the one call to attack outside of tests. We can leave the attack method returning the list, or we can change the tests to demand it. (Or maybe something more weird.)

If we add a publish to CombatRound, we can cause attack to do it unconditionally, since that’s what happens anyway, and we can call the publish method from the two guys who are calling applyDamage … or provide another method for them to call that does publish. That would leave all the callers uninterested in how the information comes out, just focusing on what they want to happen, i.e. a battle, or damage.

First, we’ll need to expose Floater to a new event. We want to wind up here:

function Floater:addItems(array)
    self.provider:addItems(array)
end

Let’s subscribe:

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
    Bus:subscribe(self,self.addItems, "addItemsToCrawl")
end

Note that we’re just going to drop right into the addItems, which means we want the info packet to be the array of items to be displayed.

Now in CombatRound:

function CombatRound:publish()
    Bus:publish("addItemsToCrawl", self, self.commandList)
end

I suspect we’d do well to nil that variable. I chose to use the variable, by the way, because I foresee removing the getter for the list.

Now as a test, let’s try the Spikes with the new scheme:

function Spikes:actionWith(player)
    local co = CombatRound(self,player)
    co:appendText("Spikes ".. self.verbs[self.state]..player:name().."!")
    local damage = math.random(self:damageLo(), self:damageHi())
    co:applyDamage(damage)
    self.tile.runner:addToCrawl(co:getCommandList())
end

We change that:

function Spikes:actionWith(player)
    local co = CombatRound(self,player)
    co:appendText("Spikes ".. self.verbs[self.state]..player:name().."!")
    local damage = math.random(self:damageLo(), self:damageHi())
    co:applyDamage(damage)
    co:publish()
end

Now spikes should go through the new scheme. This may be a crash, but I am optimistic.

Also we need to think about better testing for this.

Stepping on the spikes gives me this:

Provider:17: did not get a table in addItems
stack traceback:
	[C]: in function 'assert'
	Provider:17: in method 'addItems'
	Floater:20: in local 'method'
	EventBus:89: in method 'publish'
	CombatRound:186: in method 'publish'
	Spikes:63: in method 'actionWith'
	Player:255: in local 'action'
	TileArbiter:27: in method 'moveTo'
	Tile:110: in method 'attemptedEntranceBy'
	Tile:390: in function <Tile:388>
	(...tail calls...)
	Player:192: in method 'moveBy'
	Player:140: in method 'executeKey'
	Player:186: in method 'keyPress'
	GameRunner:327: in method 'keyPress'
	Main:39: in function 'keyboard'

This is because I can’t just call directly to addItems, there are all those things that come back from the Bus. Got to do this:

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
    Bus:subscribe(self,self.addItemsFromBus, "addItemsToCrawl")
end

function Floater:addItemsFromBus(event, sender, info)
    self:addItems(info)
end

This should be better. And it is:

spikes

Now let’s change CombatRound to do the publish, but continue to return the array, for the tests:

function CombatRound:attack()
    if self.attacker:willBeAlive() and self.defender:willBeAlive() then
        self:append(self:display("  "))
        local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
        self:append(self:display(msg))
        self:attemptHit()
    end
    self:publish()
    return self:getCommandList()
end

And change GameRunner not to publish:

function GameRunner:initiateCombatBetween(attacker, defender)
    if attacker:willBeDead() or defender:willBeDead() then return end
    local co = CombatRound(attacker,defender)
    self:addToCrawl(co:attack())
end

That becomes:

function GameRunner:initiateCombatBetween(attacker, defender)
    if attacker:willBeDead() or defender:willBeDead() then return end
    CombatRound(attacker,defender):attack()
end

Now combat should work, without going through GameRunner to publish. And it does.

I think we can commit now, though we’re not quite done with addToCrawl. Commit: Combat publishes attack results itself.

I think there’s one more reference to addToCrawl:

function Decor:damage(aPlayer)
    local co = CombatRound(self,aPlayer)
    co:appendText("Ow! Something in there is sharp!")
    local damage = math.random(1,5)
    co:applyDamage(damage, "_speed")
    self.tile.runner:addToCrawl(co:getCommandList())
end

We convert that as before with Spikes:

function Decor:damage(aPlayer)
    local co = CombatRound(self,aPlayer)
    co:appendText("Ow! Something in there is sharp!")
    local damage = math.random(1,5)
    co:applyDamage(damage, "_speed")
    co:publish()
end

That’s the last call to the addToCrawl in GameRunner. We remove the method. Commit: combat publication does not use GameRunner.

Now the GameRunner does hold on to the Floater. It uses it twice, once to draw it, which we need to have somewhere, and once to publish the original text messages for the level:

function GameRunner:createLevel(count)
    self.dungeonLevel = self.dungeonLevel + 1
    if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    TileLock=false
    self:createTiles()
    self:clearLevel()
    self:createRandomRooms(count)
    self:connectRooms()
    self:convertEdgesToWalls()
    self:placePlayerInRoom1()
    self:placeWayDown()
    self:placeSpikes(5)
    self:setupMonsters(6)
    self.keys = self:createThings(Key,5)
    self:createThings(Chest,5)
    self:createLoots(10)
    self:createDecor(30)
    self:createButtons()
    self.cofloater:runCrawl(self:initialCrawl(self.dungeonLevel))
    self:startTimers()
    self.playerCanMove = true
    TileLock = true
end

You’d think we could use a publish to make this happen, but it is going through a different Floater method, runCrawl. Let’s inspect that.

function Floater:runCrawl(array)
    self.provider:addItems(array)
    self:startCrawl()
end

function Floater:startCrawl()
    self.yOff = self.yOffsetStart
    self.buffer = {}
    self:fetchMessage()
end

I suspect we can start the crawl with nothing in the provider, and then just use addItems from GameRunner. Certainly the provider can run dry during the game.

I’ll move the startCrawl to the init:

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
    self:startCrawl()
    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
    Bus:subscribe(self,self.addItemsFromBus, "addItemsToCrawl")
end

And remove it from runCrawl:

function Floater:runCrawl(array)
    self.provider:addItems(array)
end

If this works, I can use publish in GameRunner. An odd thing happens: the crawl is jerky, pausing a bit between lines. I have a plan for that but first I will reverse out those last two changes and see if it goes smoothly again.

This is curious. It’s still going in a jerky fashion.

Some judicious checkouts using Working Copy, and I find that the jerky behavior went in on the commit named WayDown publishes messages via Bus.

Best look and see what changed. In Floater we have this:

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
end

function Floater:addTextToCrawl(event, sender, info)
    self:addItem(CombatRound():display(info.text or ""))
end

Prior to that, we went through GameRunner:

function GameRunner:addTextToCrawl(aString)
    self.cofloater:addItem(CombatRound():display(aString))
end

I wonder whether we’re getting empty lines.

Check out current version. A quick test says we’re not asking for any blank lines or empty lines.

In the first version with the jerkiness, If I comment out one line, it goes away:

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
    --Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
end

I would have said that in that version, no one was even doing publishes, except for WayDown, and I’m not interacting with it.

I put a print in every method of EventBus, and other than the tests, there’s just one init and one subscribe, the one from Floater. Still jerky.

I’ll early exit the subscribe.

function EventBus:subscribe(listener, method, event)
    print("EB Subscribe")
    if true then return end
    local subscriptions = self:subscriptions(event)
    subscriptions[listener] = method
end

It’s not jerky. Let me double check that. Comment out the if, it’s jerky.

Most peculiar.

OK, this is curious. I added a dump method to EventBus, to dump out the events table. It contains an entry for each event that is subscribed, indexed by event string, and containing a table for each subscriber, indexed by the subscriber (table) and method.

I dump here:

    print("Floater before")
    Bus:dump()
    Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
    print("Floater after")
    Bus:dump()

And also after subscribe. Before the subscribe in Floater, there are 31 entries in the Bus.events. After the subscribe, there are 32. Oh.

One issue is that I found it necessary to create the bus before running the tests. We should at least unsubscribe all before running.

That’s this:

function EventBus:unsubscribeAll(object)
    print("EB unsubs")
    for event,subs in pairs(self.events) do
        subs[object] = nil
    end
end

I think that can just clear the events table. What was I thinking?

And in Main:

function setup()
    Bus = EventBus()
    if CodeaUnit then 
        runCodeaUnitTests() 
        CodeaTestsVisible = true
    end
    local seed = math.random(1234567)
    print("seed=",seed)
    math.randomseed(seed)
    showKeyboard()
    TileLock = false
    Bus:unsubscribeAll()
    Runner = GameRunner()
    Runner:createLevel(12)
    TileLock = true
    if false then
        sprite(xxx)
    end
end

That speeds up the crawl. But why is it slowing down? No one is even using it.

I’m wrong about the unsubscribe all. It needs to do a single object. But we need some kind of a reset. Let me replace the line in Main:

    Bus = EventBus()

Bizarre. It’s jerky.

After further play, I have a clue, and some confusion. When I backed out the change above, and went back to unsubscribeAll, it didn’t go back to smooth. Further, I’m seeing some instances where it’s jerky for a while and then smooths out.

I’m starting to think that the tests are creating too much garbage and causing the game to choke until collection is completed. That would be a problem for a long time if we were holding on to a lot of storage.

And we are. The Floater has a GameRunner. That makes me think that when those 32 objects show up in the subscriptions, there are 32 full games, full of tiles and all, hanging down there. That would slow anyone down.

I’m going to take a break. This has tired me out, but I think I have identified the issue.

Much Later

I took most of the rest of the day off. I’m quite sure that the issue is that we’re creating 30 dungeons and they’re stuck in the Bus. We’ve probably filled the iPad up with tiles and other stuff.

The question, of course, is what to do. One will be to be sure to clear out the Bus more often. Another might be to make it a weak table so that otherwise unreferenced things get dropped. That could backfire, but it’s not likely that the only object holding onto important stuff is the Bus.

Another possibility is to provide a fake Bus during testing, or turn off the Bus altogether.

A quick change to the tests, the one that creates lots of GameRunners, plus making sure to create a new Bus right before the “real” GameRunner, resolves the problem

I’m going to wrap right here and take this up in the morning.

See you then!


D2.zip