Kotlin 174: Are there mice in here?

Best laid plans scarcely get past the first paragraph.

I’m really tempted to replace Subscribers with inheritance, but if it’s as simple as I think, there’s not much benefit to the code. There would be a kind of meta benefit, in that we could compare both ways (again) but I don’t expect to change any minds by so doing.

Instead, my morning plan is to improve the ship’s life cycle, which is rather complex. We’ll start by reviewing Ship’s code. Makes sense, right?

Ship is the second longest class in the system, at 99 lines. Only Saucer is larger, at 107 last time I counted. We’ll not review all Ship’s details, just the bits relevant to life cycle. That will be plenty:

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithSaucer = { saucer, trans -> checkCollision(saucer, trans) },
        interactWithMissile = { missile, trans -> checkCollision(missile, trans) },
        interactWithShipDestroyer = { _, trans -> trans.remove(this) },
        draw = this::draw,
        finalize = this::finalize
    )

    private fun checkCollision(other: Collider, trans: Transaction) {
        if (weAreCollidingWith(other)) {
            trans.remove(this)
            trans.add(Splat(this))
        }
    }

    private fun weAreCollidingWith(other: Collider): Boolean = Collision(other).hit(this)


    private fun finalize(): List<ISpaceObject> {
        if ( deathDueToCollision() ) {
            position = U.CENTER_OF_UNIVERSE
            velocity = Velocity.ZERO
            heading = 0.0
        } else {
            position = U.randomPoint()
        }
        controls.recentHyperspace = false
        return emptyList()
    }

    private fun deathDueToCollision(): Boolean = !controls.recentHyperspace

The first bits are pretty straightforward: we check interactions to see if we’re colliding and if we are, we remove ourselves and add a Splat. If there’s a destroyer, we just remove ourselves. I think this is because the Destroyer creates a Splat. If it does, there’s a bit of an inconsistency that we might want to address. If the Ship ever moves from having a Splat explosion to something more fancy, we’d probably want to do it all in here.

We’ll be reviewing the ShipDestroyer anyway as part of this effort. Let’s do it now.

Oh my! The code shows no Splat being created in Destroyer (so it must be somewhere else), but I tested the program by going to hyperspace and discovered a serious bug. When you go to hyperspace, the new ship counting logic in ScoreKeeper thinks that you’ve been destroyed, so you lose a ship from your inventory. That won’t do. We need to fix that first.

We should also think about how we made the mistake and how to test for it.

Ship Inventory

The bug is here:

class ShipChecker ...
    override val subscriptions = Subscriptions(
        beforeInteractions = { missingShip = true },
        interactWithShip = { _, _ -> missingShip = false },
        afterInteractions = { trans ->
            if ( missingShip && scoreKeeper.takeShip()) {
                trans.add(ShipMaker(ship, scoreKeeper))
                trans.remove(this)
            }
        }
    }

We should not debit the ship count (scoreKeeper.takeShip()) if we’re in hyperspace.

My first move will be to do a hot fix, so we can get our users back on the air. I sure would be embarrassed if we had shipped machines all over the country with this bug in them. I’m embarrassed enough even now. It’s so tempting to fix these mistakes quietly, rather than admit them to you. But we do make mistakes, and we try to learn from them. It’s a key aspect of growing as developer.

I think we can ask the ship deathDueToCollision and condition the ship taking on that.

If I hot fix right now, I’ll test in the game, not with a test. Clearly I need a test. How urgent is this fix? I can check this idea in … time me … 0855-

        afterInteractions = { trans ->
            if ( missingShip && (!ship.deathDueToCollision() || scoreKeeper.takeShip())) {
                trans.add(ShipMaker(ship, scoreKeeper))
                trans.remove(this)
            }
        }

Fix does not work 0858, Three minutes. The hyperspace flag is pretty volatile, so maybe it’s already cleared. Or maybe I did the logic wrong. I could recast the if statement to be sure. Clearly a quick fix is out, I’m already stuck with debugging or tracing the logic.

OK. Calm down. Don’t panic. Panic doesn’t help. Apply brain to problem. Ah. We saw where it turned off the flag:

    private fun finalize(): List<ISpaceObject> {
        if ( deathDueToCollision() ) {
            position = U.CENTER_OF_UNIVERSE
            velocity = Velocity.ZERO
            heading = 0.0
        } else {
            position = U.randomPoint()
        }
        controls.recentHyperspace = false
        return emptyList()
    }

The ship is gone, so it has finalized, so it has turned off the flag. We do have another way to know whether the ship is in hyperspace … when it died from a collision, it will be at the center of the universe.

HyperspaceOperation checks that way:

    private fun inHyperspace() = ship.position != U.CENTER_OF_UNIVERSE

We can add that to ShipChecker and use it.

        afterInteractions = { trans ->
            if ( missingShip && (inHyperspace() || scoreKeeper.takeShip())) {
                trans.add(ShipMaker(ship, scoreKeeper))
                trans.remove(this)
            }
        }
    )

    private fun inHyperspace() = ship.position != U.CENTER_OF_UNIVERSE

Works. I am confident. Commit: hot fix to bug where hyperspace cost player a ship every time.

It is now 0911. The commit took some time because there were other changes that had to be looked at. Those should have been committed but my afternoon work, well, I get a bit sloppy.

I do not claim to be a good person, just a person.

Do we even have any tests for the ScoreKeeper’s ship inventory? We’re here to clean up this area, and it’s clear that we have at least one hole in the testing.

We do test the ScoreKeeper. It really just maintains a count so that when you takeShip it tells you whether you got one. It wasn’t involved in this bug: we shouldn’t have asked it for a ship.

Meta-Commentary

This is a moderately complex bit of game behavior, no matter how the game is built. That’s not an excuse: it’s an observation. It should serve to “prove” that we need decent tests in the area. The issue may be exacerbated by the way we use separate objects to create behavior, as here, with the Ship removing itself, and the ShipChecker noticing the loss and doing something about it.

In a game with a God object, we’d have similar code:

class ImaginaryGodObject
    ...
    // deal with Ship
    if (ship is dead && shipCount > 0) ...

I think we could just as likely forget to check for hyperspace in that code as in this. Maybe not. There’s no way to do the experiment.

But the question really comes down to the ship’s state, is it in hyperspace, or not? Is it dead, or not?

Meta-meta commentary: We could, I suppose, add a ship to the ScoreKeeper when we go to hyperspace. Then when checker removed it, we’d break even. That surely classifies as a horrible hack. I’m glad I didn’t do that. I’m ashamed even to have thought of it. No, creativity is good, options are good. But it’s an evil idea.

I think we’d like to test this whole cycle, and try to learn something about this independent object design. Arguably, if all the logic were centralized somewhere, testing it would be easier. Be that as it may, we are where we are, so let’s try to come up with a relatively easy way to test this.

We have a ShipCheckerAndMakerTest file. We’ll put our new tests there.

There are a lot of tests here already and their names are:

    fun `ShipChecker does nothing if ship seen`() {
    fun `ShipChecker does nothing if ship seen via withOther`() {
    fun `creates ShipMaker if no ship seen`() {
    fun `maker delays U MAKER_DELAY seconds`() {
    fun `maker makes after U MAKER_DELAY seconds`() {
    fun `maker makes only when safe`() {
    fun `makes with ship features unchanged`() {
    fun `maker counts asteroids`() {
    fun `hyperspace failure checks`() {

The withOther test is obsolete and an exact duplicate of the prior test. Remove it.

The creates test is OK. We should really have two, however, checking whether we removed a ship from the ScoreKeeper.

We’ll write a couple of new tests for that, and try to work as directly with the objects as we can manage.

    @Test
    fun `debits ScoreKeeper if ship is dead`() {
        
    }

    @Test
    fun `does not debit ScoreKeeper if ship is in hyperspace`() {

    }

I think this test should pass:

    @Test
    fun `debits ScoreKeeper if ship is dead`() {
        val scoreKeeper = ScoreKeeper(1)
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        val checker = ShipChecker(ship, scoreKeeper)
        checker.subscriptions.beforeInteractions()
        val trans = Transaction()
        checker.subscriptions.afterInteractions(trans)
        assertThat(scoreKeeper.shipCount).isEqualTo(0)
    }

It does, but another one fails:

    @Test
    fun `ScoreKeeper provides ships to be made`() {
        val keeper = ScoreKeeper(2)
        val ship = Ship(Point.ZERO)
        val checker = ShipChecker(ship, keeper)
        val t1 = Transaction()
        checker.subscriptions.beforeInteractions()
        checker.subscriptions.afterInteractions(t1)
        assertThat(t1.adds.size).isEqualTo(1)
        val t2 = Transaction()
        checker.subscriptions.beforeInteractions()
        checker.subscriptions.afterInteractions(t2)
        assertThat(t2.adds.size).isEqualTo(1)
        val t3 = Transaction()
        checker.subscriptions.beforeInteractions()
        checker.subscriptions.afterInteractions(t3)
        assertThat(t3.adds.size).isEqualTo(0)
    }

That one fails because the ship is not at universe center, and our new code concludes that it is in hyperspace, and therefore does not debit the ScoreKeeper’s ship count. So the keeper will never run out. Fixed. Now the other test:

    @Test
    fun `does not debit ScoreKeeper if ship is in hyperspace`() {
        val scoreKeeper = ScoreKeeper(1)
        val ship = Ship(Point.ZERO)
        val checker = ShipChecker(ship, scoreKeeper)
        checker.subscriptions.beforeInteractions()
        val trans = Transaction()
        checker.subscriptions.afterInteractions(trans)
        assertThat(scoreKeeper.shipCount).isEqualTo(1)
    }

This will pass. It does. However, it is preserving the hack of checking the ship location to determine whether it is in hyperspace or killed. That should really be made more explicit. The ship should set a state in itself indicating why it is absent from the scene.

I think this might work:

Have an inHyperspace flag member in Ship.
Set it false in update.
Set it true if we go to hyperspace
Check it in the objects that want to know.

Let’s codify that in these tests:

    @Test
    fun `debits ScoreKeeper if ship is dead`() {
        val scoreKeeper = ScoreKeeper(1)
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        ship.inHyperspace = false
        val checker = ShipChecker(ship, scoreKeeper)
        checker.subscriptions.beforeInteractions()
        val trans = Transaction()
        checker.subscriptions.afterInteractions(trans)
        assertThat(scoreKeeper.shipCount).isEqualTo(0)
    }

    @Test
    fun `does not debit ScoreKeeper if ship is in hyperspace`() {
        val scoreKeeper = ScoreKeeper(1)
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        ship.inHyperspace = true
        val checker = ShipChecker(ship, scoreKeeper)
        checker.subscriptions.beforeInteractions()
        val trans = Transaction()
        checker.subscriptions.afterInteractions(trans)
        assertThat(scoreKeeper.shipCount).isEqualTo(1)
    }

IDEA is sure that there is no such flag. Implement it:

    override fun update(deltaTime: Double, trans: Transaction) {
        inHyperspace = false
        dropScale -= U.DROP_SCALE/60.0
        if (dropScale < 1.0 ) dropScale = 1.0
        controls.control(this, deltaTime, trans)
        move(deltaTime)
    }

At this point, it’ll never go true in the real code. How does Controls do it? I think it actually removes the ship:

    fun control(ship: Ship, deltaTime: Double, trans: Transaction) {
        if (hyperspace) {
            hyperspace = false
            recentHyperspace = true
            trans.addAll(listOf(ShipDestroyer()))
        }
        turn(ship, deltaTime)
        accelerate(ship, deltaTime)
        trans.addAll(fire(ship))
    }

I don’t like that much. Let’s tell the ship instead:

    fun control(ship: Ship, deltaTime: Double, trans: Transaction) {
        if (hyperspace) {
            hyperspace = false
            ship.enterHyperspace(trans)
        }
        turn(ship, deltaTime)
        accelerate(ship, deltaTime)
        trans.addAll(fire(ship))
    }

    fun enterHyperspace(trans: Transaction) {
        trans.remove(this)
        inHyperspace = true
    }

Let’s change the tests, not to set the flag but the second one will call enterHyperspace:

    @Test
    fun `does not debit ScoreKeeper if ship is in hyperspace`() {
        val scoreKeeper = ScoreKeeper(1)
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        val removalTrans = Transaction()
        ship.enterHyperspace(removalTrans)
        assertThat(removalTrans.firstRemove()).isEqualTo(ship)
        val checker = ShipChecker(ship, scoreKeeper)
        checker.subscriptions.beforeInteractions()
        val makerTrans = Transaction()
        checker.subscriptions.afterInteractions(makerTrans)
        assertThat(scoreKeeper.shipCount).isEqualTo(1)
    }

I checked to see the remove, since it was to hand. Test should fail right now, if I’m not mistaken. Good news, it fails. Now to fix the people who care about whether we’re in hyperspace. There are two that I know of.

class ShipChecker
        afterInteractions = { trans ->
            if ( missingShip && (ship.inHyperspace || scoreKeeper.takeShip())) {
                trans.add(ShipMaker(ship, scoreKeeper))
                trans.remove(this)
            }
        }

class ShipMaker
        afterInteractions = { trans->
            if (ship.inHyperspace || elapsedTime > U.MAKER_DELAY && safeToEmerge) {
                replaceTheShip(trans)
            }
        }

I expect green. I get it. I find another bug in the game … the ship position is not randomized on going to hyperspace. Let’s test that and then fix it.

The issue right now is that the random position setting is done here:

    private fun finalize(): List<ISpaceObject> {
        if ( deathDueToCollision() ) {
            position = U.CENTER_OF_UNIVERSE
            velocity = Velocity.ZERO
            heading = 0.0
        } else {
            position = U.randomPoint()
        }
        controls.recentHyperspace = false
        return emptyList()
    }

I can fix the problem by checking the right condition:

    private fun finalize(): List<ISpaceObject> {
        if ( inHyperspace ) {
            position = U.randomPoint()
        } else {
            position = U.CENTER_OF_UNIVERSE
            velocity = Velocity.ZERO
            heading = 0.0
        }
        controls.recentHyperspace = false
        return emptyList()
    }

Probably don’t need the recent flag either. That makes us green. But why wait? Let’s randomize position on entry to hyperspace and remove finalize from Ship.

    fun enterHyperspace(trans: Transaction) {
        position = U.randomPoint()
        inHyperspace = true
        trans.remove(this)
    }

And remove finalize and the subscription to it. Still green? Yes but in the game we see another issue … we need to set back to center on a normal death. Improve that test, forcing an improvement to the code:

    @Test
    fun `ship randomizes position on hyperspace entry`() {
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        val trans = Transaction()
        ship.enterHyperspace(trans)
        assertThat(trans.firstRemove()).isEqualTo(ship)
        assertThat(ship.position).isNotEqualTo(U.CENTER_OF_UNIVERSE)
    }

    @Test
    fun `ship goes to center on collision death`() {
        val ship = Ship(U.randomPoint())
        val trans = Transaction()
        ship.collision(trans)
        assertThat(trans.firstRemove()).isEqualTo(ship)
        assertThat(ship.position).isEqualTo(U.CENTER_OF_UNIVERSE)
    }

This demands a new method, collision (and that we use it).

    fun collision(trans: Transaction) {
        position = U.CENTER_OF_UNIVERSE
        inHyperspace = false // belt and suspenders
        trans.remove(this)
    }

And I need to find all the ship removals and pass them through this method.

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithSaucer = { saucer, trans -> checkCollision(saucer, trans) },
        interactWithMissile = { missile, trans -> checkCollision(missile, trans) },
        interactWithShipDestroyer = { _, trans -> collision(trans) },
        draw = this::draw,
    )

    private fun checkCollision(other: Collider, trans: Transaction) {
        if (weAreCollidingWith(other)) {
            collision(trans)
            trans.add(Splat(this))
        }
    }

Test. Some tests fail. In game testing shows me the splat in the wrong place. Need this:

    private fun checkCollision(other: Collider, trans: Transaction) {
        if (weAreCollidingWith(other)) {
            collision(trans)
        }
    }

    fun collision(trans: Transaction) {
        trans.add(Splat(this))
        trans.remove(this)
        position = U.CENTER_OF_UNIVERSE
        inHyperspace = false // belt and suspenders
    }

However … I see that when I return, I have velocity, and I’m getting two splats on hyperspace failure. Fix the latter, which is here:

class HyperspaceOperation
    private fun destroyTheShip(trans: Transaction) {
        trans.add(ShipDestroyer())
        trans.add(Splat(ship))
    }

Remove that splat, the ship always creates one now.

We have tests failing, however, which is good news in that behavior has changed, but irritating because now I have to fix them.

    fun `create game`() {
        val game = Game()
        val asteroid = Asteroid(Vector2(100.0, 100.0), Vector2(50.0, 50.0))
        val ship = Ship(
            position = Vector2(1000.0, 1000.0)
        )
        game.add(asteroid)
        game.add(ship)
        val trans = game.changesDueToInteractions()
        assertThat(trans.removes.size).isEqualTo(0)
        for (i in 1..12 * 60) game.tick(1.0 / 60.0)
        val x = asteroid.position.x
        val y = asteroid.position.y
        assertThat(x).isEqualTo(100.0 + 12 * 50.0, within(0.1))
        assertThat(y).isEqualTo(100.0 + 12 * 50.0, within(0.1))
        val trans2 = game.changesDueToInteractions()
        assertThat(trans2.removes.size).isEqualTo(2)
    }

The last assertion fails, finding one not two. The one that’s in there is ship, and I presume that asteroid should have been in there as well.

This is not good. Something has gone wrong and I don’t immediately see what. The changes in flight right now are fairly extensive and I’m a long way from the last commit. If I revert I’ll have to do a lot of work. Sunk cost fallacy but I’m feeling it.

If the ship was … oh no … I see the problem. The ship is moving itself as soon as it knows it is dead. It can’t do that … if it does … the asteroid, whose check came second, doesn’t see the ship in a colliding position. We have to set the position either in finalize, or later. For now, we’ll put the finalize back.

I use IDEA’s local history to put finalize back … now to fix things up.

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans -> checkCollision(asteroid, trans) },
        interactWithSaucer = { saucer, trans -> checkCollision(saucer, trans) },
        interactWithMissile = { missile, trans -> checkCollision(missile, trans) },
        interactWithShipDestroyer = { _, trans -> collision(trans) },
        draw = this::draw,
        finalize = this::finalize
    )

    fun enterHyperspace(trans: Transaction) {
        inHyperspace = true
        trans.remove(this)
    }

    fun collision(trans: Transaction) {
        trans.add(Splat(this))
        trans.remove(this)
        inHyperspace = false // belt and suspenders
    }

    private fun finalize(): List<ISpaceObject> {
        if ( inHyperspace ) {
            position = U.randomPoint()
        } else {
            position = U.CENTER_OF_UNIVERSE
            velocity = Velocity.ZERO
            heading = 0.0
        }
        return emptyList()
    }

Test. Good, the only one that fails are the two that check where the ship goes, and those need to call finalize:

    @Test
    fun `ship randomizes position on hyperspace entry`() {
        val ship = Ship(U.CENTER_OF_UNIVERSE)
        val trans = Transaction()
        ship.enterHyperspace(trans)
        assertThat(trans.firstRemove()).isEqualTo(ship)
        ship.finalize()
        assertThat(ship.position).isNotEqualTo(U.CENTER_OF_UNIVERSE)
    }

    @Test
    fun `ship goes to center on collision death`() {
        val ship = Ship(U.randomPoint())
        val trans = Transaction()
        ship.collision(trans)
        assertThat(trans.firstRemove()).isEqualTo(ship)
        ship.finalize()
        assertThat(ship.position).isEqualTo(U.CENTER_OF_UNIVERSE)
    }

Had to make finalize public, but as a Smalltalker, I don’t mind that. We are green and the game is nearly good. I do learn one new thing: after a collision death, I am supposed to be safe on emergence, but it happened that the Saucer was at zero and the safety check clearly doesn’t check for the saucer.

    override val subscriptions = Subscriptions (
        beforeInteractions = {
            safeToEmerge = true
            asteroidTally = 0
        },
        interactWithAsteroid = { asteroid, _ ->
            asteroidTally += 1
            safeToEmerge = safeToEmerge && !tooClose(asteroid)
        },
        afterInteractions = { trans->
            if (ship.inHyperspace || elapsedTime > U.MAKER_DELAY && safeToEmerge) {
                replaceTheShip(trans)
            }
        }
    )

Right. Add Saucer to that:

        interactWithSaucer = { saucer, _ ->
            asteroidTally += 1
            safeToEmerge = safeToEmerge && !tooClose(saucer)
        },

But I need a test for that, don’t I? There’s already a grotesquely long test for that:

    @Test
    fun `maker makes only when safe`() {
        val ship = Ship(
            position = U.CENTER_OF_UNIVERSE
        )
        val asteroid = Asteroid(U.CENTER_OF_UNIVERSE)
        val maker = ShipMaker(ship)
        val ignored = Transaction()
        maker.update(U.MAKER_DELAY, ignored)
        maker.update(0.01, ignored)
        maker.subscriptions.beforeInteractions()
        val notInteresting = Transaction()
        maker.subscriptions.interactWithAsteroid(asteroid, notInteresting)
        val nothing = Transaction()
        maker.subscriptions.afterInteractions(nothing)
        assertThat(nothing.adds).isEmpty()
        assertThat(nothing.removes).isEmpty()
        maker.subscriptions.beforeInteractions()
        // nothing
        val trans = Transaction()
        maker.subscriptions.afterInteractions(trans)
        assertThat(trans.adds.size).isEqualTo(2)
        assertThat(trans.adds).contains(ship)
        assertThat(trans.firstRemove()).isEqualTo(maker)
        assertThat(ship.velocity).isEqualTo(Velocity.ZERO)
        assertThat(ship.heading).isEqualTo(0.0)
    }

We can do better by testing at a lower level, I think. Let’s try it:

    @Test
    fun `asteroid or saucer clears safeToEmerge`() {
        val ship = Ship(
            position = U.CENTER_OF_UNIVERSE
        )
        val asteroid = Asteroid(U.CENTER_OF_UNIVERSE)
        val saucer = Saucer()
        saucer.position = U.CENTER_OF_UNIVERSE
        val maker = ShipMaker(ship)
        val ignored = Transaction()
        maker.subscriptions.beforeInteractions()
        assertThat(maker.safeToEmerge).isEqualTo(true)
        maker.subscriptions.interactWithAsteroid(asteroid, ignored)
        assertThat(maker.safeToEmerge).isEqualTo(false)
        maker.subscriptions.beforeInteractions()
        assertThat(maker.safeToEmerge).isEqualTo(true)
        maker.subscriptions.interactWithSaucer(saucer, ignored)
        assertThat(maker.safeToEmerge).isEqualTo(false)
    }

This passes.

I’m somewhat rattled. I typed an r when I meant Control+r, and removed a bunch of code, replacing it with an r, and it took me a while to fix it. Let me try the game one more time and see if I can’t get this commit load off my back.

Game seems OK. I’m going to commit. I’m sure we’re no worse off and think we’re better. This is not the level of confidence I’d like to have but it’s what I’ve got.

Commit: Extensive and tangled changes to collision logic, hyperspace detection, ship emergence and probably the hot dog cooker.

Not my finest hour. Let’s reflect.

Reflection

It all started, Doctor, when I found that problem with hyperspace costing us a ship. The hot fix was OK, but it broke my rhythm and took my mind off the purpose of the morning.

In my defense, Judge, the code managing whether or not we are in hyperspace was spread around between controls, and there was a hack where we checked the ship’s position to see if it was in hyperspace, so that getting it fully untangled really did require changes to controls, ship, checker, maker, and hyperspace operation.

What’s that, Judge? Who created the mess? Well, sir, I guess I did but it was the best I had at the time. But yes, I was in a mess of my own creation.

The good news is that we added a few tests, and refined some others, so that I think we have fairly decent coverage of the ship checker maker hyperspace collision logic. The tests are often too long and hard to understand, but they did find some problems and were readily fixed up.

And while my plan to really address the way the hyperspace and collision code works fell to nothing, that code is nonetheless a bit better, and shown to be working as intended, by virtue of the test improvements and code changes we made.

But did we do what I thought we would? Not remotely. I’d still like to take a look at reallocating the responsibilities across ship checker maker hyperspace operation and destroyer … but that will have to wait for another day.

I did notice something that I think would be nice. I would like for there to be a small splat when an asteroid breaks. It’s especially noticeable than when you hit a small one, it just vanishes. It’d be nice to get a small splat.

Let’s see if we can redeem ourselves with that.

Asteroid Splat

Splat has three constructors:

    constructor(ship: Ship) : this(ship.position, 2.0, ColorRGBa.WHITE)
    constructor(missile: Missile) : this(missile.position, 0.5, missile.color)
    constructor(saucer: Saucer) : this(saucer.position, 2.0, ColorRGBa.GREEN)

Let’s have the idea be that every asteroid collision produces an asteroid splat, which we’ll make of size 1, at least to start.

    constructor(asteroid: Asteroid) : this(asteroid.position, 1.0, ColorRGBa.WHITE)

Now in Asteroid:

    private fun dieIfColliding(missile: Missile, trans: Transaction) {
        if (Collision(missile).hit(this)) trans.remove(this)
    }

Let’s add the splat, and see some tests break.

    private fun dieIfColliding(missile: Missile, trans: Transaction) {
        if (Collision(missile).hit(this)) {
            trans.remove(this)
            trans.add(Splat(this))
        }
    }

In checking the in-game effect, I notice yet another anomaly. When a missile kills an asteroid, after a delay, we get a splat where the missile hit the asteroid. That may have happened a long time ago, but right now, it’s in the OneShot timeout:

    private val timeOut = OneShot(3.0) {
        it.remove(this)
        it.add(Splat(this))
    }

What has happened, oh this is rich, is that the missile has been finalized and dropped from the mix, but the OneShot has placed a DeferredAction in the mix, and that doesn’t know that the missile is dead and gone, so it drops the splat at the last place where the missile existed.

The remove is OK: it’s harmless to remove something that’s already removed. But we really need to cancel the DeferredAction.

Forgive me. I’m just going to code this.

class OneShot(private val delay: Double, private val action: (Transaction)->Unit) {
    var triggered = false
    var deferred: DeferredAction? = null
    fun execute(trans: Transaction) {
        if (!triggered) {
            triggered = true
            deferred = DeferredAction(delay, trans) {
                triggered = false
                action(it)
            }
        }
    }
    fun cancel(trans: Transaction) {
        if (deferred != null) trans.remove(deferred!!)
    }
}

The OneShot remembers the DeferredAction if it has made one. Cancel checks to see if there is one, and if there is, removes it.

Applying this in missile is extensive, because duplication:

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans ->
            if (checkCollision(asteroid)) {
                timeOut.cancel(trans)
                trans.remove(this)
                if (missileIsFromShip) trans.add(asteroid.getScore())
            }
        },
        interactWithSaucer = { saucer, trans ->
            if (checkCollision(saucer)) {
                timeOut.cancel(trans)
                trans.remove(this)
                if (missileIsFromShip) trans.add(saucer.getScore())
            }
        },
        interactWithShip = { ship, trans ->
            if (checkCollision(ship)) {
                timeOut.cancel(trans)
                trans.remove(this)
            }
        },
        interactWithMissile = { missile, trans ->
            if (checkCollision(missile)) {
                timeOut.cancel(trans)
                trans.remove(this)
            }
        },

Let’s extract a method:

    override val subscriptions = Subscriptions(
        interactWithAsteroid = { asteroid, trans ->
            if (checkCollision(asteroid)) {
                if (missileIsFromShip) trans.add(asteroid.getScore())
                terminateMissile(trans)
            }
        },
        interactWithSaucer = { saucer, trans ->
            if (checkCollision(saucer)) {
                if (missileIsFromShip) trans.add(saucer.getScore())
                terminateMissile(trans)
            }
        },
        interactWithShip = { ship, trans ->
            if (checkCollision(ship)) {
                terminateMissile(trans)
            }
        },
        interactWithMissile = { missile, trans ->
            if (checkCollision(missile)) {
                terminateMissile(trans)
            }
        },
        draw = this::draw,
    )

    private fun terminateMissile(trans: Transaction) {
        timeOut.cancel(trans)
        trans.remove(this)
    }

Still some duplication here but fixing it up will require a new interface and that’s too much for this quick change.

Now let’s see what tests break. None do. That could be good news about the tests, or bad news in that there aren’t enough. Either way, I’m sure this works and I’m sure that it’s time for a break. Commit: Missiles cancel their timeout splat on collision.

Let’s sum up.

Summary

Very ragged day. I’d like to blame that defect derailing me, but I think I’m not quite at 100 percent anyway. All the changes went in well, and almost everything worked on the first try, and we even got some new tests out of the deal. And a nice little feature with asteroid fragments splatting.

Tests are too hard to write, and as a result there aren’t enough of them. In addition, there are so many cases to think about, since some interactions score and some do not and so on.

We’ve added 25 lines to the code today, 9 of them in Missile but making it better, including cancelling its splat. Ship grew by 9, ShipMaker by 4, Asteroid by 3.

Overall, despite the fact that things work and the game and tests continue to improve, I am not entirely pleased. I’m not sure why, but part of it, I think, is the inherent oddness of the independent objects and their interactions.

Today’s issue with the spurious missile splat was caused by my new darling, the OneShot+DeferredAction. It makes sense that a DeferredAction would have to be cancelled, but you’d like it to be more automatic. A hack would be possible: instead of cancelling the action, we could move the missile off screen, but it’s hard to argue that that’s better.

Probably the problem is endemic to the future orientation: a similar thing would happen with a tween in Codea, set up to delay and then send a message.

Perhaps the Delay should have sent a message to the Missile. Perhaps it should have checked to see if the missile was dead. There are a number of ways to deal with it … but the fundamental issue remains that it was too easy to forget the case.

It’s at least possible that the OneShot/DeferredAction is too cool for school. A simple timing check would have stopped running and wouldn’t have fired the splat because the missile was out of the mix. Something to think about before we get in much deeper with the OneShot/DeferredAction stuff.

I’ll have to reflect on my overall discomfort. There’s something awry here. It’s exacerbated by some testing weakness, but I feel that there’s more going on than that, even though I think that all these cases would plague a more centralized design as much as they do this design.

Possibly a centralized design would be better situated to build a table of interactions and fill in the cases. In fact, that wouldn’t hurt me, either, if I kept the table up to date, even on paper.

I need to reflect. Decent results, but there was a defect in the shipped version and that always throws me off.

I’ll report further next time. See you then!