6
\$\begingroup\$

This code finds the next obstacle in the player's path, then moves it. I am trying to make it more efficient, because this code runs for each frame, and it lags a bit. It does work though.

There are 8 blockers, which are basically obstacles. We subtract from the players rotation until the rotation is equal to one of these eight blockers. Then we move the blocker. Each blocker cannot be moved two times in a row, though.

You might be wondering why rotation determines if the player collides. That is because my game is based around a planet, to where the player rotates.

whileLoop:
    while (true) {

        for (Blocker blocker : blockers) {
            if (playerRotation == (int) blocker.getRotation()) {
                if(lastBlockerMoved != blocker) {
                    lastBlockerMoved = blocker;
                    float gap = Tools.generateFloat(Config.minBlockerGap, Config.maxBlockerGap);
                    float y = Tools.generateFloat(Config.minBlockerY, Config.maxBlockerY - gap);
                    float duration = Tools.generateFloat(Config.minBlockerYDuration, Config.maxBlockerYDuration);
                    blocker.addAction(Actions.moveTo(blocker.getX(), y, duration, Config.blockerInterpolation));
                    blocker.addAction(GapSizeAction.getGapSizeAction(gap, duration));
                }

                break whileLoop;
            }
        }

        playerRotation--; //-- is going right, ++ would be going left
        while (playerRotation > 360) {
            playerRotation -= 360;
        }
        while (playerRotation <= 0) {
            playerRotation += 360;
        }
    }
\$\endgroup\$
7
\$\begingroup\$

Take out the variables that are not changing within the loops and initialise them once.

float gap = Tools.generateFloat(Config.minBlockerGap, Config.maxBlockerGap);
float y = Tools.generateFloat(Config.minBlockerY, Config.maxBlockerY - gap);
float duration = Tools.generateFloat(Config.minBlockerYDuration, Config.maxBlockerYDuration); 

It is unnecessary to continue to break out of the for loop to change the playerRotation. so then add within your for loop the playerRotation decrement with two if conditions (the while is unnecessary, it can only change by 1, so cannot exceed greater than 360 or less than 0.

while (true) {

    for (Blocker blocker : blockers) {
        if (playerRotation == (int) blocker.getRotation()) {            
            if(lastBlockerMoved != blocker) {
                lastBlockerMoved = blocker;
                blocker.addAction(Actions.moveTo(blocker.getX(), y, duration, Config.blockerInterpolation));
                blocker.addAction(GapSizeAction.getGapSizeAction(gap, duration));

                playerRotation--; //-- is going right, ++ would be going left
                if (playerRotation > 360) 
                    playerRotation -= 360;

                if (playerRotation <= 0) 
                    playerRotation += 360;

            }
        }
    }
    // If game over -> break;
}

I am not sure of what condition you need to end this game, but you will need to add this within your while loop to break out.

\$\endgroup\$
3
\$\begingroup\$

While not a huge performance hurdle like everything that Yvette pointed out, there is a far simpler way to handle the end of your method.

You currently have:

while (playerRotation > 360) {
    playerRotation -= 360;
}
while (playerRotation <= 0) {
    playerRotation += 360;
}

You can eliminate both of those loops using the Modulus Operator and some conditional checking:

if (playerRotation >= 360) {
    playerRotation = playerRotation % 360;
} 
else if (playerRotation < 0) {
    playerRotation = (playerRotation % 360) + 360;
}
// If your variable is between 0 and 360, you don't need to do anything.

That divides your playerRotation variable by 360, and assigns the remainder back to your variable.

\$\endgroup\$
  • 2
    \$\begingroup\$ Or just playerRotation %= 360; if (playerRotation < 0) {playerRotation += 360;} might even be tighter \$\endgroup\$ – Sabre Sep 7 '16 at 20:21
1
\$\begingroup\$

You should profile the game first.

While your code is in worst case doing cca. 360*8 (360 player rotation decrement x 8 blockers) loops until it finds the blocker (and it will end in infinite cycle in case you don't have any blocker in blockers) => so it can be written much more efficiently...

Still about ~3k of int compares should not lag modern phone, that's not much.

So you are very likely having bottleneck in some code which is not part of your question.

Anyway, to rewrite your current code you may try something like this (pseudo code):

// search for the Blocker which is "next in path"
// (but didn't move previously)
blockToMove = null;
blockToMoveSteps = 360;
for (block : blockers) {
    if (block == lastBlockerMoved) continue;
    int stepsToAlign = playerRotation - block.getRotation();
    // for ++ direction use: block.getRotation() - playerRotation
    // make stepsToAlign to be 0..359
    while (stepsToAlign < 0) stepsToAlign += 360;
    while (360 <= stepsToAlign) stepsToAlign -= 360;
    // compare with the currently found one
    if (blockToMoveSteps < stepsToAlign) continue;
    // new block to move found (needs less steps to align)
    blockToMoveSteps = stepsToAlign;
    blockToMove = block;
}
if (null != blockToMove) {
    // did found some block to move
    lastBlockerMoved = blockToMove;
    set gap/y/duration/addAction/...
}

This will iterate over each blocker only once - searching for the one, which needs least amount of player rotations (steps) to align with.

Those two while to convert value into 0..359 range are effective only when all source rotation values are near this range, otherwise rewrite it with modulus. If you will be profiling it, you may try modulus anyway, it will be probably faster in Java that way anyway.


edit: my proposal has somewhat different logic, searching for another blocker, when the nearest one did move, while your original code (reading it now again) is probably moving the nearest one once, and then just finding it again and again, but not moving it.

If that's the case, then remove the test if (block == lastBlockerMoved) continue; from search loop, and add condition && blockToMove != lastBlockerMoved to the second part if.


One more thing, I forgot to mention one other idea. If you can sort the blockers by the position (rotation), then you can use the last position (last index to moved blocker) to check, whether that blocker is still current for next frame, if not, move to next. Removing the loop cycle completely and having just couple of basic ifs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.