Ok, finally onto the suggestions/comments.
I’ll be listing these as I spot them, not in order of importance.
Sorry if it seems long.
Some of these things aren’t actually that important,
but I’m mentioning them so that you can make your own decisions about how important they are.
Suggestions and observations...
I’m not entirely convinced that using ‘universal initialisation’ (curly braces) for non-class variables is a good idea, but if you want to stick with it I think you should put a space before the first curly brace.
I.e. you should format things as uint8_t timeBetweenCounts { 40 }; and GameStatus gameStatus { GameStatus::Reset };
GameStatus should probably be called GameState.
‘State’ implies ‘state machine’, ‘status’ makes me think of something like ‘passed’/‘failed’ or status conditions from Pokemon.
Also remember to mark your variables that don’t change as constexpr to signify that they’re actually supposed to be constants rather than variables.
This time around you actually have more reason to have an image variable in Entity because you’re reusing Entity for both player and enemy.
I think this is a good thing, and is probably a sign of good design.
Some people think that an object should control itself completely (e.g. draw itself, update itself),
but modern wisdom suggests that actually that’s not necessarily best.
Sometimes it’s ok for an object to be pure data and for other objects to decide how to update or draw that object.
The latter approach can often be more flexible and leads to less impenetrably deep object hierarchies.
(If none of that means anything to you then don’t worry about it for now.)
Using random to initialise player and enemy is somewhat wasteful because it will be called before setup, so you’ll always get the same value in both cases.
You may as well just use 0 instead.
Global variables are always initialised before the main function.
(The main function is actually hidden from you in Arduino code, but it still exists.
It’s what calls setup and loop. You can even replace it if you know what you’re doing.)
Technically you could pack the three bool variables into a single uint8_t to save RAM but that’s probably a bit overkill in this case.
I thought I’d mention it anyway just so you know it’s a possibility - you might remember about it later when you’re writing another game, at which point you can ask how to do it.
I’m concerned about showCardNumber returning int.
Usually drawing functions shouldn’t draw things.
In fact you appear to be always discarding the return value anyway.
(The compiler really should be warning you about this,
but it didn’t warn me either so now I’m concerned about the compiler’s wellbeing.)
I’m not entirely convinced that showCard and hideCard should be part of Entity,
nor that their names are suitable.
hideCard doesn’t sound like a function that would draw something to the screen.
And instead of drawing then undoing a drawing,
wouldn’t it make more sense to just not draw in the first place?
I can find a way of replacing getImageWidth(characterHold) that saves a bit of memory by allowing the expression to be evaluated at compile time, but that’s not a big deal, it’s only 22 bytes of progmem and you have plenty to spare.
(getImageWidth contains some assembly code that prevents it being evaluated at compile time.)
(On the bright side, I’ve finally found a case where marking a function constexpr actually uses less memory than not marking it constexpr, which means my advice to people to use constexpr whenever possible is finally validated with real evidence! :P)
resetCardNumbers seems a bit odd.
Shouldn’t you be picking a new number for the cards when you give the player a new card rather than when you reset the old one?
I’m kind of confused as to what countDown is doing.
It seems like it might be overly complex and/or trying to do multiple things at once.
Also it’s being called from showCountdown,
so you’re mixing your drawing code with your update code,
which is generally a bad idea.
Ideally you want to keep your updating and drawing separate.
Partly so you can always draw everything without worrying about changing any state.
The main benefits of this are:
- It’s sometimes useful for debugging
- It makes bugs easier to track down because you have less intermingling parts
- It can make porting easier if you ever decide to port your game. (Because you theoretically won’t have to change any update logic (you still have to change logic that depends on button presses, but otherwise you shouldn’t need to change anything else).)
- This doesn’t really apply to the Arduboy much, but larger games separate updating and drawing so that the update speed isn’t tied to the render speed and the updates can ‘catch up’ if they start falling behind due to slow rendering.
Also, for an example of why it’s bad using some of your later code as an example:
The order in which you call drawCards and drawPlayer or drawEnemy affects what gets drawn.
With the exception of drawing things on top of each other,
the order in which you call draw functions shouldn’t make that much of a difference.
Some of your other drawing functions (e.g. drawPlayer and drawEnemy) also suffer from this issue.
Speaking of which drawPlayer and drawEnemy could actually be merged into a single drawEntity.
Something like this:
void drawEntity(Entity entity)
{
Sprites::drawOverwrite(entity.x, entity.y, entity.image, 0);
if (entity.isHoldingCard)
Sprites::drawOverwrite(entity.x, entity.y, characterHold, 0);
if (entity.didSwingHammer)
Sprites::drawOverwrite(entity.x, entity.y, characterSwing, 0);
if (enemy.didSwingHammer)
Sprites::drawOverwrite(entity.x, entity.y, characterHit, 0);
if (entity.didDodgeHammer)
Sprites::drawOverwrite(entity.x, entity.y, characterDodge, 0);
}
Though I notice you’re actually using slightly different images for each case.
So I’d recommend solving that by making a new struct:
struct EntityImages
{
const uint8_t * defaultImage;
const uint8_t * holdImage;
const uint8_t * swingImage;
const uint8_t * hitImage;
const uint8_t * dodgeImage;
};
And then either incorporating that directly into Entity:
struct Entity
{
// ...
EntityImages images;
// ...
};
Or having it as const EntityImages * and storing the actual EntityImages objects in progmem (though you may not know enough about progmem yet to attempt that).
The former uses more RAM but is slightly more flexible (not that you’re likely to need the extra flexibility) and the latter uses less RAM but is a little bit more difficult to use.
I’ll need a bit of time to do so, but I’m pretty sure I can rework mainAction to be a bit more straightforward…
Nothing much to say about introduction other than maybe:
arduboy.print(arduboy.audio.enabled() ? F("on") : F("off"));
Might look a tad cleaner? Personal preference I guess.
The meaning of inGame is a tad unclear to me.
Perhaps you should be having an extra state instead?
(Or maybe a few more extra states? One for waiting to display numbers, one for waiting for the player to respond to the displayed numbers and one for responding to the player’s decision or lack thereof.)
gameOver is fine. My only criticism is one of actual game logic.
If enemy and player both have winning scores then player gets precendence.
I guess technically only one player can score per turn,
so perhaps it isn’t as much of an issue.
In which case maybe a simple else would suffice, since the former case being false implies that the latter is true?
(The compiler won’t know that, and even if it could prove that, it can’t legally do anything about it, so you’d be stuck with the wasted comparison.)
That is all.
I’ll change that.