Bedazzling My Bejeweled Animations

Question

I've spent a few days working on the animations for my Bejeweled clone and I am pretty happy with the results so I am posting the code for review. This is the most advanced animation code that I have written so far, so there are probably problems with it. Here is a gif of the animation running :

I encountered two major problems when creating this code. The first one was that the update method continually called the animation code for each animation state while it was playing. This could cause weird problems and graphical glitches. I solved this by having a variable keep track of whether an animation was currently playing, and if so it would not run it again. The downside to this is needing to manually set that flag to NO at the end of each animation. The second major problem was that in order to get the animation state to properly advance, I had to place each piece of animation code inside of an SKAction block. I am not sure if this is the proper approach, but it seems to be a good solution.

I will not post the entire scene since it is a lot of code.

First off, I need these instance variables to keep track of certain information at the object level:

//animations
DMGameSceneState _state;
BOOL _isAnimationPlaying;
BZDirection _lastDirection;

Next, here are the conditionals inside of the update method of the Scene:

if (_state == DMGameSceneStatePoppingMatches ||
    _state == DMGameSceneStateReplacingMatches ||
    _state == DMGameSceneStateSwapFailure ||
    _state == DMGameSceneStateSwapSuccess) {
    if (!_isAnimationPlaying) {
        [self processGameAnimationState];
        _isAnimationPlaying = YES;
    }
}

if (_state == DMGameSceneStateRunning || _state == DMGameSceneStateBuildingBoard) {
    [self renderBoardOrbs];
}

Here is the switch statement that controls which animation will play. Initially all of the animation code was contained in this method, but I have since extracted some of it to smaller methods called from inside here:

#pragma mark - Orb Animations
-(void) processGameAnimationState {
    switch (_state) {
        case DMGameSceneStateSwapFailure:
            [self playAnimationSwapFailure];
            break;
        case DMGameSceneStateSwapSuccess:
            [self playAnimationSwapSuccess];
            break;
        case DMGameSceneStatePoppingMatches:
            [self playAnimationPoppingMatches];
            break;
        case DMGameSceneStateReplacingMatches:
            [self playAnimationReplacingMatches];
            break;
        default:
            break;
    }
}

And finally, the animation methods. This is the real meat of the code, and is probably where any major flaws will be found:

-(void) playAnimationSwapFailure {
    [self runAction:[SKAction runBlock:^(void) {

        DMOrb *firstOrb = _game.firstOrb;
        DMOrb *secondOrb = _game.secondOrb;
        CGPoint firstOrbOrigin = CGPointMake(firstOrb.boardPosition.x * (_orbWidth + _orbSpacing),
                                             firstOrb.boardPosition.y * (_orbWidth + _orbSpacing));
        CGPoint secondOrbOrigin = CGPointMake(secondOrb.boardPosition.x * (_orbWidth + _orbSpacing),
                                              secondOrb.boardPosition.y * (_orbWidth + _orbSpacing));

        for (SKSpriteNode *node in _orbLayer.children) {
            NSString *firstOrbName = NSStringFromCGPoint(firstOrb.boardPosition);
            if ([firstOrbName isEqualToString:node.name]) {
                [node runAction:[SKAction moveTo:secondOrbOrigin duration:0.1] completion:^(void) {
                    [node runAction:[SKAction moveTo:firstOrbOrigin duration:0.1]];
                }];
                firstOrb = nil;
            }
            NSString *secondOrbName = NSStringFromCGPoint(secondOrb.boardPosition);
            if ([secondOrbName isEqualToString:node.name]) {
                [node runAction:[SKAction moveTo:firstOrbOrigin duration:0.1] completion:^(void) {
                    [node runAction:[SKAction moveTo:secondOrbOrigin duration:0.1]];
                }];
                secondOrb = nil;
            }
        }
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.2] completion:^(void) {
            _state = DMGameSceneStateRunning;
            _isAnimationPlaying = NO;
        }];
    }];
}
-(void) playAnimationSwapSuccess {
    [self runAction:[SKAction runBlock:^(void) {

        //save local variables because they are used in the blocks
        DMOrb *firstOrb = _game.firstOrb;
        DMOrb *secondOrb = _game.secondOrb;
        CGPoint firstOrbOrigin = CGPointMake(firstOrb.boardPosition.x * (_orbWidth + _orbSpacing),
                                             firstOrb.boardPosition.y * (_orbWidth + _orbSpacing));
        CGPoint secondOrbOrigin = CGPointMake(secondOrb.boardPosition.x * (_orbWidth + _orbSpacing),
                                              secondOrb.boardPosition.y * (_orbWidth + _orbSpacing));

        //swap the orbs and then replace them with their counterpart once the swap is complete
        for (SKSpriteNode *node in _orbLayer.children) {
            NSString *firstOrbName = NSStringFromCGPoint(firstOrb.boardPosition);
            if ([firstOrbName isEqualToString:node.name]) {
                [node runAction:[SKAction moveTo:secondOrbOrigin duration:0.1] completion:^(void) {
                    [node removeFromParent];
                    DMOrbNode *newNode = [[DMOrbNode alloc]initWithColor:[self colorForType:firstOrb.type]
                                                                    size:CGSizeMake(_orbWidth, _orbWidth)
                                                                     orb:secondOrb];
                    newNode.position = secondOrbOrigin;
                    [_orbLayer addChild:newNode];
                }];
            }
            NSString *secondOrbName = NSStringFromCGPoint(secondOrb.boardPosition);
            if ([secondOrbName isEqualToString:node.name]) {
                [node runAction:[SKAction moveTo:firstOrbOrigin duration:0.1] completion:^(void) {
                    [node removeFromParent];
                    DMOrbNode *newNode = [[DMOrbNode alloc]initWithColor:[self colorForType:secondOrb.type]
                                                                    size:CGSizeMake(_orbWidth, _orbWidth)
                                                                     orb:firstOrb];
                    newNode.position = firstOrbOrigin;
                    [_orbLayer addChild:newNode];
                }];
            }
        }
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.2] completion:^(void) {
            _state = DMGameSceneStatePoppingMatches;
            _isAnimationPlaying = NO;
        }];
    }];
}
-(void) playAnimationPoppingMatches {
    [self runAction:[SKAction runBlock:^(void) {
        for (SKSpriteNode *node in _orbLayer.children) {
            for (DMOrb *orb in _game.matchedOrbs) {
                NSString *orbName = NSStringFromCGPoint(orb.boardPosition);
                if ([orbName isEqualToString:node.name]) {
                    [node runAction:[SKAction fadeOutWithDuration:0.2]];
                    break;
                }
            }
        }
        [_game clearMatchedOrbs];
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.2] completion:^(void) {
            _state = DMGameSceneStateReplacingMatches;
            _isAnimationPlaying = NO;
        }];
    }];
}
-(void) playAnimationReplacingMatches {
    [self runAction:[SKAction runBlock:^(void) {
        for (SKSpriteNode *node in _orbLayer.children) {
            for (DMOrb *orb in _game.movingOrbs) {
                NSString *orbName = NSStringFromCGPoint(orb.previousPosition);
                if ([orbName isEqualToString:node.name]) {
                    CGPoint destination = CGPointMake(orb.boardPosition.x * (_orbWidth + _orbSpacing),
                                                      orb.boardPosition.y * (_orbWidth + _orbSpacing));
                    CGFloat speed;
                    if (destination.x > orb.previousPosition.x ||
                        destination.x < orb.previousPosition.x) {
                        speed = 0.002 * abs(destination.x - orb.previousPosition.x);
                    } else if (destination.y > orb.previousPosition.y ||
                               destination.y < orb.previousPosition.y) {
                        speed = 0.002 * abs(destination.y - orb.previousPosition.y);
                    }
                    [node runAction:[SKAction moveTo:destination duration:speed]];
                    break;
                }
            }
        }
        [_game clearMovingOrbs];
        for (DMOrb *orb in _game.createdOrbs) {
            DMOrbNode *orbNode = [[DMOrbNode alloc]initWithColor:[self colorForType:orb.type]
                                                            size:CGSizeMake(_orbWidth, _orbWidth) orb:orb];
            [_orbLayer addChild:orbNode];
            orbNode.position = [self outsidePositionForDirection:_lastDirection orb:orb];
            CGPoint destination = CGPointMake(orb.boardPosition.x * (_orbWidth + _orbSpacing),
                                              orb.boardPosition.y * (_orbWidth + _orbSpacing));
            [orbNode runAction:[SKAction moveTo:destination duration:0.4]];
        }
        [_game clearCreatedOrbs];
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.4] completion:^(void) {
            _state = DMGameSceneStateRunning;
            _isAnimationPlaying = NO;
        }];
    }];
}

Also, here is the method that calculates the proper position when adding newly created orbs to the scene:

-(CGPoint) outsidePositionForDirection:(BZDirection)direction orb:(DMOrb *)orb {
    switch (direction) {
        case BZDirectionLeft:
            return CGPointMake(9 * (_orbWidth + _orbSpacing), orb.boardPosition.y * (_orbWidth + _orbSpacing));
        case BZDirectionRight:
            return CGPointMake(-1 * (_orbWidth + _orbSpacing), orb.boardPosition.y * (_orbWidth + _orbSpacing));
        case BZDirectionUp:
            return CGPointMake(orb.boardPosition.x * (_orbWidth + _orbSpacing), -1 * (_orbWidth + _orbSpacing));
        case BZDirectionDown:
            return CGPointMake(orb.boardPosition.x * (_orbWidth + _orbSpacing), 9 * (_orbWidth + _orbSpacing));
        default:
            return CGPointZero;
    }
}

Any and all feedback is much appreciated!

Answer 1

_state == DMGameSceneStatePoppingMatches ||
_state == DMGameSceneStateReplacingMatches ||
_state == DMGameSceneStateSwapFailure ||
_state == DMGameSceneStateSwapSuccess

Is _state a bit mask? Is DMGameSceneState an NSOptions?

If so, I'd add to the NSOptions declaration:

DMGameSceneStateShouldUpdate = DMGameSceneStatePoppingMatches |
                               DMGameSceneStateReplacingMatches |
                               DMGameSceneStateSwapFailure |
                               DMGameSceneStateSwapSuccess

And the same can be done for

DMGameSceneStateRunning | DMGameSceneStateBuildingBoard

and these large ||-ed together conditions can simply turn into:

if (_state & DMGameSceneStateShouldUpdate)

---

outsidePositionForDirection:orb: has some major repetition. (_orbWidth + _orbSpacing) is repeated 8 times. Let's stick that in a variable.

int orbConst = (_orbWidth + _orbSpacing)

And now we can just use that in those 8 spots. And then get rid of your magic -1 and magic 9, and use variables to tell me what those mean.

---

The biggest thing that bothers me about all of your animation code is how ugly all of this nesting looks.

So, I grabbed the shortest method, and I'm going to use that as an example of how to clean this up.

-(void) playAnimationPoppingMatches {
    [self runAction:[SKAction runBlock:^(void) {
        for (SKSpriteNode *node in _orbLayer.children) {
            for (DMOrb *orb in _game.matchedOrbs) {
                NSString *orbName = NSStringFromCGPoint(orb.boardPosition);
                if ([orbName isEqualToString:node.name]) {
                    [node runAction:[SKAction fadeOutWithDuration:0.2]];
                    break;
                }
            }
        }
        [_game clearMatchedOrbs];
    }] completion:^(void) {
        [self runAction:[SKAction waitForDuration:0.2] completion:^(void) {
            _state = DMGameSceneStateReplacingMatches;
            _isAnimationPlaying = NO;
        }];
    }];
}

First, let's handle the completion block. The completion block itself contains a completion block.

I've not completely wrapped my head around all that your code does, so these are placeholder names and you should find better ones...

First, since we'll be using a void-void block several times and the block syntax is quite ugly, let's typedef a void-void block.

typedef void (^CompletionBlock)(void);

Now, let's start making some block variables!

CompletionBlock block_c = ^(void) {
    _state = DMGameSceneStateReplacingMatches;
    _isAnimationPlaying = NO;
};

CompletionBlock block_b = ^(void) {
    [self runAction:[SKAction waitForDuration:0.2] completion:block_c];
};

CompletionBlock block_a = ^(void) {
    for (SKSpriteNode *node in _orbLayer.children) {
        for (DMOrb *orb in _game.matchedOrbs) {
            NSString *orbName = NSStringFromCGPoint(orb.boardPosition);
            if ([orbName isEqualToString:node.name]) {
                [node runAction:[SKAction fadeOutWithDuration:0.2]];
                break;
            }
        }
    }
    [_game clearMatchedOrbs];
};

Now let's make the only method call the method actually makes:

[self runAction:[SKAction runBlock:block_a] completion:block_b];

Now our code is broken up into distinct chunks that we can more easily digest. Our blocks can more easily be read as they more appropriately should: as discrete chunks of code.