Notes from Noah’s Ark Game Refactoring

Recently, I mostly finished a fairly large scale refactoring of code for the Noah’s Ark game.  I’m pretty pleased with how it turned out – the code seems simpler, and I think I’ll be able to more rapidly add features.

Part of the refactoring was motivated by some perceived complexity introduced by some architectural ideas I was trying out – namely, “entities”, “components”, and “systems” – that I’d roughly borrowed from other game engine designs that I’ve read about through the years.

Needless to say, those ideas didn’t seem to be working very well for the game I’m working on.  From various things I’ve learned over the last year, it seemed like there were better solutions to things I was dealing with that would allow replacing a lot of complexity with simplicity.

Will the game’s architecture ultimately return to code similar to what it looked like before the refactoring?  Maybe.  But for now, I think the code is in a much better state than it used to be.  Miscellaneous notes from the refactoring are below.

Making main() Meaningful

Originally, I had a very small amount of code in main():

int exitCode = NoahArkGame::RunGame();
return exitCode;

The bulk of the game logic was contained in a NoahArkGame class.  Part of this was a holdover from when I was using Haaf’s Game Engine before switching over to SFML, but even then, it was largely unnecessary.

So I eliminated this NoahArkGame class and basically moved its logic directly into main().  Why?  Well, using this class (along with some others – “state” and “system” classes) resulted in more indirection that was making things a bit harder to understand, force-fitting certain things into a structure that might not actually be appropriate.  Putting the core game logic in main() meant I could more clearly see the connections necessary between different parts of the code and would make it easier to modify as I discovered new things.

Another interesting idea is that many people often describe a program has having a “main loop.”  Well, if you’re someone new to a code base, where would you expect to find the code for the “main loop”?  Without introducing new constructs that someone would have to learn, the first obvious answer that comes to mind is main().  Anything else is introducing a layer of indirection that has to be jumped through.  I should note that I may not 100% agree with this philosophy, but at the very least, there’s nothing inherently bad about putting a lot of code for the Noah’s Ark game directly in main() for now.  Don’t be afraid of main().

One final note – putting logic directly in main() does have the downside of making it harder to unit test (not that I was planning to do so with the NoahArkGame class either).  But a lot of the pieces orchestrated by the main loop (i.e. actual user input, rendering to screen, etc.) are inherently harder to unit test anyway, so trying to “unit test” the could be a near futile effort anyway.  Just a valid trade-off to consider; there may be more effective ways to test the main loop than unit testing too.

Cleaning Up Tile Map Code

I had a ton of code/classes related to dealing with “tile maps” in the game.  Some of this was intentionally duplicated as I was refactoring smaller parts of the game, but even accounting for that, it was still a messy sea of complexity.

I ultimately eliminated several classes – OverworldMap, ScrollableOverworldMap, TileMapBuilder, TileMapObjectLayer, TileMapTileLayer, ITileMapData, ITileMapLayer, OverworldMapSpecification, SurroundingTileMapLoader, TileMapLayers, TileMapLoader, TiledMapJsonFile – that didn’t really have the best abstractions for useful and intuitive “real-world objects” (in the context of the problems of the game) and thus made the code as a whole more difficult to understand.  That left what seems like a more intuitive collection of classes.

A ton of code was also moved/removed from the TileMap class – it either wasn’t needed anymore or seemed better to place elsewhere for now (to avoid making the TileMap class harder to understand).   I should note that the name of the TileMap class may not best fit the abstraction that it represents, but it isn’t quite clear to me yet exactly what the best class name would be.

Loading All Assets At Startup

I was previously loading assets (i.e. tile map/image files) from disk on-demand, only when needed.  That resulted in some jitteryness/slowdowns during gameplay that required additional complexity to solve.

I did some calculations, and it seemed reasonable to fit basically all of my game assets in memory at once (i.e. 50-60 MB of total usage; my “personal limit” is to keep the game under 100 MB of usage).  Furthermore, most of the assets would be needed in memory basically for the entirety of gameplay anyway, so there isn’t going to be much of a reason to try and free up memory for them.

Therefore, I changed things to just load all assets into memory at startup.  This has the downside of taking about 6-8 seconds (it was 12-14 seconds before I added a tiny bit of multi-threading for tile map loading).  But I’m not too concerned about this since I think I can ultimately have most of this go on in the background during an intro sequence that will be added later.  The good news is that the rest of the code generally doesn’t have to worry about asset loading times.

Making Member Variables Public

Another significant change I made was to make most member variables public instead of private.  This is a common “no-no” in encapsulation/object-oriented guidelines, but I’ve been questioning it lately.

First off, nothing is truly “private” – all there really is is data in the computer’s memory.  Various programming languages do somewhat good jobs at trying to protect these private variables from external access, but this is often just a compilation error.  In C++, you can still easily access private members using pointer offsets.  And some languages don’t even properly support “private” member variables – everything is public, and you have to jump through extraordinary hoops to emulate privacy.

So privacy either doesn’t exist in some programming languages and can be subverted using [often bad] techniques in others.

What are the supposed benefits of private members?  Primarily, the intent seems to be to encapsulate/hide implementation details.  There’s certainly some value in that, but just because a piece of data can be thought of as a member of a class, that doesn’t necessarily mean it is an “implementation detail.”  Sometimes, a data member can intuitively be thought of as part of the abstraction presented by a class.  In these cases, it seems to make more sense for that data member to be public – users expect that data to be in the class by virtue of the abstraction it is trying to present.  Trying to hide this and provide public getter/setter methods just results in more code to read/write/maintain and often makes a class harder to use.

Furthermore, most of the “problems” I tend to see in code aren’t actually caused by lack of “private” members – they’re more fundamental design problems.  As code has to get more complex to solve more complex problems, the impact of a public/private member seems to get tinier in the context of these larger design issues.  And if we’re trusting our developers to solve these much more complicated problems, shouldn’t we be trusting them to make responsible decisions when it comes to data access?  In short, the usage of a data member is more of the issue rather than its public/private accessibility.  Good developers should be able to notice code that is potentially accessing a private implementation detail in a negative way and avoid that code entirely.  Rather than treating a symptom of the problem, I’m wondering if perhaps we should focus more on addressing what seems to be the root cause of these problems – mentorship and growth for programmers.  We want programmers to become better people, and not just so that they can craft simple yet exceptional software.

All of that said, I think there is still probably a case for using private members and having public member functions in public API design – you don’t want to cause frustration or unnecessary work for your users if you need to change some implementation detail or data representation.  But for “internal” code that you don’t intend to heavily reuse throughout tons of projects, I question the value of private members.  It’s your code, and you can change it however you see fit.  And if you do need to change something, the compiler (at least in languages like C++) can easily point you to all of the places that need to be changed.

Not Using Classes For Everything

Last but not least, I deleted some classes that were just serving as cruft and making it harder to use/modify code.  In particular, the GraphicsSystem and CollisionSystem classes were eliminated, with the code now existing as free-floating functions in separate files.

I still find [at least what I consider] “object-oriented programming” to be valuable in certain places.  Primarily, allowing a human to work intuitively with “objects” he/she is familiar with is powerful for solving certain kinds of problems.  Classes can also provide a nice home for functionality that can make it easier to find code in a large code base.

However, object-oriented programming certainly is not a silver bullet.  Not every concept is necessarily best represented as an object.  Too many classes can make a code base harder to navigate.  And object-oriented programming can result in too much indirection and structure that could make code harder to understand or change (which can be costly if an object-oriented design is flawed).

Might the free-floating functions eventually end up in a class?  Perhaps.  If I can find a good home for them eventually, that’s something I’d kind of like to do.  But I don’t yet know what that home would be, and right now, free-floating functions makes it easier for me to shape the code into a proper design later.

 

Anyway, that sums up a lot of the major changes I recently made for refactoring code for the Noah’s Ark game (and why).  It’s entirely possible that something I’ve mentioned above is horribly wrong or stupid.  Time will tell, but I think the code is better now than it was before.

Happy coding.

Fixing Memory Usage Issue for Noah’s Ark

I recently ran into a memory usage defect for my Noah’s Ark game. Basically, as the player scrolled to different tile maps in the overworld, memory usage would continue to climb, eventually bringing the game to a crawl. The fix was really simple but involved some more subtleties of C++ smart pointers, so it seemed worth writing about.

Graphics Management
First, it is important to understand how graphics are currently managed in the game code. Different types of graphics components are created from the GraphicsSystem. The GraphicsSystem is then responsible for rendering all of the graphics on screen.

I’m not necessarily sure this is the “best design” for the game (if you have other ideas, feel free to suggest them!). It is just something I’m experimenting with from various things I’ve read (mostly on entity component systems or ways to avoid having all of my game objects have explicit render methods). My main goal was to be able to easily apply global settings to all of the rendered graphics without having to touch a bunch of classes. Time will tell how well this works out.

Map Scrolling
The second major piece of the puzzle is to understand how the game scrolls between individual tile maps in the overworld. At any given time, the OverworldMap holds the current, top, bottom, left, and right TileMaps (assuming they exist):

The tile maps in memory at any given time in the Noah's Ark game.

When the player reaches the edge of the current map, the game scrolls to the appropriate adjacent map (if one exists), and then reloads any new maps surrounding this new map being scrolled to.

The Bug
There is a bit of a problem with the above approach, if you look at the details of the specific data types used. The graphics components created by the GraphicsSystem are returned as shared_ptrs and were (before the fix) also being stored as shared_ptrs in the GraphicsSystem class. These shared_ptrs are then used by whatever objects the graphics are for. In this case, these shared_ptrs were also used by Tiles in the TileMap class.

When scrolling to new TileMaps, the old TileMaps would be destructed, decrementing the reference counts for the shared_ptrs for Tiles in the map. However, since the GraphicsSystem still stored these shared_ptrs for the tile graphics, the reference count would still be 1, so the tile graphics would persist in memory. New maps would be loaded, creating new tile graphics components, but all of the old tiles would still exist. Obviously, this is problematic and leads to increasing memory usage over time, which ended up slowing the game to a crawl.

The Solution
I actually was aware of this problem during original implementation of the design outlined above but forgot about it by the time the defect actually manifested itself. This was a pretty major architecture problem, but I didn’t want to abandon the idea of having a centralized place (away from the main game objects) to control rendering.

I couldn’t use unique_ptrs since the graphics components needed to be accessible by both the GraphicsSystem and the individual objects the graphics are for. There are ways around this, such as using raw pointers, but that would make communicating when the resources were to be deleted from the GraphicsSystem pretty ugly (I also don’t like the lack of clarity that raw pointers can present when reading code, but that is for another blog post).

Fortunately, C++ had an elegant solution. Enter the weak_ptr. The description on the linked page is best to read for an understanding (and examples help), but a weak_ptr allowed me to break the > 0 reference count problem described above in a pretty nice way.

When an object like a Tile (part of a TileMap) is destructed, I wanted it to communicate to the GraphicsSystem that the graphics for the Tile could be deleted. To accomplish this, I made the following changes:

The GraphicsSystem was changed to store its graphics components as weak_ptrs instead of shared_ptrs:

std::list< std::weak_ptr<IGraphicsComponent> > m_graphicsComponents;

One nice thing is that the weak_ptr class has a constructor that allows it to be constructed from a shared_ptr, meaning I didn’t have to touch the code that added items to the above container.

Next, I had to update the for loops that iterated over the graphics components to use weak_ptrs instead of shared_ptrs. Before updating/rendering an individual graphics component, I needed to check if the object had already been deleted elsewhere (i.e. the Tile destructor) using the expired method:

if (!graphicsComponentWeakReference.expired())

(See the Wikipedia page for an overview of the concept of weak references.)

Assuming that the weak_ptr hasn’t expired, then the lock method can be called to obtain a usable shared_ptr to the object:

std::shared_ptr<IGraphicsComponent> graphicsComponent = graphicsComponentWeakReference.lock();

Tada! We’re almost done. The above code allows for checking if a graphics component has been deleted elsewhere, but it doesn’t actually solve the problem of increasing memory usage. The weak_ptrs are still stored in the list, so they need to be removed at some point. Fortunately, the list container has a remove_if that allows us to easily remove elements from the container that meet a certain condition (without resorting to the erase-remove idiom). Removing the expired graphics components can be done in what is effectively a single line of code:

m_graphicsComponents.remove_if([](std::weak_ptr<IGraphicsComponent>& graphicsComponent) { return graphicsComponent.expired(); });

That’s it! You can browse the full set of changes for this fix here. Finding and fixing this bug was a good learning experience for me, so hopefully it will be of use to some of you.

Tiled TMX Map Loading and Rendering – Noah’s Ark Game

I recently resumed work on a game based on the Biblical story of Noah’s Ark.  You can find out more about the game (and view its source code) at GitHub.

Anyway, after getting the basic project framework up recently, I moved onto the first major task – loading and rendering a tile map for the game. The first decision to make was deciding on a tile map editor to use.

I’d previously used Mappy and Tile Studio to varying degrees. While great tools in and of themselves, they aren’t really maintained much anymore, and I’m not too fond of their output formats.

This time, I’ve settled on using Tiled. It has a less-obscure XML output format, which generally makes things easier to parse. Adding advanced features to the XML would have minimal effect on the complexity of any parsing logic. There are also a ton of XML parsing libraries out there, so if I needed to write my own parsing library, it wouldn’t be too difficult.

Like most tile map editors, there are already a bunch of of existing libraries for loading Tiled’s TMX maps. Most of these are tied to specific rendering libraries as well. This can be advantageous, but I really wanted a TMX loading library that wasn’t tied to SDL, SFML, or some other library. My intent is to hopefully have some reusable tile map code of my own that I can largely use regardless of rendering technology.

Thankfully, such a library exists: TmxParser. I was relatively impressed with how clean the library’s API is – reading and understanding the class headers was quick and simple. Throughout the course of the past week or so, I worked on integrating TmxParser into the Noah’s Ark game. The final result is a screen like this when booting up the game:

Rendering of a test map loaded from a TMX file

Rendering of a test map loaded from a TMX file

Next on my list of features for the game is adding an overworld map to connect tile maps. Scrolling between tile maps will soon follow. However, I’ll probably be taking a break from working on this game for the next month or so.