First of all, something regarding usability: The player has to press space to start, space to play, but click to restart or continue. If you were using space for all of these actions the player wouldn't have to move the hand between mouse and keyboard. Also, you are telling the player to press space, but actually any key will work.
Variables
static boolean UNLOCKED = true;
static boolean LOCKED = false;
These variables are mutually exclusive. If the lock is locked, then it is not unlocked. That means that you only need one of those variables to get the same information. It is usually better to use the positive case for boolean values, which means that you should keep that variable locked, and remove the variable unlocked which is the negative case.
In Java by convention all uppercase variable names are reserved for constants, while actual variables are named in camelCase, so locked instead of LOCKED.
Furthermore, since Processing hides some of the object oriented details of Java, you do not need these variables to be static. Just remove the static keyword and it will work the same, because you are only accessing them from within the same class (which is abstracted and hidden by the Processing environment).
Hint: Reading later parts of the code, it seems like you tried to use theese variables as constants. Note that static does not make it constant, and you would need final instead. But: Don't do that. Having locked save a value (which can be changed) and true meaning locked, while false means unlocked, is way more intuitive than using a value of false to represent LOCKED and vice versa.
boolean lvlFinished = false;
boolean lvlFailed = false;
boolean start = false;
Is the level not finished if it is failed? I think what you mean by lvlFinished is levelCompleted. So a level can be either failed or completed.
Be careful with abbreviated variable names. they make the program hard to read. Better write levelFailed instead of lvlFailed.
start is a bad name of a variable. Variables contain the state of the program. start sounds either like a verb or a noun, so something you do (functions or methods) or something that exists as an object (represented by a class). started is a better name to represent state, and it is coherent with failed and completed.
int lvl = 1;
int nTargets = 1;
This should be level and amountOfTargets.
color bg = color(90, 175, 255); // Normal BG color
color cFail = color(255, 80, 80); // Color on fail
color fontColor = color(220); // Color of text
color cTarget = color(255, 255, 70); // Color of target ball
color cPaddle = color(255, 0, 20); // Color of paddle
This is a perfect example why good variable names are important. You have variable names that are hardly readable, and comments which describe what the variables mean, but which will not help you if you read the variable names somewhere else in the code without the comments. Variables should always explain themselves.
Better variable names, which would make the comments obsolete, would be:
color backgroundColor = color(90, 175, 255);
color failColor = color(255, 80, 80);
color textColor = color(220);
color targetColor = color(255, 255, 70);
color paddleColor = color(255, 0, 20);
// theta for target + paddle
float tTarget, tPaddle;
// paddle velocity
float vPaddle;
I had to google "theta" to understand what you mean here. If you want it to be the angle, don't give the angle a name (alpha, beta and so on would also be very frequently used). Just call it angle, or orientation.
Also, declare each variable on its own line, to make it look clearer and more readable.
float targetAngle;
float paddleAngle;
float paddleVelocity;
Hint: Seeing later in the code that the angle is in radians, and one might expect an angle to be in degrees by default, a better name would be targetAngleRadians or targetRadians, which I am going to use later on. Also, since paddleVelocity means the rotational speed, I will later use paddleRotationSpeed instead.
PFont lblFont;
PFont lvlFont;
PFont textFont;
Another example why abbreviations can be hurtful. It is easy to misread lblFont and lvlFont. Better name them labelFont and levelFont.
void setup() {
size(250, 445);
...
}
It would be better to define constants for width and height (using the final keyword) and then use them like size(WIDTH, HEIGHT);. This way you can easily change the values at the start of the code without having to search for the line where they are used, and when you see the names in the code you immediately know what they mean, which might not be the case with so called "magic numbers", which are arbitrarily chosen numbers that occur in the code.
The indentation, not just in this function but in all code blocks, is weird, because usually you indent Java code with 4 spaces. When I tried your code with processing (version 3.3.6) it seemed like it automatically indents 2 spaces. I'm not sure if it allows for changing that in the settings.
Drawing Functions
Let's look at the drawing functions next, because the next function chronologically would have been draw(), and that way we can work through the drawing code from a bottom-up perspective.
void drawLock(boolean state, color background) {
background(background);
int dY = state ? 60 : 0;
image(loop, width/2, 160 - dY);
fill(0);
ellipse(width/2, 270, 180, 180);
fill(background);
ellipse(width/2, 270, 118, 118);
}
What is state? What state can a lock have? The one state that is meant here is whether it is locked or not, so just call the parameter locked instead.
Instead of defining that variable dY, whose name does not really explain what it is for (probably something like "delta", so might as well write deltaY), and than only using it to subtract it from another value, just assign the result of that subtraction:
image(loop, width / 2, locked ? 160 : 100);
Note that I had to invert the logic, because it seems like you are using a value of false in the (intended to be constant) variable LOCKED, which is counterintutitive.
Try to be more consistent with spaces around operators. You write 160 - dY, but width/2. Always add spaces around operators, i. e. width / 2.
void drawTarget() {
if (tTarget > PI) tTarget -= TWO_PI;
else if (tTarget < -PI) tTarget += TWO_PI;
float x = width/2 + 75 * cos(tTarget);
float y = 270 + 75 * sin(tTarget);
fill(255, 255, 20);
ellipse(x, y, 20, 20);
}
The if-else looks very pressed together, which makes it hard to read. Always put the code that is executed under a condition on a different line than the condition. Furthermore, adding braces prevents you from introducing bugs when you add a line without realising the missing braces.
if (targetRadians > PI) {
targetRadians -= TWO_PI;
} else if (targetRadians < -PI) {
targetRadians += TWO_PI;
}
Looking at what you are doing with it later, you probably do not even need this if-else code, since you are only using the value as arguments for the cos() and sin() functions. These functions should be able to work with values that are positive or negative multiples of Pi.
The next two lines, where x and y are defined, contain multple "magic numbers", which makes it difficult to guess why the values are calculated like that. Using proper constant names for these values and adding a comment would help. At least that calculation is properly abstracted in the drawTarget() function, rather than written in draw().
The drawPaddle() function is similar to the drawTarget() function, so the same things apply.
void renderGame(boolean finished, boolean failed) {
if (failed)
drawLock(LOCKED, cFail);
else if (finished)
drawLock(UNLOCKED, bg);
else
drawLock(LOCKED, bg);
drawTarget();
drawPaddle();
drawText();
}
renderGame() is actually what the draw() function is for. So it is fine to put the code for rendering different parts ("objects") in different functions and call them here, but this code here actually belongs into draw(). So I will next review the draw() function with this code inserted at the start, replacing the parameters with the injected variables. Also adding some blanklines for better readability, replacing the variables names with the suggested ones from above, and fixing indendation and braces. Since LOCKED and UNLOCKED were meant as constants, I will replace their usage with the boolean literals true and false as they are expected by the reviewed version of drawLock(), i. e. the value true means it is locked.
void draw() {
if (levelFailed) {
drawLock(true, failColor);
} else if (levelCompleted) {
drawLock(false, backgroundColor);
} else {
drawLock(true, backgroundColor);
}
drawTarget();
drawPaddle();
drawText();
// Combining the conditions reduces nesting.
if (started && !lvlFinished && !lvlFailed) {
paddleRadians += paddleRotationSpeed;
// This could be in a function, returning levelFailed
if (paddleRotationSpeed > 0) {
levelFailed = angleDelta(paddleRadians, targetRadians) > 0.2;
} else {
levelFailed = angleDelta(paddleRadians, targetRadians) < -0.2;
}
}
textFont(textFont);
if (levelFinished) {
text("Click anywhere to continue", width / 2, 30);
} else if (lvlFailed) {
text("Click anywhere to try again", width / 2, 50);
} else if (!started) {
text("Press [SPACE] to begin", width / 2, 50);
}
}
You are telling the player to press space, but actually any key will work.
Input Handling Code
void mousePressed() {
if (lvlFinished ^ lvlFailed) {
if (lvlFinished) {
lvlFinished = false;
++ lvl;
} else {
lvlFailed = false;
}
nTargets = lvl;
setAngles();
renderGame(lvlFinished, lvlFailed);
start = false;
}
}
This
if (lvlFinished ^ lvlFailed) {
if (lvlFinished) {
is redundant. Using the ^ operator (which is an XOR, exclusive or) already assumes that the boolean variables are mutually exclusive, which they are in your program, and also in the real world, because you can't logically fail a level and succeed ("finish" aka "complete") at the same time. If lvlFinished is false but lvlFailed is true, the first condition will be true, causing the second condition to be evaluated again, even though it is already known to be false. Just leave out the outer condition; the whole check is not needed. Just check if (lvlFinished) { ... } and replace the else with if (lvlFailed) { ... }.
You are increasing the level if the level is completed, but you are not resetting it if the level is failed. That might be by design, but I usually expect a game to start from the beginning if it is lost.
I think calling renderGame() here should not be needed, since I guess that draw() calls it every frame anyway, and it is better not to mix input code or logic with rendering code.
So my improved version looks like this, putting some of the code in an extra function:
void mousePressed() {
if (levelCompleted) {
levelCompleted = false;
level++;
resetCurrentLevel();
} else if (levelFailed) {
levelFailed = false;
resetCurrentLevel();
}
}
void resetCurrentLevel() {
amountOfTargets = level;
setAngles();
started = false;
}
