Blade Runner

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

It looks much better already.

Let us known iIf you are prepared for more improvement suggestions.

I’m going to give you two suggestions now without you asking, but only because I think you will learn some important skills…


Firstly, in this code:

You do not need the loop, you can just do:

selectedPlayer = players[chosenPlayer];
selectedPlayer2 = players2[chosenPlayer];

There’s a reason this works, and I’m going to prove this is correct by teaching you an important technique.

The best way to understand your code is to pretend to be the computer. This is one of the most important programming techniques there is.

Imagine you are the computer going through the loop. You start with i at 0, and you go through until i is 11.

When i is equal to chosenPlayer, you set selectedPlayer and selectedPlayer2.

That means at this point in the code, i and chosenPlayer are the same. That means that this:

for(uint8_t i = 0; i < 12; i++){
	if(chosenPlayer == i){
		selectedPlayer = players[i];
		selectedPlayer2 = players2[i];
	}
}

Is the same as:

for(uint8_t i = 0; i < 12; i++){
	if(chosenPlayer == i){
		selectedPlayer = players[chosenPlayer];
		selectedPlayer2 = players2[chosenPlayer];
	}
}

Because for the duration of the inner block (the code between the inner two curly braces ({ })), chosenPlayer and i must be the same number, because the block only runs if(chosenPlayer == i).

But hang on a minute, we established earlier that if(chosenPlayer == i) is only true once during the loop. For all the other numbers, nothing happens. Only one number is doing anything, and we already know what that number is - it’s chosenPlayer. So we don’t need i, and we don’t need that loop.

selectedPlayer = players[chosenPlayer];
selectedPlayer2 = players2[chosenPlayer];

Secondly, this:

Can be simplified to:

if(!flipPlayer){
	Sprites::drawPlusMask (player.x, player.y + 1, selectedPlayer, frames);
}
else{
	Sprites::drawPlusMask (player.x, player.y + 12, selectedPlayer2, frames);
}

Timer++;

This one is simple logic. !flipPlayer and flipPlayer are opposites:

  • If !flipPlayer is true, flipPlayer is false
  • If !flipPlayer is false, flipPlayer is true

By definition, that is what ! (not) means.

That means this:

if(!flipPlayer){
	Sprites::drawPlusMask (player.x, player.y + 1, selectedPlayer, frames);
	Timer++;
}
if(flipPlayer){
	Sprites::drawPlusMask (player.x, player.y + 12, selectedPlayer2, frames);
	Timer++;
}

Can become this:

if(!flipPlayer){
	Sprites::drawPlusMask (player.x, player.y + 1, selectedPlayer, frames);
	Timer++;
}
else{
	Sprites::drawPlusMask (player.x, player.y + 12, selectedPlayer2, frames);
	Timer++;
}

(Note that this only works if flipPlayer doesn’t change during the first if block.)

But now, Timer++ happens on both the if and the else, which means that it happens regardless of what flipPlayer is. So actually, it always happens, it doesn’t need to be conditional:

if(!flipPlayer){
	Sprites::drawPlusMask (player.x, player.y + 1, selectedPlayer, frames);
}
else{
	Sprites::drawPlusMask (player.x, player.y + 12, selectedPlayer2, frames);
}

Timer++;

To be honest, you don’t actually have to make either of these changes (the else change and the Timer++ change), because the compiler is smart enough to do them for you. These are both standard compiler optimisations.

But you should makes these changes anyway because it makes your code easier to read and understand. Without those changes, it’s not immediately obvious that Timer++ always happens, or that only one of the two blocks runs, you have to think about the code to realise that’s what’s happening.


Does that all make sense?

1 Like

@NoobGeek,

Something similar to what @Pharap has described, is the case where you’re testing for one thing multiple times and then do something based on further testing:

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

Here, both if statements are testing for markerPos == 3. The value of markerPos isn’t going to change between each test, so you could do this test first for both of them:

  if(markerPos == 3){
    if(arduboy.justPressed(B_BUTTON))
      arduboy.audio.off();
    if(arduboy.justPressed(A_BUTTON))
      arduboy.audio.on();
  }
2 Likes

@Pharap @MLXXXp
Yes, now I get it. This time it was not so difficult, but I understand how important it is. Thank you!
Of course I am ready for new information! If you will allow me, I will ask a question on the use of sound that has been troubling me for a long time. As you can see, I use a Timer for each group of sounds. I am sure that this is not the correct approach, but using other methods leads to various artifacts, such as endless beeeep and so on. If not difficult, explain the easiest way to get the right sound. I have seen many people use arrays of sounds, but I do not fully understand how they can be used with different durations and pauses.

1 Like

The BeepPin classes in the Arduboy2 library are mostly intended to play single tones, not sequences of tones. If you want to play sequences of multiple tones and rests, the ArduboyTones library is a good option for this. It may require more program space, though.

2 Likes

Something that is always recommended for people using the Arduino IDE for development:

Open the IDE preferences:
File → Preferences

and change Compiler warnings to All
(Remember to click on OK, at the bottom right, to save the new settings.)

You should then try to eliminate all compiler warnings that are generated. (You can generally ignore warnings for files that aren’t part of your sketch, such as those generated for EEPROM.h)

For example, the current code is generating a warning for variable xCursor:
warning: invalid conversion from 'int' to 'const char*'
because it’s been defined as an array of pointers to char.

const char *xCursor[] = {50, 42, 44, 46, 50, 45, 38, 48, 35, 37, 48, 47};

should be:

const char xCursor[] = {50, 42, 44, 46, 50, 45, 38, 48, 35, 37, 48, 47};

There are also a few more warnings that you should try to eliminate. If you can’t figure out the reason for them, please ask and I or someone else will help you.

2 Likes

Oh … I forgot to answer you, sorry! Thanks for reminding me about using arduboy Tones! This will certainly help in the future. I will definitely open access to all kinds of errors!

2 Likes