3 ms·

Ask HN: Budding game programmer humbly requests code critique

16 points by slow_bro 17y ago

Recently rejected from a few big Flash game companies because of my code samples. Would really appreciate some pointers or tips from a spare pair of programmer eyes! http://willchou.com/asteroids1.1 http://willchou.com/symptom/ I've done a little prior reading on game architecture and I'm currently looking through the Flixel and FlashPunk libraries for models to follow.

chipsy 17y ago
Here are my notes: Use ON_ENTER_FRAME events and getTimer(), not TIMER. The Timer class is a low-accuracy system good for tweens, but it runs aground of Flash's "elastic racetrack" throttling behavior: Flash will try to keep a balance between rendering frames and code execution and that conflicts with the absolute control a game needs. ON_ENTER_FRAME will keep your main loop running once-per-frame, which is what you want; with getTimer() you can still apply a dT to your objects. You're using strings in your enumerated constants. These will compare slower than integers. However, the bigger concern for performance is..... You're using the Flash transform functions on a per-frame basis for rotations. This will always be slow; software rendering is fast only at simple memory copies and nothing else, which translates to using copyPixels as much as possible in the inner loop. Hence the performant way to architect Flash games is to prerender a giant spritesheet with draw(), use that as a lookup table for each rotation, and to draw the result onto a single surface rather than to maintain objects on the display list, because the Flash display list will do event processing per object. If you need to finely control draw layers, defer the actual rendering operation into a queue for each layer, and do it all in one pass later. Your "GameObject" won't scale well for a more complex game because it mix three concerns(drawing, motion, collision) in a single class. This caveat is true of the Flixel/Flashpunk architectures too; a more factored approach would be to push these things into components and instance them on each entity type. For an example of a more factored entity system look at Pushbutton Engine. Anyway, it looks pretty good overall, but it's a simple game. The companies may be looking for something a little more elaborate.
- slow_bro 17y ago
  Yeah that first point.. I should've changed it a long time ago. Interesting point about the enumerations. I actually used to use ints and starting using strings when I saw some sample do it; I thought that having strings safeguarded better against accidentally equivalent values across enumerations. But now I think of it you can just one-time randomly generate some large unique int per value. I had thought that industry professionals probably move away from the Flash display list. Your third point makes a lot of sense. Thanks so much chipsy! :D
ismarc 17y ago
chipsy covers a good number of concerns, and I unfortunately don't have time to enumerate the full list of issues seen in both, so I'll quickly cover some of the higher level issues: 1. (asteroids) Tracking state in a series of global variables and then relying on that state in timer callbacks to implement your gameloop leads to unpredictable gamerate and framerates. 2. Embedding the core game logic in mxml file is "bad form". I would suggest creating a game class that is instantiated and called from within that initial mxml file, but it's basically to get the launcher. 3. (symptom) You have entirely too many, pointless loops. You iterate over the same list of objects numerous times in multiple locations. A good example would be in the most recent Level.as update() function. You iterate over the GameObjects 3 different times (not counting the inner loop for collision detection) when this can be reduced to 1 loop. 4. Variable names...they're poor. An example is: for each (var object:GameObject in O) { What is O? Scroll to the top of the function. O is the result of getObjects(). Well, getObjects obviously gets some objects, but which ones? What type of collection does it use? Go to getObjects() definition and it uses a Vector, but you use the name O to build the vector internal to getObjects as well. The variable name should, at a minimum, provide a hint as to what purpose it serves. game_objects would be good, or gameObjects, or activeObjects. And, I can only guess what RBC and WBC even mean. I'm guessing maybe it's red blood cell and white blood cell? Even the associated class files don't provide any information, so from the point of view of someone reading the code, RBC and WBC are random letter strings representing a "something"
- slow_bro 17y ago
  Thanks ismarc! I appreciate all your pointers, but I feel that some of them are somewhat subjective. 1. True, chipsy pointed this out and I actually changed it last night. I'm not sure how global variables specifically effect unpredictable gamerate (and I don't think they're actually global anyways - they're class variables of the main game mxml). 2. True, I can see how the application mxml can get too big in code size without a "Game" object. I also changed this last night. ;) 3. You're right that functionally all of those loops can be reduced into one, but I feel that that comes at some cost of code readability and extensibility. For example, if my game logic was 10x longer than it is now it might be best modularized into discrete methods that reflect the separation of loops that I have currently. 4. Fair enough, but I think that this point is also somewhat subjective. E.g. the meanings of RBC/WBC are pretty clear with a single class definition lookup (it's right there in the class comment). I hope that my response doesn't seem ungrateful, I really appreciate your help. Please let me know if you think I'm mistaken on any of these points (I am learning after all).

16 points by slow_bro 17y ago

chipsy 17y ago

Here are my notes: Use ON_ENTER_FRAME events and getTimer(), not TIMER. The Timer class is a low-accuracy system good for tweens, but it runs aground of Flash's "elastic racetrack" throttling behavior: Flash will try to keep a balance between rendering frames and code execution and that conflicts with the absolute control a game needs. ON_ENTER_FRAME will keep your main loop running once-per-frame, which is what you want; with getTimer() you can still apply a dT to your objects. You're using strings in your enumerated constants. These will compare slower than integers. However, the bigger concern for performance is..... You're using the Flash transform functions on a per-frame basis for rotations. This will always be slow; software rendering is fast only at simple memory copies and nothing else, which translates to using copyPixels as much as possible in the inner loop. Hence the performant way to architect Flash games is to prerender a giant spritesheet with draw(), use that as a lookup table for each rotation, and to draw the result onto a single surface rather than to maintain objects on the display list, because the Flash display list will do event processing per object. If you need to finely control draw layers, defer the actual rendering operation into a queue for each layer, and do it all in one pass later. Your "GameObject" won't scale well for a more complex game because it mix three concerns(drawing, motion, collision) in a single class. This caveat is true of the Flixel/Flashpunk architectures too; a more factored approach would be to push these things into components and instance them on each entity type. For an example of a more factored entity system look at Pushbutton Engine. Anyway, it looks pretty good overall, but it's a simple game. The companies may be looking for something a little more elaborate.

slow_bro 17y ago
Yeah that first point.. I should've changed it a long time ago. Interesting point about the enumerations. I actually used to use ints and starting using strings when I saw some sample do it; I thought that having strings safeguarded better against accidentally equivalent values across enumerations. But now I think of it you can just one-time randomly generate some large unique int per value. I had thought that industry professionals probably move away from the Flash display list. Your third point makes a lot of sense. Thanks so much chipsy! :D

ismarc 17y ago

chipsy covers a good number of concerns, and I unfortunately don't have time to enumerate the full list of issues seen in both, so I'll quickly cover some of the higher level issues: 1. (asteroids) Tracking state in a series of global variables and then relying on that state in timer callbacks to implement your gameloop leads to unpredictable gamerate and framerates. 2. Embedding the core game logic in mxml file is "bad form". I would suggest creating a game class that is instantiated and called from within that initial mxml file, but it's basically to get the launcher. 3. (symptom) You have entirely too many, pointless loops. You iterate over the same list of objects numerous times in multiple locations. A good example would be in the most recent Level.as update() function. You iterate over the GameObjects 3 different times (not counting the inner loop for collision detection) when this can be reduced to 1 loop. 4. Variable names...they're poor. An example is: for each (var object:GameObject in O) { What is O? Scroll to the top of the function. O is the result of getObjects(). Well, getObjects obviously gets some objects, but which ones? What type of collection does it use? Go to getObjects() definition and it uses a Vector, but you use the name O to build the vector internal to getObjects as well. The variable name should, at a minimum, provide a hint as to what purpose it serves. game_objects would be good, or gameObjects, or activeObjects. And, I can only guess what RBC and WBC even mean. I'm guessing maybe it's red blood cell and white blood cell? Even the associated class files don't provide any information, so from the point of view of someone reading the code, RBC and WBC are random letter strings representing a "something"

slow_bro 17y ago
Thanks ismarc! I appreciate all your pointers, but I feel that some of them are somewhat subjective. 1. True, chipsy pointed this out and I actually changed it last night. I'm not sure how global variables specifically effect unpredictable gamerate (and I don't think they're actually global anyways - they're class variables of the main game mxml). 2. True, I can see how the application mxml can get too big in code size without a "Game" object. I also changed this last night. ;) 3. You're right that functionally all of those loops can be reduced into one, but I feel that that comes at some cost of code readability and extensibility. For example, if my game logic was 10x longer than it is now it might be best modularized into discrete methods that reflect the separation of loops that I have currently. 4. Fair enough, but I think that this point is also somewhat subjective. E.g. the meanings of RBC/WBC are pretty clear with a single class definition lookup (it's right there in the class comment). I hope that my response doesn't seem ungrateful, I really appreciate your help. Please let me know if you think I'm mistaken on any of these points (I am learning after all).