Requesting a code review

Started by
17 comments, last by Kuurde 5 years, 6 months ago

Hey guys!

After some googling, and trying different stuff, I decided to start with very basic games as suggested in this article. The article also suggested to get your code reviewed, so here I am! Today I finally finished my first game, pong. ?

The source code can be found here: https://github.com/Kuurde/Pong

Oh, and you can play it here: https://kuurde.github.io/Pong/

 

Thanks in advance,
Kuurde

Advertisement

I can't really comment on the java side as I'm not very experienced in java, but it looks alright organisation wise for a small game to me, it was quite clear and easy to read.

Although it is isn't necessary for this game, I'd suggest mastering fixed tick rates before moving to more complex games. You may also want to move towards more use of constants instead of fixed values in the code (e.g. 800, 480 appear a lot, making it difficult to change resolution), but in this instance it was clear what you were doing, you did use constants for some values (BALL_SPEED, PLAYER_SPEED).

Commenting was mostly good, not overusing where the function names worked, although there were a few spots that could have arguably done with just a little comment (around e.g. a spritebatch, explaining what it is and why used?).

The challenge will come in maintaining the easy to read / understand structure as the game complexity increases, but this is a good first step imo well done! :) 

Looks good. Here are a few ideas:

- There are arguably potential benefits to making things immutable (function arguments, local and member variables, etc.) and unextendable (classes) where possible, which in Java means use of the 'final' keyword. This may largely be a matter of personal preference, and I think some developers may consider liberal use of 'final' in Java just to be noise. Also, in a small-scale project it probably doesn't matter much. Maybe you're already aware of this and just prefer not to use 'final' liberally, but I thought I'd mention it, just in case.

- Following on what lawnjelly said, something to try in future projects would be greater flexibility with respect to resolution and perhaps aspect ratio. Ideally, you'd be able to set the resolution in a single location and have the game adapt automatically.

- Perhaps keep DRY ("don't repeat yourself") in mind. For example, it looks like at least a couple classes contain similar camera and screen-clearing code. Perhaps this could be refactored into a base class or shared module.

There's a fair amount of code duplication between players one and two that could be eliminated by having an array of players instead of separate playerOne and playerTwo variables.  It's not a big deal for a small project like this, but imagine what would happen if you wanted to allow two-player competitive matches.  Or computer-vs-computer attract-mode matches.  Or more than two players.

4 hours ago, Zakwayda said:

- Perhaps keep DRY ("don't repeat yourself") in mind. For example, it looks like at least a couple classes contain similar camera and screen-clearing code. Perhaps this could be refactored into a base class or shared module.

:) Great point!

Programmer and 3D Artist

In my day job I use java, that's why I chose to work with it for my game as well. I'm a backend webdeveloper, but games seem real hard to make. Maybe it's just me, but it's so easy to write ugly code while doing this. So all your points about code quality are valid.

I'll try to keep a more flexible resolution/aspect ratio in mind for my next game.

@lawnjelly I'm not sure what I should to with the fixed tick rates suggestion? Isn't that what the "getDeltaTime" provided by the libgdx library does for me?

 

Next game will be snake. I'll try to add nicer menus. And I should also figure out how fonts realy work in libgdx. Now I either have small text or blurry text when I try to scale it up.

3 minutes ago, Kuurde said:

 I'm not sure what I should to with the fixed tick rates suggestion? Isn't that what the "getDeltaTime" provided by the libgdx library does for me?

Fixed tick rate is how everyone should learn to structure games (imo), rather than relying on a delta. The gafferongames article explains it well, I also covered it in a recent blog post.

There are many, many reasons why a fixed timestep can make life easier, but I should only need to mention one: imagine you are actually trying to ship a game that lots of people had spent years working on, and under some particular circumstances in testing it breaks in unpredictable ways, and other times works fine. That is the kind of thing that happens with variable timestep.

2 hours ago, lawnjelly said:

Fixed tick rate is how everyone should learn to structure games (imo), rather than relying on a delta. The gafferongames article explains it well, I also covered it in a recent blog post.

There are many, many reasons why a fixed timestep can make life easier, but I should only need to mention one: imagine you are actually trying to ship a game that lots of people had spent years working on, and under some particular circumstances in testing it breaks in unpredictable ways, and other times works fine. That is the kind of thing that happens with variable timestep.

Not sure I agree, at the very least many of the most popular engines in use today don't use this method (UE4 doesn't even used a fixed physics timestep, it is semi-fixed.) Most commonly I see systems get ticked at a fixed rate if they need it (like physics usually is). Other systems tend to just be fed a float delta time, which works fine in practice as long as you don't accumulate large numbers, since those result in significant floating point deviation.

Then again I could say the same for globals and/or singletons, almost every major game engine uses them despite them becoming a warzone-topic whenever mentioned here. A more useful distinction is to understand the benefits and negatives of different architectures and pick one that suits your needs. A fixed timestep for physics updates isn't a bad idea.

1 hour ago, Satharis said:

Not sure I agree, at the very least many of the most popular engines in use today don't use this method (UE4 doesn't even used a fixed physics timestep, it is semi-fixed.) Most commonly I see systems get ticked at a fixed rate if they need it (like physics usually is). Other systems tend to just be fed a float delta time, which works fine in practice as long as you don't accumulate large numbers, since those result in significant floating point deviation.

I think he was referring to beginners in the field of game dev.  Which I would agree with, fixed time steps do a lot to help them learn.  UE4 is NOT a beginner project nor something you should compare to pong ?

"Those who would give up essential liberty to purchase a little temporary safety deserve neither liberty nor safety." --Benjamin Franklin

9 hours ago, Satharis said:

Not sure I agree, at the very least many of the most popular engines in use today don't use this method (UE4 doesn't even used a fixed physics timestep, it is semi-fixed.) Most commonly I see systems get ticked at a fixed rate if they need it (like physics usually is). Other systems tend to just be fed a float delta time, which works fine in practice as long as you don't accumulate large numbers, since those result in significant floating point deviation.

Then again I could say the same for globals and/or singletons, almost every major game engine uses them despite them becoming a warzone-topic whenever mentioned here. A more useful distinction is to understand the benefits and negatives of different architectures and pick one that suits your needs. A fixed timestep for physics updates isn't a bad idea.

Going a little off topic here, and attempting not to get into a 'religious argument' in a hodgmanesque manner, but ... :) 

If you accept that fixed timestep is a good idea for physics, and are going to implement this and it is available, then presumably you would need a good counter-argument to decide to turn this off for game logic (like AI, player movement). After all, many of the arguments for using fixed timestep for physics also apply to logic. There may also be a big benefit to synchronisation between logic and physics.

(Note that it is pretty much universal that with fixed timestep, there is also a frame update with delta, as some operations are better done per frame, so I am not in disagreement with the need for a frame update, just perhaps which operations should go in which.)

On the major engines, bear in mind that one of the biggest factors is they need to be beginner friendly, and they are aware that it is possible to shoehorn in fixed timestep even where they don't natively support. Having said that, Unity and Godot *do* implement fixed timestep, both having fixed update and frame update (although the interpolation was either broken (unity) or missing (godot) last time I looked).

There is some discussion of why UE4 doesn't default to fixed timestep here, but I am not familiar with UE4 myself:

https://forums.unrealengine.com/community/community-content-tools-and-tutorials/87505-using-a-fixed-physics-timestep-in-unreal-engine-free-the-physics-approach

https://forums.unrealengine.com/unreal-engine/announcements-and-releases/4148-physics-sub-stepping?3986-Physics-Sub-Stepping=&viewfull=1

Quote

UE4's sub-stepping is in fact Semi-fixed timestep. I agree with you that "Free the Physics" would be ideal. We actually had a debate about these two techniques and eventually decided on semi-fixed, and here's why:

If you use Free the physics you have to use a timebank. If you want to tick physics at 60fps and you have a frame that took a little bit more than 1/60 you will need to have some left over time. Then you will tick the physics engine with the perfect 1/60 delta time, leaving the remainder for the next frame. The problem is that the rest of the engine still uses the original delta time. This means that things like blueprint will be given a delta time of 1/60 + a bit. 

You can imagine a case where a user would want to use delta time in blueprint to determine the location of an object that's moving at some constant speed by saying "new position = old position + speed * delta time"
In such a case the user will expect the object to move speed * delta time. However, since delta time is longer the the time we tick a single physics frame, we would need to either brake the movement into two physics ticks to maintain the right speed, or we would have to place the object in the new expected position at the end of the frame, but increase speed. Either way the result isn't what the user would expect.

Getting the rest of the engine to use this new delta time would affect many systems, and so we ultimately decided to go with semi fixed. 

If you're interested in this there are some files I can direct you to where you could actually change this behavior quite easily, but it would introduce some of the problems I describe above.

So it seems one of the main reasons for the lack of native fixed timestep in unreal is historical, and having to retrofit to a design that does not support it.

Semi-fixed has the 'temptation' that it removes the need for interpolation (and is easy to implement in an engine, especially as an afterthought, as the unreal post shows), thus it can be attractive as a default, perhaps partly because the users don't have to be exposed to dealing with interpolation. It also has the benefit of removing the delay (up to a tick) between input and effect in the game world. Unfortunately, while it solves many of the 'exploding' problems, it still has the side effect of losing determinism and totally predictable behaviour.

And finally there is the big 'M', multiplayer: clients and servers. What is a frame? Servers don't have frames. Different machines run at different frame rates with different frame spikes. Ticks start to make a lot more sense when you build games in a multiplayer framework.

This topic is closed to new replies.

Advertisement