Flapping Shooter Feedback & Tips

Messy code it is: https://github.com/Quickdrawkid/BAT-GAME/tree/master
Unclean! Unclean!

MOST of the variables should be made into constants or local variables. I did this on the “clean” version, but somehow broke it.

I am not really familiar with github, so let me know if I need to fix a setting or something. I did make a “develop” branch, though.

I really appreciate the assistance.

1 Like

I’ve seen a lot worse. (A lot worse.)

My biggest gripe is that you’re using macros.
The first thing you should do when you have chance is to scrap the macros and use constexpr variables instead.
(And yes, you can use constexpr variables to specify the size of an array.)

The second thing you should do is move your images to a separate header file,
just for the sake of decluttering.
Images tend to be autogenerated as they’re not something you’d want to be looking at often so usually they get shoved in a header file and then ignored until new images are needed.

Eventually you should end up moving more of your code into separate headers so you have several short files instead of one long one,
but at the moment you’re not quite at the point where you should be panicking about it*.

My other gripes are more minor, mainly to do with inconsistency and formatting.
I’d offer work on the formatting, but I don’t know if you’d be happy with the formatting style I’d offer.
Do you have a preference when it comes to brace style and tabs vs spaces?
(Personally I’m strongly in favour of Allman style and tabs.)

Nor was I when I first started visiting the forum.

It takes some time to get used to, particularly because it’s based on git, git is horribly complicated, and a lot of tutorials for it assume you’re using command line git with it,
but I’ve found that GitHub’s interface is actually easier to work with than git, you just have to find the right info.


* When should you start panicking?

As a rough guide:

  • 0…250 lines per file is ideal
  • 250…500 lines is mildly concerning, but not a major issue
  • at 500…750 lines you should really consider refactoring
  • at 750…1000 lines you have a problem
  • at 1000 lines or more I phone the code police and they ship you off to gitmo :P
1 Like

Allman style is actually what we were taught in college, so I dont see anything wrong with that.

The macros will be done away with when I get a chance.

1 Like

I approve of your college. :P

Before I learnt C++ I mainly used C#, where Allman style is more predominant.
However, I’ve tried other styles and I’ve decided that Allman style has more practical merits.

(I actually learnt C# during my college days, but it’s not what they taught us.
They taught us in Visual Basic, and I took up C# in my spare time.)

I’ve made some commits on a local fork that do away with the macros and move the images into a separate header:

If you’d like, I can make a PR (pull request) and you can merge the commits onto the development branch.


Completely unrelated, but I’ve been wondering:
what is your forum icon supposed to be?

No matter how much I stare at it, I can’t figure it out.

Merging the changes is okay with me. I assume I would need to approve changes?


My icon is Destructosaurus. He’s a giant, 6 legged, fire breathing dinosaur. He is the OG kaiju in my little fantasy world. See related pic of my cubicle, bottom left. (The icon is a 16 x 16 sprite for the forbidden future project)

2 Likes

Indeed you would. (Hence why it’s a ‘pull request’.)

Unfortunately GitHub seems to be having issues at the moment.

I’m assuming it’s actually GitHub having the problems and not just me?
Maybe the latest Firefox update broke AJAX or something?

I know what a 怪獣 is, but you lost me with ‘OG’.

I.e. another person attempting a Pokemon-like.
(Or possibly a Digimon-like.)

OG is “original gangster.” Basically, he’s the first and exceptional.

It is not a Pokemon like game, though. It is basically a pacman-esque game where you can destroy parts of the maze to open new paths, along with some other mechanics.

1 Like

Forgive the delay, I’ve fixed my GitHub problem.
(Somedays boredom leads me to do stupid things in the name of science…)

Here’s the PR at last:

I was in such a hurry to get it done that I forgot to retarget it to ‘develop’ instead of ‘master’,
but since there aren’t any other commits in the way,
that can be fixed easily enough by just deleting ‘develop’ and recreating it after the PR has been merged.

Thank you @Pharap for the improvements! I have merged the changes.

I sat down and played with this a bit more this evening with the intent of fixing the collisions. The only thing I could think to change is how the Collide() functions are called.

So here is the code I was using:

bool collisionSonic (int enemyX, int enemyY, unsigned char PROGMEM enemySprite) { //adapted from filmote
    
  for (uint8_t i = 0; i < maxSonics; i++) {

    if (sonic[i].enabled == true) {

      if (collide(enemyX, enemyY - getImageHeight(enemySprite), enemySprite, sonic[i].x, sonic[i].y - getImageHeight(sonicbullet), sonicbullet)) {

        sonic[i].enabled = false;
        return true;
          
      }
      
    }
    
  }

  return false;
 
}

I decided to experiment with the getImageHeight() bit. I could not understand why we needed to subtract it since the XY origin is the top left of the screen. Would not subtracting the height put the collision detection above the actual position? Changing the code to the following seemed to completely eliminate the issues I having:

bool collisionSonic (int enemyX, int enemyY, const uint8_t *enemySprite) { //adapted from filmote
    
  for (uint8_t i = 0; i < maxSonics; i++) {

    if (sonic[i].enabled == true) {

      if (collide(enemyX, enemyY , enemySprite, sonic[i].x, sonic[i].y , sonicbullet)) { //- getImageHeight(enemySprite)  - getImageHeight(sonicbullet)???

        sonic[i].enabled = false;
        return true;
          
      }
      
    }
    
  }

  return false;
 
}

Changing the way the sprite is defined in the function helped eliminate the delayed and non-responsive collisions, as far as I can tell. I don’t really know why, so I will chalk it up to magic.

Removing the height modifiers altogether seemed to fix the collisions-where-there-should-be-none problem. All I could figure is that the height modifier was a way to ensure that all sprites were at ground level in the Steve the Dino tutorial. (And it turns out I was right… I guess that’s what I get for copy/pasting without thinking!)

I still think there is a bit of a timing issue with collisions though. This probably has to do with the crow sprite being drawn after checking for collisions (not confirmed). Otherwise, everything is good and I feel that I can move forward with the project.

Next up is enemy collisions with the bat.

1 Like

Well, for one thing enemySprite isn’t a pointer…

In this version it is a pointer, so that’s a step forward.

The short answer is ‘pointers’.

The long answer requires explaning what pointers are,
which requires explaining what RAM and progmem are,
and possibly what an address bus is.

If you’re up for an explanation then I’m happy to explain it,
but as I say it won’t be a short explanation.

It depends on whether its position is being modified in between the two, and if so how.


While I think of it, what even are ‘sonics’?


Does anyone happen to know where @filmote’s collision function is kept and/or whether it has a licence?

its here > https://github.com/filmote/CollideImage
BSD 3-Clause.

1 Like

I’ve made a quick attempt at improving/normalising the formatting.
I stuck to using 2 spaces becuase I expect you’re still using the Arduino IDE,
but I’ve changed the brace style to Allman style and moved the comments (most of them at least) to above what they describe rather than at the end of the line.

If you’re happy with those changes I’ll make a PR.

I can think of a number of other possible improvements,
but I don’t want to do everything at once in case you find it a bit overwhelming.

I'll leave a list of thoughts/suggestions though...
  • Make it clear that the collide function is under a BSD 3-clause licence
  • Change Collide.ino into a header file, collide.h
  • Rename Sonic (and related variables) to Bullet
  • Combine some of the sprites into sprite sheets so you can make use of the Sprites function’s frame parameter
    • E.g. Sprites::drawSelfMasked(enemyList[i].x, enemyList[i].y, crowSprite, crowFrame);
  • Create a Player struct that combines all the player variables together
  • Separate collision detection from collision resolution
  • Replace uses of if(x == true) with if(x), because the == true is redundant
  • Change the name of some functions - they should be using ‘camel case’, and should start with a verb
    • enemyspawn -> spawnEnemy
    • updateenemies -> updateEnemies
    • collisionSonic -> isBulletCollidingWith
    • enemyReset -> resetEnemy
    • drawcrow -> drawCrow
    • crow -> updateCrow
    • drawbat -> drawBat
    • batfire -> fireBullet?
  • Remove the delay logic from enemyspawn (again, see ‘separation of concerns’)
  • Use a scoped enumeration (enum class) to keep track of the bat’s glide/flap/dive state
  • Use a scoped enumeration to keep track of the enemy type
  • Clarify what modA and modB do (there might be some better options)
  • Possibly use a switch statement in drawbat
    • (I’m willing to bet I know what you were doing wrong before - most likely you left out the break; statement from each case)
  • Maybe settle on a licence for the game itself?

(Not an exhaustive list, but all I could think of at the moment.)

These look great to me.

I’ll have to look up the proper way to do that, but I agree it’s important.

This was something I was doing early on, but eventually changed everything to sonic later (bat echolocation bullets?). This was to account for the future enemy bullets. In retrospect, this is a silly concern… I could just call them enemyBullets…

This is something I wanted to do initially, but did not fully understand how the spritesheets worked. Especially because some sprites, like the bat, are not uniform in size.

This was also something I initially tried, but failed at. So, I stuck with what I knew.

I feel fairly certain that I included the breaks, but I can’t remember. Definitely needs to be revisited. Especially for the future enemy additions.

So this one is a little complicated because modA and modB do completely different things depending on the enemy type. This is a compromise to keep all the enemy variables in the array. The following are some examples.

Crow:

  • A - vertical velocity
  • B - goal Y position, or the altitude the crow tries to maintain

Skull:

  • A - shooting timer
  • B - movement timer


Bosses:

  • A - shooting timer
  • B - hit points remaining

On death, modA and modB would also be changed in different ways depending on the enemy type. I hesitate to call them anything specific for these reasons. I thought it was the simplest solution to the problem, but my knowledge is pretty limited. I made a paper list to keep track of these, but I should have transcribed that list to code comments or a readme.

I am still using the Arduino IDE. It’s convenient for me because I can run it on my laptop when I am at work. (It’s running Puppy Linux, which is not ideal)

I welcome any help or improvements as long as I can follow them (or be taught to follow them).

I thought that I had selected a license on Github, but maybe I did it wrong. That’s all new to me…

In which case, here’s a PR:

Probably best to dump the licence at the top of the file given that BSD-3 is quite a short licence.

Also, probably best to do that at roughly the same time the .ino is changed to a .h.

BatBullet would also work.

Depending on how things pan out you may be able to reuse the same structure for enemy bullets, in which case just Bullet would make sense.

Have a look at this old post, it might clear things up a bit:

That is certainly more of an issue, but it looks like that’s probably a minor problem.

Where it could be more of a problem is with the collision function.
I don’t think the collision function takes frames into account,
so it might have to be modified.

When the PR has been merged I could modify one of the things I mentioned to demonstrate.

You’d have something like:

// : uint8_t means 'use uint8_t as an underlying type'
enum class BatState : uint8_t
{
  // Starts with an underlying value of 0
  Glide,
  // The next entry has a value of 1
  Flap,
  // A value of 2... and so on
  Dive,
};

// The bat starts off by gliding
BatState batState = BatState::Glide;

// ...
// Then later you modify it like:
if (arduboy.justPressed(A_BUTTON))
{
  // ...
  batState = BatState::Flap;
  // ...
}
// ...

There are several reasons why a scoped enumeration is prefered:

  • You don’t actually have to think about what the underlying values are, you can just think of the enumeration values as special names
  • Scoped enumerations are completely type safe, you can’t accidentally use a wrong value
    • E.g. before you could do batsprite = 11; by mistake and wouldn’t know about the bug until runtime, and then you’d have a hard time tracking it down, but by having BatState batState, you cant do batState = 11, that’s an error, you can only assign one of the three designated names

I know of two solutions to this problem, but given the Arduboy’s limited resources only one of them is really viable.

Unfortunately it would take a fair bit of explaining,
because it requires both an enum class and a union to create something called a ‘tagged union’.

Basically the union allows the memory for the different enemy types to overlap, and the enumeration (the ‘tag’) identifies which enemy is currently active.

Are Notepad++ or VSCode viable options?
If so, either of those would be more suitable.

That’s why I’m trying to not do too much at once.

On a second look, you have. It’s MIT.

I was looking at the source rather than what GitHub says.
Usually people put the licence at the top of the code,
or at least at the top of the main file of the code.

Okay, I have merged the pull request and subsequently replaced it with a newer version.

I have added collisions with the enemy and game over/ title screen logic. There are no graphics, yet. I have asked my wife to design these for me. I gave her free reign, but I did offer the suggestion of Koumori Caves or Caverns in large Japanese characters with BAT GAME as an overlay. I sort of like the idea of an alternate Japanese title.

I removed the enemy delay modifier logic as it would interfere with resetting the game in its current implementation.I have also added the license to the Collide code and made it a .h file. No other real changes, but progress is progress!!

I will look into this tomorrow. I had an issue with my laptop at work and could not get onto the internet today.

1 Like

I had to rename batandcollisionsdeath.ino to batandcollisions.ino before it would compile properly.


I’ve made two more small PRs.
One is just a bit of minor reformatting and an added function:

In the other I created a proper EnemyType type like I discussed earlier:

I based the values on your .pdf notes, but I couldn’t quite make sense of one of the lines,
so I called the corresponding value ‘illegible’.

I also changed ‘beholder’ to ‘eyeball’ because ‘beholder’ is actually copyrighted by ‘wizards of the coast’:

Beholders are one of the few classic Dungeons & Dragons monsters that Wizards of the Coast claims as Product Identity and as such was not released under its Open Game License.

Not that I think they’ll bother complaining, but better safe than sorry.

Ok, I will fix this.

This is a stalagtite / stalagmite. If I did not write it myself, I don’t think I would have caught it, either!

This is interesting. I will have to redesign this enemy, then. It will force me to be more creative.
Thank you for the enumeration example and the other updates! They are now merged.

Notepad++ itself is not a viable option for Linux. There’s a few similar alternatives, though.
VSCode is available for Linux, but I again did not get online to download it. I would definitely prefer the white on black environment to the black on white of the arduino IDE. However, the best solution is probably to convert the laptop to Windows 10 before doing anything else.

I was thinking about the separation of concerns regarding the enemies and collisions. The issue I am coming across is this:
The collision is dependent upon the enemy sprite, but this sprite is only known within the enemy code. It is not tracked outside of the enemy (crow) function. This is true for bullet/enemy and enemy/player collisions.
At the moment, the only place I can really put it is the current location…unless I add a frame variable to the enemy structure?
The collision concern also plays into the use of spritesheets. If spritesheets would significantly hinder the use of the existing Collide.h code, would it really be an improvement? Perhaps there is a way to modify Collide.h to account for this by using known sprite widths?

As far as progress goes, I have laid out my approach to two more enemies: the skull, and a lightning bug (possible replacement for the ghost). This was all on paper and untested, but I am confident that it will do what I intend with minimal tweaking.

1 Like

Oh, I see that now.

It’s written “stalag mite/tite” so I was trying to read it as two words.
Also the t crossed over the s, which had me a bit stumped.

You can still have flying eyeballs or a monocular monster,
you should just make it distinctly different from a beholder somehow.

Hopefully the way the scoped enum is used makes sense to you?

If you think you understand it now, have a go at upgrading your game state system from an int and numeric constants to a proper enum class (probably named GameState).

You could always try vim or emacs. :P

Really though all that matters is that you use something with proper IDE features, like a button that renders spaces and tabs.

Pretty much all sensible IDEs have some kind of ‘theme’ system.
I’m the opposite, I’ve purposely installed the light theme for VSCode.

Ultimately in this case the collision code absolutely has to know about the sprite,
but there’s probably still a few ways to keep the interface somewhat separate.

It depends… Either approach could end up being shorter depending on how many images you have.

I’m erring on the side of thinking that repeating the same width and height for different images will eventually outweigh the cost of making the collision code understand frames.

Almost certainly. I could probably figure it out, though it would take me time to read through the code, understand it and modify it.

Basically you can think of a sprite as being something a bit like:

// Pseudocode
struct Sprite
{
  uint8_t width = W;
  uint8_t height = H;
  uint8_t frame[N][W * H / 8];
};

So it should be possible to generalise the collision code to work for any frame by offsetting the address it looks at to find the frame it operates on.

When you have at least 2 more enemies implemented I might step in and demonstrate some alternate solutions to handling the enemy logic.

Update!
I have just added a second enemy to the game. The Wisp is a ghost type enemy which is invulnerable and flies across the screen. The demo at the top of the page has been updated to reflect the current state of the game.

I could have also added stalactites and stalagmites, since they behave similarly to the wisps. They are just more limited in their spawn range. However, I decided to call it quits for the day.
The code is nearly 700 lines at this point and adding/correcting things is becoming a little tedious. After adding the Wisp and correcting a few minor issues that arose, I thought that I lost all my changes by closing without saving! It was a false alarm, as there are now many files with similar name variants to “batwithcollisions” on my PC.
With projects, I usually say that it’s time to take a break when you start making mistakes…

@Pharap
Yes, the scoped enum makes sense. I still have to implement it for the gamestate logic, though.
I had planned on that for today, but I did not get to it before my mini heart-attack.
Question:
If I wish to move all of the enemy logic to another .h file, would that cause any issues with global variables used within those functions?

1 Like

Yeah, you’ll definitely want to start thinking about how to split the code into multiple files soon.

The first thing I’d recommend is moving every type defintion (struct, enum class et cetera) into a separate file,
though you may want to group related types (like EnemyType and Enemy) if you feel that doing so makes life easier.

In fact, looking at your functions, it probably makes sense to move all the background-related code into one file, all the player related code into another and all the enemy related code into another.

That’s why GitHub exists. Commit little and often!

As long as it’s been commited somewhere you can easily go back to that point in history and retrieve the code as it was.

This is where branching comes into play.

Instead of creating X copies of the same file with slightly different names and slightly different states of completeness, add more branches to your GitHub repo and commit your modifications to separate branches,
then download them as needed or delete them when you’re sure they’re no longer relevant.

When you download them, you’ll get a zip of the form <repo_name>-<branch>.zip, which will unpack to a repo_name-branch folder, so it’s easy to tell the different variations apart by looking at their file path.

Yes and no.

No in the sense that you can just stuff all your globals into a single header file and then include that file,
but yes in the the sense that you’ll get problems if you don’t do that.

That said, I frown immensely at the idea of having a globals.h because relying too heavily on global variables is a bad thing, and you should aim to avoid global variables where possible,
either by packing variables into data structures or by passing variables as parameters to functions.

For my games I tend to have a Game class, and instead of having global variables dotted around,
all the would-be globals become members of the Game class,
and I’m left with a single global variable - the instance of Game.

But even then, I try to keep the number of variables in the Game class small.

In your case, before even thinking about a solution like that you should focus on the smaller things.
For example, packing all your player variables together into a single player structure would make the code a lot easier to manage.

And in particular I think learning about references would help simplify a lot of the code where you’re continually indexing arrays.


Also I think renaming your variables and functions would help make the code easier to follow, and thus easier to edit and add to.

Ideally functions should be like verbs, variables should be like nouns,
and the code should be able to be read like English.

For example…

  • Instead of enemyReset(i), have resetEnemyAt(index).
  • Instead of if (collisionWithEnemy(enemyList[i].x, enemyList[i].y, ghostb)), have:
// The '&' means that 'enemy' is a reference
Enemy & wisp = enemies[index];
if(isCollidingWithEnemy(wisp.x, wisp.y, wispImage))

Something like increaseDifficulty() is a good function name because it’s verb-oriented and describes an action.
Something like titleScreen() is not as good because it’s a noun (a titlescreen is a thing) and doesn’t describe an action.
(Something like updateTitleScreenState() would be better - verb-based, describes an action and describes what it does precisely.)


One last thing: x -= y is shorthand for x = x - y.
The same is true for +=, /=, *=, %= et cetera.
They can help readability by getting rid of redundancy.


Sorry if I’ve overloaded you with information,
but it’s a lot easier to get this stuff sorted when the code is still smallish.
If you wait until the code grows larger then you end up having to expend a lot more effort to get everything sorted.