Kotlin 259 - Cleaning
GitHub Decentralized Repo
GitHub Centralized Repo
GitHub Flat Repo
It’s past time for some cleaning, and if I’m on my game, some tests.
Timor mortis conturbat me, so I am up at oh-dark-thirty and wanting to turn my mind to programming. The cat wants to turn my mind to making breakfast for her. Soon, the cat will win, but not yet, it’s too early for her to eat.
The Zoom gang looked at the code last night, and while there were no great insights, we did do a few things. I think this one is perhaps the most significant:
data class SpaceObject(
val type: SpaceObjectType,
var x: Double,
var y: Double,
var dx: Double,
var dy: Double,
var angle: Double = 0.0,
var active: Boolean = true,
) {
var position: Vector2
get() = Vector2(x,y)
set(v:Vector2) {
x = v.x
y = v.y
}
var velocity: Vector2
get() = Vector2(dx,dy)
set(v:Vector2) {
dx = v.x
dy = v.y
}
var scale = 1.0
var components: MutableList<Component> = mutableListOf()
}
We added position and velocity, with getters and setters allowing use of Vector2 when setting the, well, position and velocity of a space object. This allowed us to change a number of x equals this, y equals that, to position = Vector2(this, that), which simplifies the reading a bit. The changes aren’t worth reviewing here: we’ll see them as we change things, I imagine.
Once all the references to x and y, dx and dy are covered by references to position and velocity, we should be able to remove x, y, dx, dy from SpaceObject, and make position and velocity into the actual member variables.
There were a few other changes. I think they were all cosmetic. We did try out a change to this code:
private fun checkAllMissiles(
firstMissile: Int,
lastMissile: Int,
firstAsteroid: Int,
lastAsteroid: Int
) {
for (missile in spaceObjects.slice(firstMissile..lastMissile)
.filter { it.active }) {
val missilePos = Vector2(missile.x, missile.y)
checkAllAsteroids(firstAsteroid, lastAsteroid, missilePos, missile)
}
}
One change we tried was this:
private fun checkAllMissiles(
firstMissile: Int,
lastMissile: Int,
firstAsteroid: Int,
lastAsteroid: Int
) {
val activeMissiles = spaceObjects.slice(firstMissile..lastMissile)
.filter { it.active }
for (missile in activeMissiles) {
val missilePos = Vector2(missile.x, missile.y)
checkAllAsteroids(firstAsteroid, lastAsteroid, missilePos, missile)
}
}
This pattern is called “Explaining Temporary Variable”, I think, and that’s what it does.
Another possibility is this one:
fun activeMissiles(spaceObjects: Array<SpaceObject>): List<SpaceObject> {
return spaceObjects.filter {it.type == SpaceObjectType.MISSILE && it.active}
}
private fun checkAllMissiles(
firstAsteroid: Int,
lastAsteroid: Int
) {
for (missile in activeMissiles(spaceObjects)) {
val missilePos = Vector2(missile.x, missile.y)
checkAllAsteroids(firstAsteroid, lastAsteroid, missilePos, missile)
}
}
That seems much preferable to the original and I prefer it over the explaining temp as well. I’ll test the game. We still have no decent tests for the collision code.
Game is good. Commit: refactor to activeMissiles(spaceObjects) function.
We can do the same thing with the asteroids, changing this:
private fun checkAllAsteroids(
firstAsteroid: Int,
lastAsteroid: Int,
missilePos: Vector2,
missile: SpaceObject
) {
for (asteroid in spaceObjects.slice(firstAsteroid..lastAsteroid).filter { it.active }) {
checkOneAsteroid(asteroid, missilePos, missile)
}
}
To this:
fun activeAsteroids(spaceObjects: Array<SpaceObject>): List<SpaceObject> {
return spaceObjects.filter {it.type == SpaceObjectType.ASTEROID && it.active}
}
private fun checkAllAsteroids(
firstAsteroid: Int,
lastAsteroid: Int,
missilePos: Vector2,
missile: SpaceObject
) {
for (asteroid in activeAsteroids(spaceObjects)) {
checkOneAsteroid(asteroid, missilePos, missile)
}
}
And then IDEA can help us change the signature:
private fun checkAllAsteroids(
missilePos: Vector2,
missile: SpaceObject
) {
for (asteroid in activeAsteroids(spaceObjects)) {
checkOneAsteroid(asteroid, missilePos, missile)
}
}
We’ll test that. Works. Commit: refactor to activeAsteroids().
Warnings remind me to remove the temps in the caller, changing this:
private fun checkAllMissiles(
firstAsteroid: Int,
lastAsteroid: Int
) {
for (missile in activeMissiles(spaceObjects)) {
val missilePos = Vector2(missile.x, missile.y)
checkAllAsteroids(missilePos, missile)
}
}
To this:
private fun checkAllMissiles() {
for (missile in activeMissiles(spaceObjects)) {
val missilePos = Vector2(missile.x, missile.y)
checkAllAsteroids(missilePos, missile)
}
}
We now have the ability to get the position directly, but since our subordinate is getting the missile, let them get the position if they want it. Change signature, in two functions, remove redundant temps and we get this:
private fun checkCollisions() {
checkAllMissiles()
}
private fun checkAllMissiles() {
for (missile in activeMissiles(spaceObjects)) {
checkAllAsteroids(missile)
}
}
private fun checkAllAsteroids(missile: SpaceObject) {
for (asteroid in activeAsteroids(spaceObjects)) {
checkOneAsteroid(asteroid, missile)
}
}
private fun checkOneAsteroid(
asteroid: SpaceObject,
missile: SpaceObject
) {
val killDist = 16.0 * asteroid.scale + 1
val dist = missile.position.distanceTo(asteroid.position)
if (dist <= killDist) {
if (asteroid.scale > 1) {
asteroid.scale /= 2
asteroid.velocity = randomVelocity()
spawnNewAsteroid(asteroid)
} else deactivate(asteroid)
deactivate(missile)
}
}
Your keen eye probably causes you to ask why we have checkCollisions just calling checkMissiles. Why do we need both? We do, because we will soon have to check the Ship and Saucer for collisions as well. So we’ll let that redundancy stand.
This is good enough to eat. Test. Game works. Commit: refactoring collision logic.
I notice something that I don’t like. Often, when an asteroid splits, both its fragments head in roughly the same direction. That often leaves them overlapping for what seems to be too long. I think that the asteroids should always separate. We could do that by rotating the velocity of the second one by a random angle, say, between 90 and 270. Let’s do that.
private fun spawnNewAsteroid(asteroid: SpaceObject) {
val newOne: SpaceObject? = spaceObjects.firstOrNull { it.type == SpaceObjectType.ASTEROID && !it.active }
if (newOne != null) {
newOne.position = asteroid.position
newOne.scale = asteroid.scale
newOne.active = true
newOne.velocity = randomVelocity()
}
}
We just need to change that last line.
private fun spawnNewAsteroid(asteroid: SpaceObject) {
val newOne: SpaceObject? = spaceObjects.firstOrNull { it.type == SpaceObjectType.ASTEROID && !it.active }
if (newOne != null) {
newOne.position = asteroid.position
newOne.scale = asteroid.scale
newOne.active = true
val angle = Random.nextDouble(90.0, 270.0)
newOne.velocity = asteroid.velocity.rotate(angle)
}
}
Try it. Playing it, I don’t quite like the look. Too often, the fragments go almost directly opposite each other. I think it would look better if the angle between them tends to be nearer to 90 degrees. Let’s try for an angle between 45 and 135 or -135 and -45.
I think that’s pretty good. Commit: angle between split asteroids’ velocity between 45 and 135 degrees.
After playing a while, I decide to put it back to the 90-270 version, based on a vague feeling that it seemed better. It’s a near thing. What do you think?
Let’s reflect.
Reflection
We’ve unquestionably (according to me) improved the collision code. We’ve removed its dependence on specific indices in the space objects, which it shouldn’t know about, and have greatly reduced the number of parameters in the calling sequence.
We have also had a possibly undesirable side effect, which is that now the collision logic is considering both saucer missiles and ship missiles, where our slicing had eliminated the two dedicated saucer missiles from consideration. They are unused now, so that’s harmless but it will mean that saucer missiles can kill asteroids, if we don’t change something. Now in the other two versions, saucer missiles do kill asteroids, but I had been thinking we might change that. We’ll worry about that if the story comes up.
Then we tweaked asteroid splitting a bit, first, to ensure that split pairs don’t go in too close to the same direction, and then to keep them from separating too widely. Then we tweaked it back. At least they don’t run on top of each other now, so that’s good.
What’s not to like? I think this code is questionable:
private fun spawnNewAsteroid(asteroid: SpaceObject) {
val newOne: SpaceObject? = spaceObjects.firstOrNull { it.type == SpaceObjectType.ASTEROID && !it.active }
if (newOne != null) {
newOne.position = asteroid.position
newOne.scale = asteroid.scale
newOne.active = true
val angle = Random.nextDouble(90.0, 270.0)
newOne.velocity = asteroid.velocity.rotate(angle)
}
}
The creation logic there is quite ad hoc and it seems to me that it’s telling us something, but I’m not clear just what is needed. Certainly we need some kind of activate scheme for asteroids, and for missiles as well, and it would be good if it could come down to some similar code that can be folded together. But for now, I don’t see it.
Overall, though, we have quite a bit of game play working, and we have far less code than in our other versions for roughly equivalent capability. I think it’s too soon to make a fair comparison, but I continue to feel that this version will be more compact and nearly as clear as the more object-oriented versions. Certainly it will be more straightforward than the decentralized version with all its smart objects interacting and somehow creating a game among themselves.
We’ll press on with features for a bit, while trying to keep the code decent. I am mot delighted with this code, but I feel that it is better than some and not far off from a decent non-OO design.
But tests! We really need some collision tests. Maybe next time. For now, I’m done. It’s 0800 hours here chez Ron.
See you next time!