Friday, 23 July 2010

A code review of PewPew by Mike Chambers


A very rare event has occurred - the chance to look at, discuss and play with the full source for a Flash game! I've worked with the source to dozens of games by different developers, but I'm normally under a non-disclosure agreement so I cannot divulge the full horrors of what I found there. But Mike Chambers from Adobe has kindly released the full source to his game under MIT open source license, and I thought this could be a great spring-board to open up discussion on the architecture and best practice of making Flash games.

but you also need to download is framework from here: http://github.com/mikechambers/Simple-Game-Framework/archives/master

Mike, hasn't made a game since Flash 4, so if you read this Mike, please take it in the scholarly spirit in which it is intended :)

and to everyone else, remember I am just one guy with some strong opinions, weakly held.

Ok here's my thoughts...

Repository - Mike has not included his framework in the game repository. I'd consider this a bad idea for 2 reasons.
  • A developer coming to the project cold cannot compile it. I believe that the repo of a project should contain everything you need to get started.
  • There is a strong chance that API changes could be made to the framework code that break the build of the game. You really want to use a stable version of a framework rather than having to fix errors every time an update is made to the framework. Then if you want to update to the latest version of the framework you grab the new version and fix all the problems at once. You don't want to come back to make a quick change to your game in 6 months, only to find that you can longer compile because you have changed the API of your framework.
Compiling
  • Mike has chosen to compile from the Flash IDE, which is fine with me and most design-driven agencies. Some developers won't like it, but who cares.
  • Mike has unticked the "include hidden layers" option in the Flash publish settings tab. This is one of my absolute no-nos as it means you can break the build just by hiding and unhiding layers while you're looking around the timeline. If I hadn't encountered this lunacy before, I might have spent hours figuring out why it suddenly wasn't compiling. In the end I had to revert to the original version because I couldn't remember exactly what should and shouldn't be hidden. This is what guides are for, people.
  • One mistake Mike's made that I see quite a lot is that he hasn't put his Main.as document class file in his package structure, it's just in the root of src with the fla. The Main class is just as much a part of the project as anything else, and should be in the package with it's friends.
Game Design
  • the game is a pretty basic asteroid-type shoot 'em up.
  • the home-made visual and audio style is acceptable in the Flash game world, although Mike could have chosen a more hand-written looking font, rather than Helvetica. There are lots of free handwriting fonts out there, and there's always Comic Sans!
  • the controls are optimised for a touchscreen mobile device, with a little virtual joystick in the bottom corner, so it doesn't control well on the desktop.
  • the joystick doesn't allow you to set speed, only direction. A simulated analogue stick would have been nicer.
  • There is some commented-out code for making the ship follow the mouse for browser play, but it is incomplete. Mike would have done better to have a boolean for isMouseControlled which could be switched when targeting the browser, rather than completely commenting out the code. I rarely delete or comment out working code, I'm much more likely to keep it in my class as a switched-off option.
  • At 480x800 it's too tall for a browser game, as you have to consider users with a minimum 1024x768 display.
  • The movement of the ship is not very graceful - it doesn't seem to accelerate or decelerate.
  • The speed of the bullets is way too slow.
  • The collision detection between the bullets and enemies is inconsistent, and bullets often sail under the wings of the UFOs without scoring a hit.
  • There are no roll-overs or even finger cursors on any buttons - again this is probably an artefact of originally being a touch-screen game.
  • There aren't any transitions between screens.
  • The enemies bounce around the screen like asteroids, but they are UFOs, which doesn't make any sense, as you would expect UFOs to know you are there and either attack or make defensive manoeuvres.
Library
  • Mike's library is beautifully arranged into folders with all the items named properly, and no Symbol 1's lying around. I rarely organise my libraries into folders any more - I just rely on the invaluable CS4/5 library search box.
  • Mike has linked library items directly to classes. This is what I often do as it is fast and makes it easy to understand what code relates to what graphics. It can have it's draw-backs though, for example if you want to runtime load assets from multiple swfs without having to republish them with every code change, and I'm coming round to the idea that it is often better to treat movieclips as "skins", with no code of there own, and have another class that controls them. It would be nice if at runtime you could tell Flash - take this movieclip and make it an instance of this class. That would be cool right, and totally feasible, Adobe engineers?
Code style
  • All Mike's code is beautifully organised, with well-named full-word function and variable names like isOnStage, displayTitleGraphic and onGameOver. This is painfully rare to see, so I salute you Mike!
  • Mike has commented to the point of obsession, which is great, but as his code is so well named and self documenting, most are unnecessary, such as:
    //start the game
    gameArea.start();
    //added to enemies vector
    enemies.push(enemy);
  • Throughout his code Mike has used // double slash comments on his public vars and functions, where he should have used /* slash star */ JavaDoc style comments. This would have allowed code editors like FlashDevelop to give the comment as a hint / tooltip. Really useful when working with an unfamiliar API.
  • There are some slightly strange things with splitting single lines of code over multiple carriage returns, with orphans tabbed weirdly across the page. This is either a Mac/PC formatting issue, or a sign of madness.
Architecture
  • Mike wrote in his blog that he thought he had over-engineered the game and he's not wrong. I ran a little app called cloc (count lines of code) on the folder and it told me that not including white space and comments there are 2420 lines of ActionScript. That's quite a lot when you consider that I managed to get a similar game running in 25 lines of code. Not all the code in a framework needs to be used on every project though, so it's not as bad as it sounds! If you ignore the framework it's only 1644 :) I have a feeling cloc might be over reporting because of block comments, but I might be wrong. So where is all that code going?
  • The GameArea class contains most the game logic, and is a fairly typical well organised game class. I has more white space and comments than it does code though, which I think makes it less readable. Overall this class is very sane though and doesn't include anything particularly wacky.
  • SoundManager doesn't do much - it doesn't even play sounds! Instead it returns and instance of a sound which you can then call play() on. It also contains constants for each sound class name in his library. This might sound like a good idea, but it isn't. Either use the class reference directly or just put the string in your function call. Not every string you ever type has to be put in a constant.
  • For entities there is an inheritance chain of MovieClip > GameObject > PewPewGameObject > Enemy > ChaserEnemy. I've used this kind of approach on many games, and it works up to a point, but it doesn't make your code very reusable, and you often have to hunt around different inheritance levels to find code. These days I use a very simple component system (nothing to do with native Flash components by the way) where functionality is broken down into modules which are owned by objects. So you would have a health component, a weapon component, a position/movement component, a sprite component etc. In Flash we lurve inheritance because it makes us feel like proper programmers, but composition is often a much better approach. This has taken me literally years to come to terms with, so I won't be surprised if lots of people disagree with me. Mike would also be in trouble if he wanted to adapt his game to use Away3D for rendering for example, as he has extended MovieClip. That said, you should always focus on the game you are making rather than worry about "what if" scenarios that will never happen.
  • Mike is using standard Flash events in his game. Again, I've done this many times myself, but since AS3Signals comes out I just use that - but TurboSignals is supposed to be even faster and so probably more appropriate for games. If you want real speed don't use eventing at all - give each entity a reference to your game class and call methods directly on that.
  • Mike has a reusable GameObjectPool class for object pooling, which is nice. However, when you ask it for e.g. a ChaserEnemy, it returns an instance of GameObject which must then be cast back to ChaserEnemy anyway, so he might as well of made an object pool that supports any class type, not just GameObjects.
  • Mike also has the start of some nice util classes for useful Maths functions, but they're not particularly extensive.
Conclusions

Overall PewPew has the hallmarks of a disciplined programmer with good style. Unfortunately it also has many of the limitations that 90% of Flash games suffer, which is a lack of some basic features that you would have seen in games from 20 years ago:
  • A health bar rather than instant death.
  • A scrolling world.
  • A solid world with walls, rooms, corridors etc, rather than just an open void.
  • Basic physics like momentum
  • Ability to pause the game
  • Particle effects for trails and explosions
  • Animated sprites
Good on Mike for releasing his code for public scrutiny - I worry that this is the extent of Adobe's knowledge of game development though. Their gamedev hub http://www.adobe.com/devnet/games/ seems to have been mostly contributed by 3rd parties, and it even includes some advice on such topics as making games with Cairngorm, which is never going to end well. Flash is badly in need of an official gaming API/framework on the scale of Flex. I don't see that happening any time soon, so until then we're all just going to keep re-solving solved problems and repeating the mistakes of the past.




8 comments:

Seb Lee-Delisle said...

Hi Iain,

great post, thank you for this - I've been meaning to review this code so thanks for saving me the trouble! And hats off to Mike - I still find it scary revealing my source to the world :)

Framework included in the source? Totally agree. We'd use SVN externals to do this. You can include one SVN repo's folder within another, and even specify a revision. Not sure how you'd do this with GIT though, someone mentioned submodules.

And totally agree re hidden layers / guides. Use guides!

I don't mind the app class in the root, at least it's quick to find. My personal preference is to name it after the app rather than Main - we're not Java coders ;-)

Agree re the control mechanism - it's actually quite hard to get touch screen joystick control right and many Flash games really suffer from this on mobile (the fact you can't use cursor keys sadly renders most Flash games unplayable on mobile). One game that's got it really right is geometry wars on iPad and iPhone. Not sure about the directional firing but the motion controller with the left hand is spot on.

Totally agree re linking code to MovieClips - we just don't do it anymore and it greatly helps the collaborative process between art and code.

I think you get a little picky when it comes to transitions, I guess it would benefit from extra polish in that area but not that important. And interesting points about other types of polish, particles etc. I think you'd start to find the limitations of the mobile devices if you went too crazy in those areas, but should def try!

And instant death really isn't a problem if the rest of the playability is adjusted for that. Call me old fashioned if you like!

But thanks again for a great write up and thanks Mike for allowing us to delve around in your private space :-)

Seb

Dave Reynolds said...

Nice post Iain.

I like your point on composition vs excessive inheritance. I see this all the time along with overuse (abuse!) of interfaces. It seems like some people think more OOP = better.

I disagree with you on your suggestion of passing game entities a reference to the main game class. To me this is just tighter coupling which should be avoided. I think events or signals (as you mentioned) are preferable to this.

Thanks for the code Mike! I always learn something new when I look at other people's code.

mesh said...

Awesome write up.

Im not going to go over all of the points, but I did have a few clarifications.

--
For entities there is an inheritance chain of MovieClip > GameObject > PewPewGameObject > Enemy > ChaserEnemy. I've used this kind of approach on many games, and it works up to a point, but it doesn't make your code very reusabl
--

I completely agree with you on this, and this is one of the main things I meant when I talked about over engineering. If / when I do another game, I will almost certainly not use inheritance so extensive for game items. It really leads to a lot of complexity and refactoring as you develop the game.

Transitions between screens : i actually originally had this in about a year ago, but had to remove them for performance reasons. However, they should work fine now on the new android devices.

I use a circle based collision detection, which as you noticed is not always exactly accurate. This was again done for performance (on the iphone). I actually had versions of the game over the past year which used just about every collision detection strategy imaginable. I suspect, that, once again, using pixel perfect collision detection would work just fine on the android devices (maybe ill add that back in).

Ill fix the "include hidden layers" issue.

Anyways, great write up. This is why I released to code. I really wanted to help generate this type of discussion.

mike chambers

mesh@adobe.com

Andreas said...

Pretty cool article, though like most code reviews I've seen/been subjected to, it often comes down to subjective beliefs, for instance composition vs inheritance. And every time I see signals pop up when performance is concerned I have to scratch my head; Performance intensive code has no business being event driven in the first place, and at that point the performance benefits to omitting native events become entirely negligible. Beyond that, a rudimentary observer pattern will have you covered to the max if you really need events and want messaging. The described scenario goes something like thousands of events make for lots of cloned event objects and therefore more left to GC and poorer performance, but I have no idea who would fully justify that kind of code in any project requiring fast real-time performance, or even dispatching that many events either. It's just a weird edge case that I refuse to believe we actually struggle with.

In Flash we don't love inheritance because it "makes us feel like proper programmers". We love it because it's a fantastic tool and composition aside completely and utterly indispensable for anything resembling a sizable project.

I find it a little odd that you point out that inheritance makes for less reusable code, which i just can't agree with; Poor planning is what makes for less reusable code. I agree that excessive inheritance doesn't bode well for anything, but composition and inheritance go hand in hand; It's not an either-or proposition.

Formal code reviews (I realize this is informal ;P) are really only useful if both sides of the table have intimate knowledge of the goals, time frame and history of the code in question. For instance when reviewing CHANGES to a product, and evaluating them to ensure the quality and stability of the product isn't compromised. When it boils down to dude A talking about dude B's code without any context, I have to admit I actually find it a tad offensive.

Another pet peeve of mine is reliance on on-screen "buttons" for touchscreen control. This isn't "optimized for touch screens", it's "shoehorned into touch screens". Touch UI has nothing to do with buttons, and the more designers choose to go with that paradigm the worse the world is off for it.

End rant :) I'd love to see you do a post mortem of one of your own games next, perhaps one you did a year or so ago, with the same granularity.

Iain said...

Hi Andreas. Thanks for your input. I am not sorry for offending you :)

If you have worked at a company that actually does code reviews, then you are one of the lucky ones. Most of us are stuck coding away in a corner without a clue how other people work or better ways things could be done.

There is an element of subjectivity in this of course, but isn't that what makes the discussion interesting?

I wasn't trying to suggest that you shouldn't use inheritance at all, just that a combination of polymorphism and composition is best. I've never yet seen composition over-used. The problem with Mike's code is the number of levels of inheritance rather than the fact that he is using it at all. Inheritance gives you code reuse within a project, but too much hierarchy absolutely makes your code harder move to another project. Imagine trying to add a ChaserEnemy from PewPew to another game. You'd also have to bring along all it's parents including PewPewGameObject, etc, which you really don't want in your new game. Now if you had a flat structure where everything is just an Entity, you could port it across no problem. Or imagine if you wanted to reuse some functionality from Enemy in a class that doesn't extend enemy, e.g. the PlayerShip. You will have to right the same code twice as Flash doesn't have multiple inheritance. Check out Ben Garney's articles about PushButtonEngine for some more info on why this is a good idea.

Re. Signals, I use them because I like to have events as part of my code-contract/interface, so that they show up on autocompletion menus etc. That's why for speed I suggested Turbo Signals, or just passing a reference to your main Game class to each entity and calling it's methods directly. Native AS3 events just aren't sexy, for all the reasons Robert Penner lists in his introduction to Signals.

Agree with you 100% re. buttons on touch screen. Sliding between screens etc is much nicer.

Re. doing a post-mortem on my old projects, that's a notion too horrifying to even consider :)

I am quite proud of my Open Source Gamepad project, and I would genuinely appreciate your thoughts (http://github.com/iainlobb/Gamepad) The hard part is getting someone to go to the effort of actually looking at your code!

Sunjammer said...

I wouldn't commonly consider getting a code review a good thing, mostly because hey, if you learn nothing from it and it becomes a religious debate, what good was it to anyone? We never did code reviews in any company I've worked for (I suspect due to them not really taking Flash seriously as a development challenge), but I've had third parties taking over code maintenance do them, often taking over code that was never even intended to be maintained in the first place. The results are harrowing.

The best way to learn about your own style and where it sits in the ecosystem, i think, is to code with someone else and work together on projects as equals. Granted that's also somewhat rare in our business, so those moments are to be cherished. It's so much better to review decisions BEFORE you commit to them rather than look at them in retrospect and feel like a jackass when someone else calls you out on your mistakes.

I dunno man. I think I might be antagonizing code reviews a little much, but I really believe there's a time and place where they are valuable, and without context that isn't it.

It's a rare project when you can look back at it and be completely happy with everything you did, which is why it's so nice to go back with new knowledge, look at how you did things and recognize how you can now do them better. Sometimes your old solutions will actually impress you, I guarantee it :)

We should make a game sometime, it'd be fun to compare techniques.

Sunjammer said...

Also, Gamepad is rad. I don't have anything to say about it beyond that. Your style is impeccable. Much cleaner than mine, which I don't really have much pride in ;)

Readable, functional and generous. It's a wonderful utility. I gave up writing my own because of it.

Prince Porter said...

Very nice break-down of the game. I very much like the amount of detail you've included here, excellent work. I also learned about the tooltip option you mentioned in FlashDevelop; I'd done it before, but on accident, didn't notice what was allowing it as I usually use // myself. Overall, excellent break-down, thanks for the good read, and thanks to Mike for sharing his code.