Monpals - Bug Fixing and Improvements

#1

I was playing the game (which for me is actually somewhat rare - I spend more time flicking through code than I do actually playing Arduboy games) and I spotted a bug, which I’ve since identified in the code.

When you release a mon, the code that’s supposed to shift the monsters down in the party has a bug where it tries to read outside the party array,
so instead of releasing the mon it copies some unrelated data into the array,
which gives the player a random mon.

Edit:
It’s not entirely random, it’s controllable…

If you make sure that the mon you release is the second mon in the party then you end up getting a copy of the mon you last fought.
So actually this is one of those bugs that can be abused.
If you want a particular mon, you just spam the dojo until it turns up and then release your mon.

Worse yet, that means the code is also zeroing out some unrelated data.
(I think it’s either player or enemy that gets zeroed out, but technically reading outside the array is ‘undefined behaviour’ so that’s not guaranteed behaviour on other computers/game systems.)

The problem is here:

It can be fixed by doing:

if ((monpals.mons[menuCursor][0] == 0) && (monpals.mons[menuCursor + 1][0] > 16) && (menuCursor <= 2))
{
	// Move the monpals down, one by one
	for (uint8_t i = menuCursor; i < 2; ++i)
	{
		// Create a local copy of the next monpal
		uint8_t next0 = monpals.mons[i + 1][0];
		uint8_t next1 = monpals.mons[i + 1][1];
		
		// If the next monpal isn't empty
		if(next0 != 0)
		{	
			// The next monpal is copied into the current monpal's slot
			monpals.mons[i][0] = next0;
			monpals.mons[i][1] = next1;
		}
		// Otherwise, if the next monpal is empty
		else
		{
			// Erase the current monpal because it's the last one
			monpals.mons[i][0] = 0;
			monpals.mons[i][1] = 0;
		}
	}
}

Hopefully that makes sense to you. If not I can explain it in more detail.
I think the best way to understand it is to work it out on paper,
following the algorithm.

There’s a few other ways to do this, some of which are easier to understand (like swapping the monpals),
but most other approaches use more resources (e.g. procecssing time).

I would have suggested this as a PR, but your code doesn’t have a licence so technically I don’t have permission to modify it.

And by the way, just since I noticed it…
The only times you have to put a semicolon after an end curcly brace are:

  • When defining a struct, class, enum, enum class or union
  • When using curly braces for initialisation
    • E.g. initialising an array, or initialising an object with ‘uniform initialisation’ syntax
  • Lambda expressions.

(There might be some obscure cases I’m forgetting about, but that’s all I can think of offhand.)

If there are some of those you don’t know the meaning of, don’t worry too much.
If you don’t know what they are then you’re unlikely to encounter them when writing your own code.

Conversely you do not need a semicolon for:

  • Function definitions
  • for or while loops
  • if or else statements
  • Scope blocks
  • Probably any other time you might need to use a curly brace
1 Like
Monpals - A Monster Battling RPG
#2

If you are looking for savings, I would look to combine all your graphics into arrays and then streamline this from:

void BattleVisuals() {
...
if (monpals.spc[a] == 1){
if (b == 0) arduboy.drawBitmap(0, 24, audiouseBack, 24, 24, WHITE);
if (b == 1) arduboy.drawBitmap(104, 0, audiouseFront, 24, 24, WHITE);
};
if (monpals.spc[a] == 2){
if (b == 0) arduboy.drawBitmap(0, 24, beethroneBack, 24, 24, WHITE);
if (b == 1) arduboy.drawBitmap(104, 0, beethroneFront, 24, 24, WHITE);
};
...

to

void BattleVisuals() {
...
if (b == 0) arduboy.drawBitmap(0, 24, imagesFront[monpals.spc[a]], 24, 24, WHITE);
if (b == 1) arduboy.drawBitmap(104,04, imagesBack[monpals.spc[a]], 24, 24, WHITE);

Likewise, this code:

void MenuVisuals() {
...
if (menuCursor == 0) arduboy.drawBitmap( 9, 29, characterOne, 8, 8, WHITE);
if (menuCursor == 1) arduboy.drawBitmap( 43, 29, characterOne, 8, 8, WHITE);
if (menuCursor == 2) arduboy.drawBitmap( 77, 29, characterOne, 8, 8, WHITE);
if (menuCursor == 3) arduboy.drawBitmap( 111, 29, characterOne, 8, 8, WHITE);
...
if (menuCursor == 0) arduboy.drawBitmap( 9, 29, characterTwo, 8, 8, WHITE);
if (menuCursor == 1) arduboy.drawBitmap( 43, 29, characterTwo, 8, 8, WHITE);
if (menuCursor == 2) arduboy.drawBitmap( 77, 29, characterTwo, 8, 8, WHITE);
if (menuCursor == 3) arduboy.drawBitmap( 111, 29, characterTwo, 8, 8, WHITE);

… could become:


const uint8_t posX[4] = { 9, 43, 77, 111 };

void MenuVisuals() {
...
arduboy.drawBitmap(posX[menuCursor], 29, characterOne, 8, 8, WHITE);
...
arduboy.drawBitmap(posX[menuCursor], 29, characterTwo, 8, 8, WHITE);

Probably not a big saving but much more readable.

This could also be done in the same way. At the very least, use a switch!

if (buttonClick == 1) { 
menuCursor=2;
buttonClick=0;
} else if (buttonClick == 2) { 
menuCursor=0;
buttonClick=1;
} else if (buttonClick == 3) { 
menuCursor=3;
buttonClick=0;
} else if (buttonClick == 4) { 
menuCursor=1;
buttonClick=0;
...

and this …

void MonSpcStatConstruct() {

if (monpals.spc[a] == 1){monpals.spd[a]+=3, ++monpals.def[a], ++monpals.atk[a], monpals.maxEng[a]+=3, monpals.type[a]=3;};
if (monpals.spc[a] == 2){monpals.spd[a]+=2, monpals.def[a]+=3, ++monpals.atk[a], monpals.maxEng[a]+=2, monpals.type[a]=2;};
if (monpals.spc[a] == 3){++monpals.spd[a], monpals.def[a]+=3, ++monpals.atk[a], monpals.maxEng[a]+=3, monpals.type[a]=1;};
if (monpals.spc[a] == 4){monpals.spd[a]+=3, ++monpals.def[a], monpals.atk[a]+=2, monpals.maxEng[a]+=2, monpals.type[a]=1;};

...
}
2 Likes
#3

I should have added … I love the game however when we go into battle the emulator seems to freeze. I need to test this on a real arduboy to see it its just an emulator issue.

Nice work!

2 Likes
#4

Sorry for the late response, I was at work.

Thank you for finding this, I’ll be sure to fix that asap!

So would I also need the the Apache 2.0 license within the code and not just the repository?

Thank you. I’ll be sure to makes those changes as soon as I get the chance.

I’m not at all familiar with registers, but I’ll definitely look into that and implement that in once I know.

I would have to say that Chlorosaur is my favorite one in terms of design.
In terms of name, my favorite is Beethrone. I gave myself a little self-indulgent pat on the back for coming up with that one :laughing:

Okard is Drako backwards and Spymera is a portmanteau of the words spy and camera.
Also Kribbit is a portmanteau of the name of the muppet character, kermit and ribbit, thought I think kero would of been better to base its name off of.

That’s strange. It worked for me on my phone and laptop, though there isn’t any sound (which I’m guessing is normal) and it loads all the variables with 255 when pressing load on the start screen.

#5

Ah, I didn’t spot that there was a licence file,
Probably either because I was looking at the code or I was accidentally looking at another GitHub tab at the time.

It’s actually recommended in the licence text to put the provided boilerplate message at the top of your code:

In your case:

//
//  Copyright (C) 2019 Revlis
//
//  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//       http://www.apache.org/licenses/LICENSE-2.0
//
//  Unless required by applicable law or agreed to in writing, software
//  distributed under the License is distributed on an "AS IS" BASIS,
//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
//  See the License for the specific language governing permissions and
//  limitations under the License.
//

Technically you don’t really need to know what registers are because you don’t actually get to decide how and where they’re used (unless you start writing in assembly, of course).
I was just explaining why using local variables would save memory so you had at least a vague idea of why.

If you don’t want to get too far into hardware details,
just imagine they’re another kind of memory that’s faster and cheaper than RAM, but there’s a lot less of it.

Local variables can exist in registers because local variables can’t be accessed outside the function they’re defined in,
whilst global variables can’t exist in registers because they technically have to be available to the whole program.

If you do want to know more about the hardware side of things then that’s good, but it’s not critical knowledge.

Even without being more optimal, you should use local variables anyway simply because they make code more readable and more useful.

Ah, I might have got it if it had been ‘Ocard’.
It’s unusual to see ‘draco’ spelt with a ‘k’.

Ah, I thought it was going to be ‘chimera’ (since it would then rhyme with ‘chimera’).
Either that or some obscure word I hadn’t heard of.
Spymera looks a bit like a seed to me, so I was expecting some kind of botanical term.

I’m glad you chose this section to edit…

@Revlis
Those should be semicolons, not commas.
It probably still compiles, but if so then you really don’t want to know why it works.

I can actually see a better way to structure this particular section of code.
I notice that in each statement you modify the same 4 values:
spd, def, atk and maxEng.

So instead of repeating the same thing over and over again with different values,
you can use a 2D array to do this:

const uint8_t statLookup[15][5]
{
	// spd, def, atk, maxEng, type
	{ 3, 1, 1, 3, 3 },
	{ 2, 3, 1, 2, 2 },
	{ 1, 3, 1, 3, 1 },
	{ 3, 1, 2, 2, 1 },
	// et cetera
};

void MonSpcStatConstruct()
{
	uint8_t spc = monpals.spc[a];
	if((spc > 0) && (spc < 16))
	{
		uint8_t index = (spc - 1);

		// Add stats
		monpals.spd[a] += statLookup[spc][0];
		monpals.def[a] += statLookup[spc][1];
		monpals.atk[a] += statLookup[spc][2];
		monpals.maxEng[a] += statLookup[spc][3];

		// Set type
		monpals.type[a] = statLookup[spc][4];		
	}
}

There are in fact even better ways of doing it than this.
In particular there’s a way where you can do statLookup[spc].spd instead of statLookup[spc][0],
but I won’t explain that yet because I want to explain how to apply that idea to where you’re currently doing monpals.mons[x][0],
and possibly also explain how you could be using monpals[a].spd instead of monpals.spd[a].

But all those things would require quite a bit more explaning,
and I don’t really have time to explain much at the moment.

Hopefully you can see how this works since it’s fairly similar to how you’ve structures monpals.mons (i.e. a 2D array),
but if not I’ll explain it when I’m next around.

2 Likes
#6

I added the boilerplate message to the code along with the bug fix you suggested. I also gave you credit around that bit of code as well, if that’s the correct way of doing it, or do I put it in the top of the code and/or title screen? Didn’t yet add the other changes at the moment, but I’ll try to put them in once I get the time to.

1 Like
#7

Thanks.

It’s entirely up to you.
Putting the note close to the change (as you did) probably makes more sense.

Usually it’s easier when people commit to GitHub via PR because then there’s a commit message that documents the change.

I tend to avoid making PRs until I know someone’s happy with a change though.

GitHub takes some time to get used to, but it can be really useful when you get to grips with how it works.

There’s no rush.

Do you understand what they’re all doing and/or how they work?

I’ve just been running the code to test it because I actually didn’t test if my bugfix would work or not and it would be really awkard if it didn’t.
Fortunately it does work as intended, so my reputation is safe. :P

Looking back at the code I accidentally made the bugfix do more work than it needs to.
(Probably because I was half asleep at the time.)

I’ve made a PR (pull request) with a simpler version,
which apparently you discovered while I was in the middle of writing this comment and have already merged.

1 Like
#8

Yeah, the moment after I got a gmail from github about a pull request, I quickly went to the site and merged it.

most of it. I don’t know how to make my variables local instead of global.

I also don’t understand that bit either. The rest of it seems simple enough.

I’m also curious as to why I should use semicolons and not commas.

Also thank you and @filmote for the help thus far. :slight_smile:

1 Like
#9

A local variable is a variable declared somewhere within a function.

At the moment you’re declaring almost all your variables at the start of the program outside of any functions.
The ones that don’t need to persist for more than one function can simply be made local.
In particular the ones you’re using for loop counters or only using in one function would be better off as local variables.

Here’s a simplified example of using a global variable as a counter in a for loop:

#include <Arduboy2.h>

Arduboy2 arduboy;

uint8_t variable = 0;

void setup()
{
	arduboy.begin();
}

void loop()
{
	if(!arduboy.nextFrame())
		return;
		
	arduboy.pollButtons();
	
	arduboy.clear();
	
	for(variable = 0; variable < 10; ++variable)
	{
		arduboy.print('a');
	}
	
	arduboy.display();
}

And here’s the same program now using a local variable instead:

#include <Arduboy2.h>

Arduboy2 arduboy;

void setup()
{
	arduboy.begin();
}

void loop()
{
	if(!arduboy.nextFrame())
		return;
		
	arduboy.pollButtons();
	
	arduboy.clear();
	
	for(uint8_t variable = 0; variable < 10; ++variable)
	{
		arduboy.print('a');
	}
	
	arduboy.display();
}

Now the variable named variable only exists for the duration of the for loop because it’s declared as part of the for loop’s ‘initialisation statement’ (the first of the three parts that go inside a for loop’s brackets).

(I’d like to point out that the second version uses 20 bytes less progmem than the first version.
Those extra 20 bytes are the extra instructions needed for reading and writing the global variable in RAM.
The second version also uses 1 less byte of RAM because the local variable doesn’t need to be kept in RAM.)

Using a local variable that isn’t a for loop counter is also quite simple.
For simplicity I’ll demonstrate using a program that does the same thing but using a while loop instead:

#include <Arduboy2.h>

Arduboy2 arduboy;

void setup()
{
	arduboy.begin();
}

void loop()
{
	if(!arduboy.nextFrame())
		return;
		
	arduboy.pollButtons();
	
	arduboy.clear();
	
	uint8_t variable = 0;
	
	while(variable < 10)
	{
		arduboy.print('a');
		 ++variable;
	}
	
	arduboy.display();
}

If you’re happy enough with just that, then you don’t have to read the next bit,
but while I’m at it I’m going to explain what scope is and some other slightly more advanced stuff.
You’ll need to know this stuff at some point to be able to use local variables properly,
but you don’t necessarily need to know about it immediately.

About scope...

The part of the program that a variable exists for and/or is accessible from is called its ‘scope’.
When a variable’s scope ends, the variable ceases to ‘exist’,
and usually it is also destroyed at that point.

Global variables have a scope that spans the entire program.
Local variables have a scope that spans until the block they’re in ends.

A block is (roughly speaking) started by a { and ended with a }.
For example, in the code:

if(someCondition)
{
	uint8_t someVariable = 0;
}

someVariable exists from the line it’s defined in until the } that terminates the if.

This means that technically the variable in the third program (the while version) exists for a bit longer than in the second program (the for version).

This scoping rule is also why fors allow a loop variable to be defined within their round brackets.

If you declare a variable in the body of a for loop (between the loop’s curly braces - more formally the ‘body’ of the loop),
that variable ceases to exist at the end of an iteration of the loop,
and is then recreated when it is encountered in the next iteration,
meaning it can’t persist between iterations.

And if you were to declare it before the for loop started, it would exist after the for loop ended.

Thus you are allowed to define it as part of the initial for part, so it can exist for all iterations of the loop, but still not outside of the loop.

That’s going to take quite a bit of explaining, which is one of the reasons I said “I won’t explain that yet”.

One of the other reasons for not explaining that yet is because it will be easier to explain after some of the other changes have been made and the code has been cleaned up and simplified a bit.

(At the very least once the code has been properly indented and slimmed down a bit.)

The short answer is because a semicolon is what you use to terminate a statement, and a comma is something different (a ‘comma opertator’) that does something different.
In the example shown, each of the operations should be a separate statement, not chained together into one long statement as the comma operator allows.

The long answer would require going into detail about the difference between a statement and an expression,
explaining what the ‘comma operator’ is, and what it does,
and possibly touching on some language rules about expression evaluation.
I.e. boring language-lawyering stuff.

I’m perfectly happy to explain it,
but most people find that kind of stuff really boring and quite difficult.

One other thing while I think of it:
In case you don’t know what the && I used in my bugfix is, it’s the ‘logical and’ operator.

It basically has the same meaning as the ands you’ve been writing,
but it’s actually the ‘proper’ version of the ‘logical and’ operator.
and is a version that only exists as a backup - as a remnant from the days where some keyboards didn’t have a way to write an & symbol.

Similarly || is preferred over or, and the relationship between the two is pretty much the same as between && and and.

and and or are actually even called ‘alternative operator representations’,
precisely because they are the alternative/‘back-up’ versions that shouldn’t be used in general.

You should aim to switch to using && and || when possible, and get used to using those, because they’re what 99% of C++ code uses.
(I made that figure up, but it’s probably not far off the truth.)

Lastly, if your worried about our suggestions cluttering up the thread and drowning out any praise people have for your game, then we could move these comments to another thread or a PM if you want.

1 Like
#10

Okay, now I finally understand how local variables work! :slight_smile:

Fair enough. I’ll keep that in mind for future projects and of course when I get around to cleaning up the code.

I’ll make sure to implement that in my code from now on.

That sounds like a good idea. It should be a seperate thread rather than a PM so anyone else new to C++ can learn from this besides me.

1 Like
#11

Done.

In fairness you should probably still learn the difference between a statement and an expression at some point because it will come in handy, but it can wait until you’re a bit more experienced.

Even then the comma operator is something you should generally avoid.
Pretty much every possible use of it that I’m aware of are what I’d consider ‘improper hacks’.
I’m only aware of one use of it that I’d consider ‘legitimate’, and it’s a really specific use-case.

1 Like
#12

I went through and reformatted the code to use proper indentation, Allman-style bracing, one statement per line and other spacing improvements.

I also moved the sound and images into separate header files.

Do these changes look like something you would be interested in incorporating with the original?

(And yes, I did this all by hand like a neanderthal instead of attempting to write a parser to do it for me or downloading an already-existing formatting tool.
In my defence I wasn’t expecting it to take as long as it did.)

#13

You know VSCode can do this automatically? And I know you do use VSCode …

#14

There’s a lot about VSCode I don’t know.
I find a lot of the documentation to be either lacking info or poorly explained.

Is it possibe without installing yet another extension?
And if so, where do I change the details of the format style?
(Brace style, tabs vs spaces, tiny context-specific details…)

#15

To format, highlight the code (ctrl-a) right click and select Format Document.

Let me look to see how to set the style.

Looks like you can create a .clang format file if you don’t like the default.

#16

I can only imagine the amount of work that took, thank you! :smiley:

So if I wanted to incorporate your version into the master branch, how would I merge it?

1 Like
#17

@pharap … is your tab set to 8 characters? You must love scrolling left-right or have a really, really wide screen!

#18

Last I checked that required the clang formatting tool and/or an extension to be installed,
but maybe I got the wrong end of the stick.

Either way it doesn’t say where to put the .clang file.
I’m dreading the possibility of the answer being you need a separate .clang file for every project’.
Even git has a way to have global gitignore settings (even if they do insist on being in your ‘home’ directory instead of next to the executable where they ought to be).

It’s set to 4. Always.

8 character per tab is a mild form of insanity.

I have this fun little invention called ‘line wrapping’ enabled.

1920x1080 is suitably wide. Much nicer than the days of 1366x720.

It’s more time than effort.
I don’t mind a bit of mindless ‘drone’ work.
Sometimes not having to think is a nice relief.

If you’re definitely interested then I’ll make a PR.
Before I do though, I’ll add two more changes - I’ll swap the ands for &&s and I’ll add some brackets around some of the expressions to make them a bit more obvious.

1 Like
#19

I don’t use the feature so I don’t really know.

In Github your code looks to have 8 characters.

Yuk.

#20

@Revlis

I’ve got a PR ready now. I also managed to clear up a few things I’d missed along the way:

Hopefully the code still makes sense to you.
Like I say, I specifically only changed formatting-related things,
I haven’t modified any of the code’s behaviour at all.
(Not intentionally anyway. I doubt I accidentally changed anything but it’s always possible.)

The ‘diff’ is going to look a little odd because GitHub’s ‘diff’ tool starts to struggle after so many changes.
Finding differences between two files is rarely easy.

As far as I’m aware that’s GitHub’s decision.
The code itself still has tabs, which is the important thing.

It’s mainly for other people’s code. :P

My code doesn’t tend to have that many long lines overall for a number of reasons.

1 Like