GitHub Repo

Having written up the theory of the game this morning, this afternoon I feel pretty certain that what’s supposed to happen isn’t really happening. We need to see what’s to be done about it.

One of the “rules” if my game setup i that every object, when it is removed from the mix, is supposed to be sent finalize and allowed to return objects to be put back into the mix. I am really quite sure that that’s not always taking place.

It seems to me that this should be handled in the SpaceObjectCollection class itself.

I’m interested in senders of finalize.

In Game, there is a direct call to finalize anything returned from interactionWith:

    fun processInteractions() {
        val toBeRemoved = colliders()
        if ( toBeRemoved.size > 0 ) {
            knownObjects.removeAll(toBeRemoved)
        }
        for (removedObject in toBeRemoved) {
            val addedByFinalize = removedObject.finalize()
            knownObjects.addAll(addedByFinalize)
        }
    }

beginInteraction doesn’t return anything, so this is not a concern.

finishInteraction returns a transaction and the loop applies the accumulation of all those:

    private fun finishInteractions() {
        val buffer = Transaction()
        knownObjects.forEach {
            val result: Transaction = it.finishInteraction()
            buffer.accumulate(result)
        }
        knownObjects.applyChanges(buffer)
    }

Apply changes, however, looks like this:

class SpaceObjectCollection ...
    fun applyChanges(t: Transaction) {
        t.applyChanges(this)
    }

class Transaction ...
    fun applyChanges(spaceObjectCollection: SpaceObjectCollection) {
        spaceObjectCollection.removeAll(removes)
        spaceObjectCollection.addAll(adds)
    }

That clearly does not finalize anything returned in a transaction. And back in SpaceObjectCollection:

    fun removeAll(moribund: Set<SpaceObject>): Boolean{
        return spaceObjects.removeAll(moribund.toSet())
    }

No finalization. We can do this:

    fun removeAll(moribund: Set<SpaceObject>): Boolean{
        moribund.forEach { spaceObjects += it.finalize() }
        return spaceObjects.removeAll(moribund)
    }

Method would better be named removeAndFinalizeAll, wouldn’t it?

Now here:

    fun processInteractions() {
        val toBeRemoved = colliders()
        if ( toBeRemoved.size > 0 ) {
            knownObjects.removeAndFinalizeAll(toBeRemoved)
        }
        for (removedObject in toBeRemoved) {
            val addedByFinalize = removedObject.finalize()
            knownObjects.addAll(addedByFinalize)
        }
    }

We can remove the finalization part.

    fun processInteractions() {
        val toBeRemoved = colliders()
        if ( toBeRemoved.size > 0 ) {
            knownObjects.removeAndFinalizeAll(toBeRemoved)
        }
    }

This deserves a run of the tests. A test fails, this one:

    @Test
    fun `transaction can add and remove`() {
        val coll = SpaceObjectCollection()
        val aOne = newAsteroid()
        coll.add(aOne)
        val t = Transaction()
        val aTwo = newAsteroid()
        t.add(aTwo)
        t.remove(aOne)
        coll.applyChanges(t)
        assertThat(coll.spaceObjects).contains(aTwo)
        assertThat(coll.spaceObjects).doesNotContain(aOne)
        assertThat(coll.size).isEqualTo(1)
    }

The asteroid slated for removal was not previously finalized and now it is. It will have split, and generated a Score. Three more objects than expected. I’d like to have a better way of checking these collections. Let’s see, what can we check that won’t split. I think we need to do ships, which currently don’t do anything cute on finalizing. When we do ship explosions, this may break again.

    @Test
    fun `transaction can add and remove`() {
        val coll = SpaceObjectCollection()
        val shipOne = newShip()
        coll.add(shipOne)
        val t = Transaction()
        val shipTwo = newShip()
        t.add(shipTwo)
        t.remove(shipOne)
        coll.applyChanges(t)
        assertThat(coll.spaceObjects).contains(shipTwo)
        assertThat(coll.spaceObjects).doesNotContain(shipOne)
        assertThat(coll.size).isEqualTo(1)
    }

This passes. We’re green. I want to see who else is calling finalize. Other than tests, the spot in SpaceObjectCollection is now the sole caller, and that is as it should be. After testing I commit with a message that expresses how I feel: “improve finalize, move to SpaceObjectCollection. Everything is covered now, I believe. Hard to be sure …”.

If something has gotten through the cracks, I’m not at all certain how we could be sure. I think improving the SpaceObjectCollection might help. Let’s see if we can be sure everything funnels down as it should:

class SpaceObjectCollection {
    val spaceObjects = mutableListOf<SpaceObject>()

    fun add(spaceObject: SpaceObject) {
        spaceObjects.add(spaceObject)
    }
    
    fun addAll(newbies: Collection<SpaceObject>){
        spaceObjects.addAll(newbies)
    }

    fun applyChanges(t: Transaction) {
        t.applyChanges(this)
    }

    fun collectFromPairs(pairCondition: (SpaceObject, SpaceObject) -> List<SpaceObject>): MutableSet<SpaceObject> {
        val pairs = mutableSetOf<SpaceObject>()
        pairsToCheck().forEach { p -> pairs.addAll(pairCondition(p.first, p.second))
        }
        return pairs
    }

    fun forEach(spaceObject: (SpaceObject)->Unit) = spaceObjects.forEach(spaceObject)

    fun pairsToCheck(): List<Pair<SpaceObject, SpaceObject>> {
        val pairs = mutableListOf<Pair<SpaceObject, SpaceObject>>()
        spaceObjects.indices.forEach { i ->
            spaceObjects.indices.minus(0..i).forEach { j ->
                pairs.add(spaceObjects[i] to spaceObjects[j])
            }
        }
        return pairs
    }

    fun removeAndFinalizeAll(moribund: Set<SpaceObject>): Boolean{
        moribund.forEach { spaceObjects += it.finalize() }
        return spaceObjects.removeAll(moribund)
    }

    val size get() = spaceObjects.size
}

I wonder if the objects should be a Set. We really don’t want duplicates. But we use the indices in the pair logic. Ah. Sets produce them, somehow. But we can’t access Sets by index. Let’s maintain the set property ourselves.

    fun add(spaceObject: SpaceObject) {
        if (spaceObjects.contains(spaceObject)) return
        spaceObjects.add(spaceObject)
    }
    
    fun addAll(newbies: Collection<SpaceObject>){
        spaceObjects.forEach { add(it) }
    }

Makes me wonder about the speed of asList but we interact a lot more often than we add, so we’ll leave this. Test. A test fails. I suspect contains checks equality, not identity. Grr. We could override equals and hashCode, but that doesn’t appeal to me. And, with this definition of equal, we can’t maintain a set easily anyway, since two objects could in principle look alike enough. We’ll leave well enough alone.

I think we’ve resolved the issue we came in with, ensuring that all objects are sent finalize. They do not, in general, care, but they might.

Another thing that should be done is to have all the methods that currently return lists of objects to be added or removed instead return a Transaction. That would allow implementors to return both adds and removes from any of those methods.

Let’s start with update:

    open fun update(deltaTime: Double): List<SpaceObject> { return emptyList() }

Will IDEA’s “Change Signature” help us with this? I doubt it will help much. But we can trek through the errors. For example:

    fun control(ship: SolidObject, deltaTime: Double): Transaction {
        if (hyperspace) {
            hyperspace = false
            recentHyperspace = true
            return Transaction().also{ it.addAll(listOf(SolidObject.shipDestroyer(ship))) }
        }
        turn(ship, deltaTime)
        accelerate(ship, deltaTime)
        return Transaction().also { it.addAll(fire(ship)) }
    }

And

    override fun update(deltaTime: Double): Transaction {
        if (elapsedTime < 3.0) return Transaction()

        val toAdd = mutableListOf<SpaceObject>()
        for (i in 1..numberToCreate) {
            val a = SolidObject.asteroid((U.randomEdgePoint()))
            toAdd.add(a)
        }
        done = true
        return Transaction().also { it.addAll(toAdd) }
    }

I am nervous about this but Kotlin should lead me through the necessary changes. The ones here could be pushed deeper but first let’s just get everything being done via Transaction in SpaceObjectCollection.

    fun tick(deltaTime: Double) {
        knownObjects.applyChanges(cumulativeTransactionFromUpdates)
        cumulativeTransactionFromUpdates.clear()
        knownObjects.forEach { cumulativeTransactionFromUpdates.accumulate(it.tick(deltaTime)) }
    }

I trek through a bunch of tests that need to rip the adds out of the newly resulting transaction, but having done that blindly, they all run. The game works. Commit: update functions now return a Transaction.

Summary

This was, I thought, the simplest of the changes to allow everyone to produce a Transaction rather than only a list to add or a list to remove. It was rather extensive and ad hoc. Before doing the next one, I want to think about whether there is a better way to do this.

As a related comment, the return Transaction().also{it.xyz(abc)} notation, while idiomatic Kotlin, isn’t always as clear as it might be. We may wish to have a more capable constructor on Transaction.

I’d also note that this scheme where objects interact to create and destroy others, especially during finalization, makes individual tests a bit tricky, as when a test asteroid was finalized and split into three new objects. It did the right thing, but not what the test expected.

Is this an inevitable effect of this design? Or is there a way around it? One possibility might be a different SpaceObjectCollection for testing. It’s a concern and at this moment I don’t have an answer.

For now, the evil of the day is sufficient thereto. See you next time!