• Advertisement
Sign in to follow this  

Unity Pong Project near completion; looking for Critique.

This topic is 1895 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello community,

I have completed a small Pong project, I am quite happy with it and am looking towards the community now that I am at a point to share it. At some points it got me down, as this was my first outing with SFML and there are some aspects that I definitely need to understand better (such as the ordering of event queues and key-presses), but I am happy that i persevered.

I wasn't sure where to put this, it is kind of an announcement, but I am also looking for a code critique and some feedback. Basically, I have gone back to basics and am trying to code a game as quickly as possible, with as few mistakes as possible, but they definitely exists. Currently I am aware that the Paddle and Ball classes share some methods, and this will lead me to developing an inheritance structure, this is one of the known issues and an example of what I am aware of in my code.

I am trying to find as many shortfalls and understand why some concepts shine, through the experience of not using them.

A few things I am aware of:

  • Hard-coded values,
  • Movement that jutters at times.
  • Over-diluted with methods that could be combined.
  • Unclear comments in some places.
  • A few methods that are not used or have no body.


    I appreciate any feedback on my code and the application itself, and look forward to your comments. Below you will find two rar files attached, one with the exe and one with the Header/CPP files.

    Regards,

    Stitchs. Edited by stitchs

Share this post


Link to post
Share on other sites
Advertisement
Hey Stitchs,

The executable complains about MSVCP100.dll not being present, you might as well want to include that.
Furtermore, I took a quick look at your code, and it seems okay to me. I don't know how much experience you have in C++, but I think this program is written with a good sence of object orientation. One thing though, why do you save the score in the bat, and not in the arena?

Aart

Share this post


Link to post
Share on other sites
One problem I see is that, probably because of hard coded values, what's happening is that no matter what angle the ball hits the player controlled paddle from, the ball always goes towards the bottom of the screen. This can be easily fixed by changing some values however, so it's not that big of a deal :).

Also, before you continue reading, realize I also finished Pong with SFML in C++ only about a month ago.

Another problem in your code is over-commenting. Holy crap, you're probably getting half of what you should be getting done finished because you're commenting obvious things. And if you're having to write comments for obvious things, it's bad code. An example would be this code in your Paddle.cpp:

void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < 800)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > 0)
{
position_.y -= speed_ * elapsedTime;
}
}
}


This is hard to understand, even though you have that comment.

It would have been better to have some const int's like so:

enum Screen
{
Left = 0,
Right = 800,
Top = 0,
Bottom = 600
};


Now look at your code:

void Paddle::SetPositionY(float yPosIn, float elapsedTime)
{
if(position_.y < (yPosIn - 10))
{
// prevents paddle going off screen
if(getArea().Bottom < Screen.Right)
{
position_.y += speed_ * elapsedTime;
}
}
else if(position_.y > (yPosIn + 10))
{
if(getArea().Top > Screen.Left)
{
position_.y -= speed_ * elapsedTime;
}
}
}


Not to mention all the other variables with other names. If you corrected those, then it'd be way, way, way easier to understand. Also, what's with the _ after the name of everything? Edited by superman3275

Share this post


Link to post
Share on other sites
Instead of using if-else chain to manage states, you could have used a nice polymorphism.
[source lang="cpp"]while(running)
{
currentState->update();
currentState->draw();
//change what currentState points to when necessary
}[/source] Edited by lride

Share this post


Link to post
Share on other sites
Thank you all for your responses, I am not currently at my home computer and will make a fuller reply tomorrow, when I have had a chance to compare the notes with my code. Two things I picked up on from Superman and rip-off; the trailing '_' after variables and using 'k' in Enumerated types is something I have been following from the Google Style guide on C++. I wanted to find a way of making my code consistent. If it seems wrong, then I will look into changing that for my next project.

I only pick this out because it is the one I can answer off the top of my head.

Thanks again and I will reply in full tomorrow,

Stitchs.

Share this post


Link to post
Share on other sites

Two things I picked up on from Superman and rip-off; the trailing '_' after variables and using 'k' in Enumerated types is something I have been following from the Google Style guide on C++. I wanted to find a way of making my code consistent. If it seems wrong, then I will look into changing that for my next project.
[/quote]
These are stylistic things, so they aren't "wrong" necessarily. I use the trailing underscores myself in some projects. The "k" prefix is one I haven't seen. But if you think it makes sense for you, that is OK.

Given that C++ places the enum names in the enclosing scope, I prefer to prefix the enum names with some "tag" that groups them, e.g.

enum Direction {
DirectionNone,
DirectionUp,
DirectionDown
// ...
};

// Later
Direction direction = DirectionUp;

Some might find duplicating the name of the enumeration like this distasteful, and I can't say they are wrong.

Some people prefer to place global enums inside a namespace instead:

namespace Direction {
enum Type {
None,
Up,
Down
// ...
};
}

// Later
Direction::Type direction = Direction::Up;

However IMO now your declaration syntax starts getting quite funky.

There isn't a definitive answer, but it is good to think about these things and not just blindly applying them. Edited by rip-off

Share this post


Link to post
Share on other sites

Some people prefer to place global enums inside a namespace instead:

namespace Direction {
enum Type {
None,
Up,
Down
// ...
};
}


With C++11
you can just use
[source lang="cpp"]enum class Direction: unsigned char //You can even specify underlying data type
{
None,
Up,
Down
};
[/source]

Note this doesn't allow implicit type conversion
[source lang="cpp"]int direction=Direction::None;//Error
Direction direction=Direction::None//Ok[/source] Edited by lride

Share this post


Link to post
Share on other sites
I know this reply comes late, I do hope that it is still a valid to post in topic.

I am currently changing the code to match the suggestions made here. I have one question: would a method named 'OnPaddleCollision' be of type Bool or type Void, I can't differentiate between the two. I would of gone with bool, but what I have written in the method does not need to return true or false for the arena to reference, which makes me want to use Void as its' type.

Or, to go against my usual thought train, would I: check for a collision with the paddle/ball in the arena and, if one occurs, call the void Ball::OnPaddleCollision(). Saying it like this seems to make slightly more sense, I guess I just need some outside input to help concrete this in my head.

Thanks and I look forward to your replies,

Regards,

Stitchs.

Share this post


Link to post
Share on other sites

I know this reply comes late, I do hope that it is still a valid to post in topic.
[/quote]
It is perfectly fine. If the thread had started to be a few months old, maybe. There is no hard rule, just common sense AFAIK. Additional leeway would probably be given in this instance as this thread is very specific to your project, as opposed to another member bumping this to ask a follow up question.


I am currently changing the code to match the suggestions made here.
[/quote]
Great! When you're happy with it, you can post it here again for further feedback.


would a method named 'OnPaddleCollision' be of type Bool or type Void
[/quote]
I don't believe there is any sensible meaning to the return value of OnPaddleCollision, thus I would use the "void" version. I'd recommend against including return types when the result is irrelevant to the caller.


would I: check for a collision with the paddle/ball in the arena and, if one occurs, call the void Ball::OnPaddleCollision()
[/quote]
Yes. Edited by rip-off

Share this post


Link to post
Share on other sites
@rip-off:

So, as I said, I am using your notes to improve my game and I implemented Ball::OnPaddleCollision. Then it came to Paddle::OnPoint and I suddenly had a bit of a mind-blank. I was trying to figure out "what does this method need to handle apart from increasing the score by 1?"

I had a brainwave and I wanted to check with you to ensure that I have this right, in my head. I was trying to figure whether the paddle should reset itself on a point, or the Arena. It took me about 15 minutes and then I realised, "If I have a paddle handle it's own reset on a point score, what about the other paddle, and the ball position?". This is definitely something that the Arena needs to handle. To cement what I had just learned, I looked back to the Ball collision method and thought, "Well, when is it appropriate for the Object or the Arena to handle certain functions" such as the balls' direction changing. I imagined having 2 or more balls in play, and this is when it clicked. I realised that if the Arena handled all of these methods (reverse speed, increase speed...) then it will become very messy and confused, because with 2 balls or more, each of these methods need to be repeated. Inevitably, this will make it harder to trace bugs.

To summarise, am I on the correct train of thought? If I am, do I need to try to imagine scenario's like this, to try and make sense of what could happen and how easy it would be if someone using my code wanted to achieve these effects?

Regards,

Stitchs.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
  • Advertisement
  • Popular Tags

  • Advertisement
  • Popular Now

  • Similar Content

    • By GytisDev
      Hello,
      without going into any details I am looking for any articles or blogs or advice about city building and RTS games in general. I tried to search for these on my own, but would like to see your input also. I want to make a very simple version of a game like Banished or Kingdoms and Castles,  where I would be able to place like two types of buildings, make farms and cut trees for resources while controlling a single worker. I have some problem understanding how these games works in the back-end: how various data can be stored about the map and objects, how grids works, implementing work system (like a little cube (human) walks to a tree and cuts it) and so on. I am also pretty confident in my programming capabilities for such a game. Sorry if I make any mistakes, English is not my native language.
      Thank you in advance.
    • By Ovicior
      Hey,
      So I'm currently working on a rogue-like top-down game that features melee combat. Getting basic weapon stats like power, weight, and range is not a problem. I am, however, having a problem with coming up with a flexible and dynamic system to allow me to quickly create unique effects for the weapons. I want to essentially create a sort of API that is called when appropriate and gives whatever information is necessary (For example, I could opt to use methods called OnPlayerHit() or IfPlayerBleeding() to implement behavior for each weapon). The issue is, I've never actually made a system as flexible as this.
      My current idea is to make a base abstract weapon class, and then have calls to all the methods when appropriate in there (OnPlayerHit() would be called whenever the player's health is subtracted from, for example). This would involve creating a sub-class for every weapon type and overriding each method to make sure the behavior works appropriately. This does not feel very efficient or clean at all. I was thinking of using interfaces to allow for the implementation of whatever "event" is needed (such as having an interface for OnPlayerAttack(), which would force the creation of a method that is called whenever the player attacks something).
       
      Here's a couple unique weapon ideas I have:
      Explosion sword: Create explosion in attack direction.
      Cold sword: Chance to freeze enemies when they are hit.
      Electric sword: On attack, electricity chains damage to nearby enemies.
       
      I'm basically trying to create a sort of API that'll allow me to easily inherit from a base weapon class and add additional behaviors somehow. One thing to know is that I'm on Unity, and swapping the weapon object's weapon component whenever the weapon changes is not at all a good idea. I need some way to contain all this varying data in one Unity component that can contain a Weapon field to hold all this data. Any ideas?
       
      I'm currently considering having a WeaponController class that can contain a Weapon class, which calls all the methods I use to create unique effects in the weapon (Such as OnPlayerAttack()) when appropriate.
    • By Vu Chi Thien
      Hi fellow game devs,
      First, I would like to apologize for the wall of text.
      As you may notice I have been digging in vehicle simulation for some times now through my clutch question posts. And thanks to the generous help of you guys, especially @CombatWombat I have finished my clutch model (Really CombatWombat you deserve much more than a post upvote, I would buy you a drink if I could ha ha). 
      Now the final piece in my vehicle physic model is the differential. For now I have an open-differential model working quite well by just outputting torque 50-50 to left and right wheel. Now I would like to implement a Limited Slip Differential. I have very limited knowledge about LSD, and what I know about LSD is through readings on racer.nl documentation, watching Youtube videos, and playing around with games like Assetto Corsa and Project Cars. So this is what I understand so far:
      - The LSD acts like an open-diff when there is no torque from engine applied to the input shaft of the diff. However, in clutch-type LSD there is still an amount of binding between the left and right wheel due to preload spring.
      - When there is torque to the input shaft (on power and off power in 2 ways LSD), in ramp LSD, the ramp will push the clutch patch together, creating binding force. The amount of binding force depends on the amount of clutch patch and ramp angle, so the diff will not completely locked up and there is still difference in wheel speed between left and right wheel, but when the locking force is enough the diff will lock.
      - There also something I'm not sure is the amount of torque ratio based on road resistance torque (rolling resistance I guess)., but since I cannot extract rolling resistance from the tire model I'm using (Unity wheelCollider), I think I would not use this approach. Instead I'm going to use the speed difference in left and right wheel, similar to torsen diff. Below is my rough model with the clutch type LSD:
      speedDiff = leftWheelSpeed - rightWheelSpeed; //torque to differential input shaft. //first treat the diff as an open diff with equal torque to both wheels inputTorque = gearBoxTorque * 0.5f; //then modify torque to each wheel based on wheel speed difference //the difference in torque depends on speed difference, throttleInput (on/off power) //amount of locking force wanted at different amount of speed difference, //and preload force //torque to left wheel leftWheelTorque = inputTorque - (speedDiff * preLoadForce + lockingForce * throttleInput); //torque to right wheel rightWheelTorque = inputTorque + (speedDiff * preLoadForce + lockingForce * throttleInput); I'm putting throttle input in because from what I've read the amount of locking also depends on the amount of throttle input (harder throttle -> higher  torque input -> stronger locking). The model is nowhere near good, so please jump in and correct me.
      Also I have a few questions:
      - In torsen/geared LSD, is it correct that the diff actually never lock but only split torque based on bias ratio, which also based on speed difference between wheels? And does the bias only happen when the speed difference reaches the ratio (say 2:1 or 3:1) and below that it will act like an open diff, which basically like an open diff with an if statement to switch state?
      - Is it correct that the amount of locking force in clutch LSD depends on amount of input torque? If so, what is the threshold of the input torque to "activate" the diff (start splitting torque)? How can I get the amount of torque bias ratio (in wheelTorque = inputTorque * biasRatio) based on the speed difference or rolling resistance at wheel?
      - Is the speed at the input shaft of the diff always equals to the average speed of 2 wheels ie (left + right) / 2?
      Please help me out with this. I haven't found any topic about this yet on gamedev, and this is my final piece of the puzzle. Thank you guys very very much.
    • By Estra
      Memory Trees is a PC game and Life+Farming simulation game. Harvest Moon and Rune Factory , the game will be quite big. I believe that this will take a long time to finish
      Looking for
      Programmer
      1 experience using Unity/C++
      2 have a portfolio of Programmer
      3 like RPG game ( Rune rune factory / zelda series / FF series )
      4 Have responsibility + Time Management
      and friendly easy working with others Programmer willing to use Skype for communication with team please E-mail me if you're interested
      Split %: Revenue share. We can discuss. Fully Funded servers and contents
      and friendly easy working with others willing to use Skype for communication with team please E-mail me if you're interested
      we can talk more detail in Estherfanworld@gmail.com Don't comment here
      Thank you so much for reading
      More about our game
      Memory Trees : forget me not

      Thank you so much for reading
      Ps.Please make sure that you have unity skill and Have responsibility + Time Management,
      because If not it will waste time not one but both of us
       

  • Advertisement