Sorry, this is going to be a long post.
Ah, possibly because you didn’t write that function…
Did someone else give you the getImageWidth() and getImageHeight() functions?
Basically they should look more like this:
// Retrieve the width of a Sprites-format image.
uint8_t getImageWidth(const uint8_t * image)
{
return pgm_read_byte(&image[0]);
}
// Retrieve the height of a Sprites-format image.
uint8_t getImageHeight(const uint8_t * image)
{
return pgm_read_byte(&image[1]);
}
(If you get an error after that, you might need to #include <stdint.h> at the top of that file.)
You’re probably overwriting the setting with the score, or vice versa.
Firstly I’d like to mention that you should avoid using EEPROM_STORAGE_SPACE_START directly and should instead choose a semi-random offset from that point.
Purely because if every game used EEPROM_STORAGE_SPACE_START then that part of the EEPROM chip would wear out faster than the rest - the data of all Arduboy games needs to be spread out to ‘wear level’ the chip.
Option 1:
Be very careful with where you’re saving stuff too.
Be aware of how many bytes each object you’re saving uses,
and manually figure out which objects will occupy which EEPROM addresses.
Option 2:
Write your code to carefully calculate the address of each object by taking the address of the previous object and adding the size of that object.
E.g.
constexpr uint16_t object2SaveAddress = (object1SaveAddress + sizeof(object1));
constexpr uint16_t object3SaveAddress = (object2SaveAddress + sizeof(object2));
Option 3:
Make a single struct that stores the whole of your save state.
The advantage is that you can just load the whole thing once at the start of the program and then when saving you don’t need to worry about what specifically you’re saving, you just save the whole thing.
The downside is that to save any single part of the struct you must save the whole struct.
This doesn’t wear out EEPROM because the functions that can save the struct will only write to EEPROM if the value has changed,
but it does mean that you have to be careful about when you save and edit the struct.
For example, if you tried to store the player’s current score in the struct and you offered an option on the pause menu to change a particular setting then when you saved the setting you’d also end up saving the player’s score.
The chances of this being an issue depend on the game.
Also you have to make sure to never remove or reorder any of the member variables (unless you know what you’re doing).
Personally I tend to stick with option 3 because I’ve never been in a situation where any of the theoretical issues with it actually become issues and it’s by far the easiest option.
Here’s another list of suggested improvements.
If you want you can leave looking at this until later
(Some are more pertinent than others.)
More urgent issues
-
launchObstacle:
- Avoid
sizeof(array) / sizeof(*array) and prefer to use the getSize(array) function provided by this header that I wrote. There’s a long explanation in the README.md about why it’s superior - TL;DR: it’s easier to read and makes it harder to accidentally create hard to find bugs.
-
This
for loop is redundant:
-
i starts at 0
-
i < launchTimer is true
--launchTimer-
i--, causes i to underflow, so i becomes 255
-
i < launchTimer is false so the loop ends
- The compiler most likely can’t optimise the loop away
- Instead you should just put
--launchTimer in the launchTimer > 0 branch.
-
playGame:
- Should probably be broken down into more functions. The fact you have a big
/*--- Begin obstacle process ---*/ comment should be a big hint. Any time you feel the need to write a big ‘sectioning’ comment like that, it’s usually a sign that your function has grown to big and should be broken up into smaller pieces.
Advice/tips
-
toggleSoundSettings
- Firstly, this is what
audio.toggle() is for. (You still have to call audio.saveOnOff() afterwards.)
- Some more general advice: You call
audio.saveOnOff() as the last statement of each branch of the if statement, which means that you could just move it outside the if since it’s being called either way. I’m pretty sure that the compiler will notice this and perform that optimisation for you, but you should still do that yourself when possible because it makes the code clearer and something might change that means the compiler can’t make the optimisation.
-
toggleScreenFlash:
- Similar situation to
toggleSoundSettings() - you could move the EEPROM.put calls to afterwards.
Minor optimisations
-
collide & collisionTarget:
-
playerRect could be moved to the start of the function to prevent it being constructed every iteration of the loop.
- Instead of doing
if (matters[i].enabled) { you could do the if(!matters[i].enabled) continue; trick.
-
detectHit:
-
arduboy.justPressed should come before collisionTarget because arduboy.justPressed is a much cheaper function. (If justPressed returns false then you can completely avoid the for loop in collisionTarget.)
-
introduction:
- Instead of drawing the sound icon here and then drawing it again here and then again here, just do the drawing after the
ifs.
- This is one of the reasons why ideally you should avoid mixing updating and rendering. (See “Separation of concerns”.)
Less urgent issues
-
drawBackground:
- Shouldn’t also be moving the ground, that functionality should be a separate function. In general updating and drawing should be kept separate wherever possible. (See “Separation of concerns”.)
-
launchObstacle:
- Avoid using both
rand and random, pick one and stick with it if possible.
-
updateSynapses:
- The
if statements here end up repeating a condtion and having one condition be the inverse of another. This could be rewritten to make the code simpler.
A few stylistic notes:
Naming
Keep your naming consistent. At the moment you’ve got a type called Matter whose launch function is launchObstacle and your array of Synapse is called targets.
Ideally it should be:
-
Matter, matters, launchMatter (or maybe spawnMatter)
-
Synapse, synapses, launchSynapse (or maybe spawnSynapse)
Some other specific things:
-
initializeGame() should probably be called resetGame() if it’s being called again to reset the game after a game over.
-
collide should specify what is colliding with what.
Blank lines
You have far too many blank lines in your code.
A few blank lines to break things up is good, but too many makes the code just as hard to read as having too few.
In particular you typically don’t need a blank line immediately after a start brace or end brace.
E.g. places like this use far too many lines for what little they actually do.
(For an if, for, while etc statement that is. For variables like your Rects it makes more sense to have the space.)
(If you’re struggling to see where a block starts and ends, give Allman style bracing a try instead - it’s much easier to pair up the braces because they’ll be vertically aligned.)
Comments
Don’t bother with all the hyphens around your comments, you’re just wasting your time by typing them.
A single line comment above your function is fine.
If you really need something more obvious, just use three single-line comments and leave the first and last line blank.
E.g.
//
// Retrieve the width of an image
//
And if the function is self-explanatory (as getImageWidth is) then you may not even need it.
i++ vs ++i
Personally I recommend getting used to using ++i instead of i++ for two reasons.
Firstly because the former reads more like ‘increment i’, and secondly because technically i++ creates a copy of i.
The compiler will actually elide that temporary copy for simple types like integers, but in more advanced C++ ++i is used for incrementing objects called ‘iterators’ and those tend to have more expensive copying operations that the compiler can’t elide, at which point using i++ can actually have a performance penalty.
This is a ‘good habits now to avoid problems later’ kind of thing.
Either way, pick a single style and stick to it.
Really nitpicky stuff
The brackets around arduboy.nextFrame() are a waste of time, you’re best off just doing if(!arduboy.nextFrame()).
A unary operator (e.g. ! or ~) will always be evaluated after a function call because otherwise it won’t have a value to operate on,
and a function call will always be evaluated after a member access operator (.) because it depends on the member access to make sense.
Most of all it’s an issue just because it makes the code harder to read than it needs to be.
This is much easier to see because the ! isn’t sandwiched between two (:
if (!arduboy.nextFrame())
return;
Personally speaking I don’t bother with the = for arrays anymore because it’s effectively redundant, but that’s not really a big deal.
E.g.
const uint8_t image[] PROGMEM
{
// data...
};
To avoid sounding too critical or too negative, here is a list of good things:
-
updateSynapses and drawSynapses are separate functions - good separation of concerns
- Using
enum class for tracking states
- Most of the behaviour is out of the main
.ino file
- In most cases the smallest suitable type is in use
- The
int8_t/uint8_t x/y problem has been fixed
-
Rects are being initialised in a better way than before
- The game is (mostly) using
EEPROM properly
- All strings appear to be wrapped in
F macros
- Obstacle sizes are now set at creation time instead of at regular intervals
-
launchObstacle is randomly selecting a Size in a scalable way
- Technically if you only ever have
Small and Medium then this is actually pretty overkill (Size size = (rand() % 2 == 0 ? Size::Small : Size::Medium); would be simpler and cheaper), but if you intend to have more then this will scale up better.
