Blade Runner

Hello everybody! Finally took the time to finish my little game. Many thanks to @drummyfish @ESPboy @Pharap for helping with the damned bridge!
A little about the game:
Your task is to move from one platform to another through the constructed blade-bridge. If you get to the exact center of the bridge, you earn one coin. Perfect hit 5 times in a row gives +5 coins. At the hard level, these indicators double. Well, also you will find a small bonus in the store :slight_smile:
Have fun!
BladeRunner.hex (60.4 KB)

I would also like to ask professionals to evaluate my code, but it is so convoluted that even I get lost in it :slight_smile: Anyway, all comments and advice are welcome.

Download here: GitHub - NoobGeek-Ilya/BladeRunner

7 Likes

Congratulation, very nice game :slight_smile: Thanks also for sharing it under a free license. It’s pretty cool you included the users of this forums as the players! Is this your first game?

EDIT:

Code reviews are @Pharap’s speciality here.

If you want my comments, there are things to improve, but you’re on the right track. Don’t take it as bashing your code, just a constructive criticism. Remember, you’re already on a programming level most people will never reach.

E.g. chosenPlayer doesn’t have to be an array at all, it can simply be a variable that holds the number of the selected player. The player bool variables (PharapCH, drummyfishCH, …) can all be deleted – it seems like you’re thinking that you are storing the bool variables in arrays (buyPlayer, costPlayer, …) but you’re not, you only initialize the arrays with their initial value (0). I am actually kinda confused about all these arrays. If you share what you intended to do, we may tell you how it could be done.

Formatting could be better – yeah, it’s extra work but as code gets longer it becomes more and more important. E.g. the super long line at the very beginning doesn’t help readability, unless there are like 3 variables it’s best to put each variable on a separate line. Also try to stay consistent with bracket formatting in control structure (if, for, …), it greatly helps to see the code structure. For example I always use

if (something)
{
  doSomething
}
else
{
  doSomethingElse
}

Naming functions and variables should also be consistent: functions should ideally be named by actions, e.g. handleMenu() instead of just menu(). Probably most common convention is to use lowercase-starting camelCase (like myAwesomeVariable) – your functions sometimes start with lowercase, sometimes uppercase (uppercase is used for type and class names).

I am not sure what this function does but this is where arrays could actually help make it much much shorter. Example:

instead of

const char *name0 = "John";
const char *name1 = "Jack";
const char *name2 = "Mary";

void printName(int number)
{
  if (number == 0)
    print(name0);
  else if (number == 1)
    print(name1);
  else if (number == 2)
    print(name2);
}

do

const char *names[] = {"John", "Jack", "Mary"};

void printName(int number)
{  
  if (number >= 0 && number < 3) // for safety
    print(names[number]);
}

These are just some things to possibly improve.

2 Likes

Interesting game! :smile:

Some suggestions:

As we discussed in your Variable for array topic, your sprites should have a height that’s a multiple of 8. If you round up all heights to the nearest multiple of 8, you will be properly following the API. It shouldn’t affect the operation of the game in any way.


You would be better to use the Arduboy2 audio class to control sound muting. To do this:

Get rid of the soundEnable variable.

For the code to turn sound on and off change:

  if (arduboy.justPressed(B_BUTTON) && markerPos == 3) soundEnable = 1;
  if (arduboy.justPressed(A_BUTTON) && markerPos == 3) soundEnable = 0;

  if(soundEnable == 0) Sprites::drawOverwrite (76, 49, on1, 0);

to:

  if (arduboy.justPressed(B_BUTTON) && markerPos == 3) arduboy.audio.off();
  if (arduboy.justPressed(A_BUTTON) && markerPos == 3) arduboy.audio.on();

  if(arduboy.audio.enabled()) Sprites::drawOverwrite (76, 49, on1, 0);

Remove the test for sound enabled before all beep.tone() calls. For example:

Change:

if(soundEnable == 0) beep.tone(beep.freq(1000), 1);

to just:

beep.tone(beep.freq(1000), 1);
2 Likes

Many thanks! This is my second game. The first was Ardubloxx. Thanks for the constructive criticism! The fact is that my knowledge is less than my creative impulse. So when I try to create something, I only use what I have already learned. I start writing structured code, but at some point my brain says: hey dude, I already want to play this, finish quickly! :grin: I’m in the process of making a fighting game. And in it, I already feel the need for the order of the code and its efficiency, because there is not enough memory. In general, I will work on my impulsiveness :slight_smile: Thanks for the reminder about the correct use of the array and the readability of the code. I will definitely fix it!

I’ll definitely try to resize the sprite, thanks for reminding me!
The built-in mute function is what I was looking for! thank you so much:)
Regarding the sound, I also wanted to clarify the question: when launching the Ardubloxx and Blade Runner games on ESPboy, the sound does not sound correctly. I understand that it is better to discuss this topic on the appropriate forum, but still I want to figure out if I am using beep.tone correctly?

Guys, please tell me how to change the logo? I searched but could not find instructions…

Do you mean the ARDUBOY logo?

Yes if it is possible

What do you want to change it to?

My usual logo, I don’t know what it will be yet, but it will definitely be a static picture without effects. Of course, if posting my logo is legal :grin:

Then why not just let the ARDUBOY logo be displayed and then display your logo after that in setup()? You could clear the display then display your logo as a sprite and then add a delay to give time to read it.

I think two logos are too much. Besides, precious memory will be consumed.

If people don’t want to wait for the ARDUBOY logo, it can be bypassed in two ways:

  • Pressing the RIGHT button before or during the logo display will abort it.
  • A user can set a flag in system EEPROM to eliminate the display of the logo for all sketches that use the Arduboy2 library in the normal way. The flag can be set using the SetSytemEEPROM example sketch included in the Arduboy2 library.

However, if you are trying to eliminate things to gain more program space, see the Ways to make more code space available to sketches section of the Arduboy2 library documentation.

1 Like

Thanks for the link! The information is very useful :slight_smile:

Wow I love this game!

2 Likes

Glad you liked it!

2 Likes

I apologise in advance if I repeat something someone else has said,.
(If multiple people are saying the same thing, it’s probably a significant issue.)


Firstly, defining variables by doing int variable1, variable2, variable3 is usually less readable than:

int variable1;
int variable2;
int variable3;

You’re mixing byte and uint8_t, but they’re both the same thing (on Arduboy at least).
Typically you should prefer uint8_t because byte is Arduino-specific.


You should probably have moved your images to a separate header file, but I’m presuming you might not actually know how to do that yet. Essentially you’d just make a file with the extension .h, e.g. images.h, and then you’d include it from the main file with #include "images.h".

I decided this would be easiest to demonstrate by example so I made a branch* with these changes to demonstrate.

* If you’re reading this in the (possibly distant) future and the link is broken, try here instead.


As @drummyfish already mentioned, YourPlayer could be changed to deal with arrays instead.

E.g.

const unsigned char * const players[]
{
	ninga,
	girl,
	mush,
	panda,
	filmote,
	MrBlinky,
	Pharap,
	drummyfish,
	Vampirics,
	mlxxxp,
	ESPboy,
};

const unsigned char * const players2[]
{
	ninga2,
	girl2,
	mush2,
	panda2,
	filmote2,
	MrBlinky2,
	Pharap2,
	drummyfish2,
	Vampirics2,
	mlxxxp2,
	ESPboy2,
};

void YourPlayer()
{
	if((markerPlayer >= 0) && (markerPlayer <= 11))
	{
		selectedPlayer = players[markerPlayer];
		selectedPlayer2 = players2[markerPlayer];
	}
}

The way you are using chosenPlayer is inefficient. You’ve got an array of about 11 values, each being either 0 or 1, presumably to indicate which character the player chose, but really you only need one variable to indicate this.

You could instead have just a single variable, uint8_t chosenPlayer, and use a variation of the code I just posted above to figure out which player that represents. E.g.

if((chosenPlayer >= 0) && (chosenPlayer <= 11))
{
	selectedPlayer = players[markerPlayer];
	selectedPlayer2 = players2[markerPlayer];
}

This would also mean that instead of doing:

chosenPlayer[0] = 0;
chosenPlayer[1] = 0;
chosenPlayer[2] = 0;
chosenPlayer[3] = 0;
chosenPlayer[4] = 0;
chosenPlayer[5] = 0;
chosenPlayer[6] = 0;
chosenPlayer[7] = 0;
chosenPlayer[8] = 0;
chosenPlayer[9] = 0;
chosenPlayer[10] = 0;
chosenPlayer[11] = 0;
chosenPlayer[i] = 1;

You would just do:

chosenPlayer = i;

In fact you could probably get rid of the for loop in Players by replacing references to i with markerPlayer.


I also notice that you are comparing some bool variables to 1, but this is redundant…

bool, the boolean data type, holds the boolean values of true or false.
When you use an if, the value between the brackets is implicitly converted to bool, i.e. a value of true or false.

For example, when you do lengthBridge > 0, the value of lengthBridge is compared to 0 and if it is greater than 0 the result of the expression lengthBridge > 0 is true. Otherwise if lengthBridge is 0 or less, the result of the expression lengthBridge > 0 is false.

When you write playerOnPlatform == 1, you are effectively writing true == 1 or false == 1, neither of which makes sense because a boolean value isn’t an integer. To account for this, the compiler will attempt to convert 1 to a boolean value, which it does by saying ’0 is false, everything else is true'. In this case 1 would be true, which means your code ends up being true == true or false == true, either of which is redundant.

Long story short: you don’t need to compare a boolean variable to anything, you can just do if(playerOnPlatform).

If you want to do the opposite (e.g. if player is not on the platform) you would use !, the ‘not’ operator, e.g. if(!playerOnPlatform) (read as ‘if not playerOnPlatform’).

Similarly, instead of assigning values of 1 and 0 to boolean variables, you should assign values of true or false.
E.g. instead of if (lengthBridge > 0) bridgeIsDown = 1; you should do either:

if (lengthBridge > 0)
	bridgeIsDown = true;

Or:

bridgeIsDown = (lengthBridge > 0);

(If you aren’t sure why this works, think about what I said earlier: lengthBridge > 0 is an expression that is evaluated to either true or false, so the datatypes match up.)


You have a bug here:

= is the assignment operator, == is the comparison operator.

What happens with level = 2 is that level is assigned a value of 2, then implicitly converted to bool (as mentioned above, non-zero values are converted to true), thus if(level = 2) is always true.


As others have pointed out, your formatting is a bit hard to follow.

E.g. instead of :

void DrawLine(){
if (arduboy.pressed(B_BUTTON) && allowDrawLine == 1){
    if(platformFirstMove == 0 && platformSecondMove == 2 || platformSecondMove == 0 && platformFirstMove == 2 ){
      buttOn = 1;
      lengthBridge++;
    }
  }
  else buttOn = 0;
  if (lengthBridge > 0){
  if (platformFirstMove == 0 && platformSecondMove == 2) drawBridge(rectFirst.x + randFirstWidth - 1,53, lengthBridge, arduboy.frameCount);
  if (platformSecondMove == 0 && platformFirstMove == 2) drawBridge(rectSecond.x + randSecondWidth - 1,53, lengthBridge,arduboy.frameCount);
  }
  if (arduboy.collide(endLine, rectFirst) || arduboy.collide(endLine, rectSecond)) bridgeOnPlatform = 1;
  if (buttOn == 0 && lengthBridge > 1) allowDrawLine = 0;

  if  (playerOnPlatform == 1) endLine.x = -100;
 }

It would be clearer if rewritten as:

void drawLine()
{
	if (arduboy.pressed(B_BUTTON) && allowDrawLine)
	{
		if(((platformFirstMove == 0) && (platformSecondMove == 2)) || ((platformSecondMove == 0) && (platformFirstMove == 2)))
		{
			buttOn = true;
			lengthBridge++;
		}
	}
	
	if (lengthBridge > 0)
	{
		if ((platformFirstMove == 0) && (platformSecondMove == 2))
			drawBridge(rectFirst.x + randFirstWidth - 1, 53, lengthBridge, arduboy.frameCount);
		
		if ((platformSecondMove == 0) && (platformFirstMove == 2))
			drawBridge(rectSecond.x + randSecondWidth - 1, 53, lengthBridge, arduboy.frameCount);
	}

	if (arduboy.collide(endLine, rectFirst) || arduboy.collide(endLine, rectSecond))
		bridgeOnPlatform = true;
		
	if (buttOn && (lengthBridge > 1))
		allowDrawLine = false;

	if (playerOnPlatform)
		endLine.x = -100;
}

Notice:

  • Proper use of boolean datatypes
    • true and false instead of 1 and 0
    • Using the boolean value directly instead of using == or !=
  • A single space after every if
    • It’s up to you whether you want a space between the if and the (, but make sure the same rule is applied to every if
  • A space after every comma
  • Spaces around all binary operators
  • Extra blank lines to help separate the code blocks
  • Extra brackets to clarify the order of operations

There are tools (typically called ‘linters’) you can use to automatically enforce style rules if you have trouble following rules yourself.

Another quick example:

if(costPlayer[i] < 100)arduboy.setCursor(13, 41);
else arduboy.setCursor(7, 41);

versus:

if(costPlayer[i] < 100)
	arduboy.setCursor(13, 41);
else
	arduboy.setCursor(7, 41);

As others have mentioned, there’s a few naming oddities.

Typically for Arduino projects:

  • Function and variable names are camel case
    • e.g. drawLine
  • Type names are Pascal case

Generally function names should start with verbs and represent actions and variable names should be nouns.

One notable exception is bool variables and functions returning bool, which some people prefer to phrase like a question, e.g. soundEnabled/isSoundEnabled or soundOn/isSoundOn.

Good function name examples: drawLine, drawBridge, updateMenu
Bad function name examples: YourPlayer, Platform, Menu, Bonus


When you use local variables, you should declare them as close to the point you use them as possible.

For example, in Players you declare uint8_t i; at the top of the function and then don’t use it until half way down in for (i = 0; i < 12; i++). You’d be better off just doing for (uint8_t i = 0; i < 12; i++)


Lastly, tittle should be title and ninga should be ninja.

A few of your other words are a bit odd, like markerPlayer (should be playerMarker) and lengthBridge (should be bridgeLength), but I expect that’s due to language differences. E.g. I could imagine someone who spoke French would be tempted to have an enemyBlue instead of blueEnemy.


I can think of a few other ways to improve your code (e.g. introducing you to enumerations) and how your code is hosted on GitHub (e.g. putting it into a subfolder, putting the .hex in a release), but I will let you read through and make sense of what I have already written first. I don’t want to bombard you with too many things.

2 Likes

Thanks for the feedback and detailed explanation! I will definitely work on the bugs in my spare time! I’m sure this will free up a significant amount of memory, and creating separate libraries and writing the code correctly will improve readability. Thanks again for your work!

1 Like

Bugs fixed. Corrected everything you talked about. Hopefully the code looks more sane now. Debugging my code freed 1500 bytes anyway! My compliments to you guys :slight_smile: I’m already starting to feel my brain getting a little bigger :grin:

4 Likes

I am not sure how close to the maximum you were but that’s a huge saving!

2 Likes