Vertical-scrolling 2D platform game

Question

I am working on a vertical-scrolling 2D platform game using GameplayKit, SpriteKit and I am writing it in Swift 2.0.

Today I have been working on a tiling-routine which repositions SKNode right after they have left the upper or lower screen-border. I am not spawning new SKNodes because when the game becomes very fast it is more performant to simply reposition and reuse SKNodes than to destroy and create new ones.

The whole code is encapsulated into a GKComponent and is being applied to a layer entity. You can assign different Tiles where the tile system just has one single column but multiple rows. Tiles can be of different size and so on.

Here's the Code:

override func updateWithDeltaTime(seconds: NSTimeInterval) {
 super.updateWithDeltaTime(seconds)
 for child: SKSpriteNode in self.tileNodes {
 let absolutePosition = RPGameScene.sharedGameScene.convertPoint(child.position, fromNode: child.parent!)
 /* Camera is moving up */
 if cameraComponent.cameraState == RPCameraState.MovingNorth.rawValue {
 /* See if cameraState has just switched */
 if previousCameraState != cameraComponent.cameraState {
 /* In case it has we have to up-invert tileIndex */
 /* We are doing this by enumerating over tileNodes
 count +1 (+1 for new placement) and adding one 
 count per time */
 /* Normally it would be: self.tileNodes.count + 1 but
 since we start by 0 it goes automatically up to
 count + 1 */
 for _ in 0...self.tileNodes.count { tileIndex += 1 }
 previousCameraState = cameraComponent.cameraState
 }
 if absolutePosition.y + child.texture!.size().height <= 0 {
 /* We have to add old textures size to the
 lowerPositionOffset since we are moving this node to
 the upperPositionOffset */
 lowerPositionOffset += child.texture!.size().height
 /* We have to work with a reference to new texture
 since even after changing texture the current child
 still has its old size! */
 let newTexture = self.tileSet.tiles[tileIndex]
 child.runAction(SKAction.setTexture(newTexture, resize: true))
 child.position.y = upperPositionOffset
 /* Add textures size to the position offset */
 upperPositionOffset += newTexture.size().height
 tileIndex += 1
 }
 }
 /* Camera is moving down */
 else {
 if absolutePosition.y >= RPGameSceneSettings.height {
 /* See if cameraState has just switched */
 if previousCameraState != cameraComponent.cameraState {
 /* In case it has we have to down-invert tileIndex */
 /* We are doing this by enumerating over tileNodes
 count +1 (for new placement) and subtracting
 one count per time */
 /* Normally it would be: self.tileNodes.count + 1
 but since we start by 0 it goes automatically up
 to count + 1 */
 for _ in 0...self.tileNodes.count { tileIndex -= 1 }
 previousCameraState = cameraComponent.cameraState
 }
 /* We have to substract old texture size from
 upperPositionOffset since we are moving this node to
 the lowerPositionOffset */
 upperPositionOffset -= child.texture!.size().height
 /* We have to work with a reference to new texture
 since even after changing texture the current child
 still has its old size! */
 let newTexture = self.tileSet.tiles[tileIndex]
 /* When moving down we have to subtract newTextures
 height before replacing it 
 due to Y-AnchorPoint = 0 */
 lowerPositionOffset -= newTexture.size().height
 child.runAction(SKAction.setTexture(newTexture, 
 resize: true))
 child.position.y = lowerPositionOffset
 tileIndex -= 1
 }
 }
 }
}

There is nothing wrong with the code - It's working like I wanted it to. But there is one thing I would really like to know:

Since coming from Objective-C and just making my first steps with Swift I am pretty sure that there are ways to optimize this code - especially when using a modern language like Swift. But the problem is, I am not really that deep into the Swift language features, especially the new ones.

So how can I optimize the code above using pure Swift language features?

EDIT:

There is quite a lot of repetition in that code - I could imagine using a nested function to combine this all together.

EDIT:

Below you'll find the parts missing in the code sample above for a better understanding:

unowned let renderComponent: RPRenderComponent
let tileSet: RPTileSet
var tileNodes: [SKSpriteNode]
var cameraComponent: RPCameraComponent!
var previousCameraState = RPCameraState.Resting.rawValue
private var tileIndex_: Int = 0
var tileIndex: Int {
 get { return tileIndex_ }
 set(newTileIndex) {
 tileIndex_ = newTileIndex
 if tileIndex_ >= self.tileSet.tiles.count {
 tileIndex_ = 0
 }
 else if tileIndex_ < 0 {
 tileIndex_ = self.tileSet.tiles.count - 1
 }
 }
}
private var upperPositionOffset: CGFloat = 0
private var lowerPositionOffset: CGFloat = 0 

tileSet is of type RPTileSet which is:

struct RPTileSet {
 let tileSize: CGSize
 let tiles: [SKTexture]
 let startTile: SKTexture?
}

• Phrancis: Welcome to Code Review! I hope you get some good answers.
• Simon Kemper: Thank you very much @Phrancis ! I am looking forward to receive answers and help others!
• nhgrif: What is the type of self.tileNodes?
• Simon Kemper: @nhgrif See my latest edit above! Thx.

Answer 1

All right, so I know this code was primarily added for some context, but I still want to mention a few things here anyway...

var cameraComponent: RPCameraComponent!

You should decide whether this makes sense as a non-optional or a full optional and go with that. Implicitly unwrapped optionals are dangerous and should be avoided. Apple advertised implicitly unwrapped optionals as something to use when you're certain by the first time you use it, it will have been initialized, but to me, implicitly unwrapped optionals are something to use when you're not sure whether your variable should be optional or non-optional. A little bit of unwrapping code (enforced by replacing your ! with a ?) will save some crashes.

var previousCameraState = RPCameraState.Resting.rawValue

Why don't you just let previousCameraState represent the enum value? Why are you using the rawValue? You took the time to define an enum, you might as well use it properly.

private var tileIndex_: Int = 0

As I comment on the next piece of code, you'll find that this instance variable is likely unnecessary (certainly without an underscore). But if you are going to use a private instance variable with a computed property combination, we should stick with the Objective-C standard of prefixing the underscore rather than postfixing. That'd make this variable _tileIndex.

var tileIndex: Int {
 get { return tileIndex_ }
 set(newTileIndex) {
 tileIndex_ = newTileIndex
 if tileIndex_ >= self.tileSet.tiles.count {
 tileIndex_ = 0
 }
 else if tileIndex_ < 0 {
 tileIndex_ = self.tileSet.tiles.count - 1
 }
 }
}

As I can see, there are a few problems here. It looks like we're trying to make tileIndex be an index that wraps around the tileSet.tiles array, right?

So, first of all, your set appears to assume that tileIndex will always be only incremented or decremented by one. If you want that, then I recommend you replace your stored property and this computed property with the following...

private(set) var tileIndex = 0
func incrementIndex() {
 tileIndex += 1
 if tileIndex >= tileSet.tiles.count {
 tileIndex = 0
 }
}
func decrementIndex() {
 tileIndex -= 1
 if tileIndex <= 0 {
 tileIndex = tileSet.tiles.count - 1
 }
}

But, if we want the user to be able to freely set tileIndex to any value, we still don't need the stored/computed pairing. We can just use a didSet.

var tileIndex = 0 {
 didSet {
 guard !(0..<tileSet.tiles.count).contains(tileIndex) else {
 return
 }
 tileIndex = ((tileIndex % tileSet.tile.count) + tileSet.tiles.count) % tileSet.tiles.count
 }
}

---

Now, with all of that out of the way, let's start with some nitpicking on your code...

---

for child: SKSpriteNode in self.tileNodes {

A few problems I have here.

First, it is generally preferred that we omit self in Swift except where necessary.

Second, because tileNodes is defined as having type [SKSpriteNode], we really don't need to specify it.

Third, child isn't a very descriptive name of the loop variable. We typically expect that the array will have a plural name and members of that array of the singular version of that name.

So combining these three points, our loop should look something like this:

for tileNode in tileNodes {

---

The entire method is contained within the loop body and it's doing way too much. How could you possibly hope to test any of this? And how long does someone have to sit mentally parsing this code before they have a very clear picture of what the entire method is doing (which is kind of necessary before you go about changing code within a method)?

• Simon Kemper: Ok. There are a few things that I actually didn't know. Thank you for these. But the method inside the loop body isn't doing too much at all. This component is added to more than 10 entities and there are loop methods which are doing even more but still the impact on CPU / GPU is low. No lag in frame rate / etc. But there is no way to leave out things I am doing in the loop method ... So I am having no problem with it and I clearly understand it the first time I take a look at it!
• nhgrif: The point isn't to leave stuff out. Instead, refactor those things into another function which you call from within the loop. You should be writing small functions. For me, 20 lines is way to long, and your loop body is way longer than that.
• Simon Kemper: Ah ok! I'll give it a try! Sorry that got lost in translation. English is not my mothers language .. :D
• Simon Kemper: I am often working with nested functions when possible. But I think I can now better understand why there are such things like "first class" functions and/or closures. This helps us / or gives us better options to split code into smaller pieces which are easier to read, right?