ArduJudge - Judge demake

A simple, barebones remake of the Game & Watch game Judge. I mostly wanted to make this for some practice, but it works and is kind of fun so I figured I might as well share it. It’s pretty fast, so try to keep up!

ArduJudge.hex (40.0 KB)

ArduboyCapture

ArduboyCapture(2)

Source code
Hex file

The main difference between this one and the original version is that your player is on the left side instead of the right. The countdown speed is also always the same - the original version would change it up every turn.

If your number is bigger than or equal to the enemy on the right, hit the A button to whack him with your hammer. If it’s smaller, hit B to dodge. Hits are worth 3 points, dodges are worth 2. If you hit the wrong button, the enemy gets the points. First one to 99 points wins!

8 Likes

This is really cool and I hadn’t seen the original G&W. Its elegantly simple!

tumblr_lmjaeiWKVX1qzj5ggo1_640

3 Likes

Thanks! You could probably build a much more faithful remake in half the time it took me, but this was fun!

These G&W lend themselves to the Arduboy and make great learning projects - they are not so complex that you think you will never finish them. Mine only look faithful because I have a master artist @Vampirics doing all the hard work!

3 Likes

Surprisingly effective.


Are you looking for code improvement suggestions at all?

Yeah, whenever you have the chance!

I can already predict the biggest culprits, like the big if statements in the mainAction() function and probably the use of delay(2000).

1 Like

delay is just evil :crazy_face:

1 Like

But it does exactly what I need! :yum:

You’re ahead of me there. I hadn’t even gotten round to thinking about complaining about that.

You’re right though, it’s violating DRY to the point where I’m struggling to even find any difference between the branches. :P

Maximum points for Vampirics here, delay makes me cry internally.

It might be seemingly more elegant or less complex than some of the alternatives,
but you’re effectively halting the entire CPU for that time span so you can’t actually do anything else in the meantime - not even idle the CPU to save power.

Here’s what delay actually looks like on the inside:

(Technically you could make yield() idle the CPU, but I won’t go into that now.)


I won’t look it all over just yet because I need to eat something before I forget,
but here’s something I’ll point out now:

Instead of:

  if (arduboy.audio.enabled()) {
    arduboy.audio.off();
  } else {
    arduboy.audio.on();
  }

You can just do arduboy.audo.toggle().

(I might have mentioned this last time. Or maybe I said it to someone else?)

1 Like

More like arduboy.audio.toggle() :wink:

Clearly my fingers move too fast for the keyboard. :P

(Technically you could also just do Arduboy2Audio::toggle() instead.)

You did mention this last time, and I forgot all about it :upside_down_face: I’ll change that.

I’ll be most interested in your improvement for this function. I had it a little shorter before but it started getting longer when I realized that everything really needed to be different for each scenario with those numbers. But yeah, stuff is definitely repeated when it probably doesn’t need to be.

1 Like

I changed it to arduboy.audio.toggle(), and got rid of delay(). It was bothering me too ever since I added it, so hopefully it’s a little more efficient now. I’m making a new minor release.

1 Like

You get a happy face for replacing delay, but a frown for trying to implicitly cast inGameTimer to bool.
Don’t let integers implicitly cast to bool, use == 0, != 0 or > 0 (whichever is more appropriate).

Most people understand what casting an integer to bool does,
but it hides the conversion and makes what’s going on less obvious.
(A principle otherwise known as “Explicit is better than implicit.”.)


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.

1 Like

Whoa, thank you! Just in a first read-through I can tell which ones I’ll be getting to first. Basically, if they’re broad enough to be applied to future games (like the separate struct for the images and drawing vs. updating) I’m thinking they’ll be most important.

1 Like

I believe I’ve found a bug (or two) in the logic.

  1. Play a game.
  2. Press B or A repeatedly.
  3. Hilarity ensues.
  4. ???
  5. Profit.

Rapidly pressing the same button multiple times in a single turn can earn you or your opponent multiple points.

I think my “add some new states” suggestion could fix this.

Dang it! This wasn’t happening when I was using delay(), for obvious reasons.

Now where’s that profit?

Just in case anyone doesn’t get the joke here’s a Reddit link. :P


Like I say, the best solution is probably to just switch to a new state after the player presses A or B,
then just hold that state until the timer elapses.

One thing I’m confused about: what is afterRoundTimer supposed to do?

I’m really close to having fixed that bug without adding a new state by refactoring that 'big if',
but afterRoundTimer is causing me a bit of confusion.

I still think adding extra states will probably be the best/easiest solution,
but I wanted to have another one just to prove it’s possible.

Sorry to triple-post, but here you go:

// Check for a processed action
bool mainActionProcessed { false };

void processMainAction()
{
	if (inGameTimer > 0)
	{
		// Player control
		if (arduboy.justPressed(A_BUTTON))
		{
			mainActionProcessed = true;
			player.didSwingHammer = true;
			
			if (player.cardNumber >= enemy.cardNumber)
				player.score += 3;
			else
				enemy.score += 3;
				
			sound.tone(NOTE_G5, 50);
		}
		else if (arduboy.justPressed(B_BUTTON))
		{
			mainActionProcessed = true;
			player.didDodgeHammer = true;
			
			if (player.cardNumber <= enemy.cardNumber)
				player.score += 2;
			else
				enemy.score += 2;
				
			sound.tone(NOTE_G5, 50);
		}
	}
	// If the time is up and player hasn't hit either button have the enemy make a decision
	else
	{
		// Enemy AI
		if (player.cardNumber > enemy.cardNumber)
		{
			enemy.didDodgeHammer = true;
			enemy.score += 2;
		}
		else
		{
			enemy.didSwingHammer = true;
			enemy.score += 3;
		}
		
		sound.tone(NOTE_G5, 50);
		mainActionProcessed = true;
	}
}

void mainAction()
{
	if(!mainActionProcessed)
		processMainAction();

	if (inGameTimer > 0)
	{
		--inGameTimer;
	}
	else
	{
		--afterRoundTimer;
		if (afterRoundTimer == 0)
		{
			inGame = false;
			mainActionProcessed = false;
			resetTimers();
			countDown();
		}
	}
}

I still think at the very least AfterRound should be its own state.
Then you could get rid of mainActionProcessed (which is what I replaced didPressButton with) and use a state change instead.

The rest of that should work fine, seems to solve the multiple points issue and is somewhat more straightforward than the original 3-part if.
Whether or not it’s more readable is debatable, but I think it seems more readable.

What do you think?